Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d46tX-0005Jn-8y for pgadmin-hackers@arkaria.postgresql.org; Fri, 28 Apr 2017 14:29:15 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1d46tW-00022f-Mm for pgadmin-hackers@arkaria.postgresql.org; Fri, 28 Apr 2017 14:29:14 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1d46tH-0001do-7N for pgadmin-hackers@postgresql.org; Fri, 28 Apr 2017 14:28:59 +0000 Received: from mail-it0-x22a.google.com ([2607:f8b0:4001:c0b::22a]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1d46tC-0006LK-9Y for pgadmin-hackers@postgresql.org; Fri, 28 Apr 2017 14:28:58 +0000 Received: by mail-it0-x22a.google.com with SMTP id f187so40856308ite.1 for ; Fri, 28 Apr 2017 07:28:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pivotal-io.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=FESHeP2ov3SYII8toF31+amHg/5x3IwRjEHFFivLytE=; b=CUIlFB96iGRmu7o4KHuqtdnKCdJ9bwonspy2p+tkYe6clC5BZZ3RRKjEeBF+yoLal4 l8gYJSSHQuh+m7HxNL876f6G68+ZbEGIwudxM7BF/ZrrirP/gJgc1ZKSmtihtgc5LvG6 pe7Wd7dAqUf9/0ERMdM5dKEW7vvvySATIPFLVBcNMUt8l6NQ7lfuo1fpKNccM4webdwF br1vGBl4uhA+HvIWqEBRwv5juGWiLYV2VEHnrx6+Jr5P/uZuMC3S+RXuhxZHRUzntI62 GcPUbH8XE/8ZhxqXER6v8i1MaUVza1zcZVwYJOUG+a6ssPA7t16RdKcd6HkKscfP8xXw TOZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=FESHeP2ov3SYII8toF31+amHg/5x3IwRjEHFFivLytE=; b=g9sFY+ERYLXLcosRq350i7JNuVIWQX9a8XeMkawEXiaXg/b1IfRh76x30Vu0bV8vet 2roqd7WqzQWFzK83oYKYn7XH9KmOJjx85/Cw4GxKrvnYXVsdc4hAUJGBYAGu8GrJcJ15 EX6Qg6x8j9EaI6DJVbq2VOen+Y03Pv34J+UsaeSHyq3jH+2y7MQgtp4RdFZPFp3dKQz/ eM9MBXbApNPuaD/EY9exgxFJy4jyR0sOnCAdHNIcwxasSsV/KQzA38t7gateEksiyUTF u0VhDJp0lXmhKAYcUofFddUmhmBvETFIBOEFcafgLQx1QN3SdKcw4KFNQc1JLcA9ts+3 kG9Q== X-Gm-Message-State: AN3rC/66+tbw7j8ubeAgvwMRLjTW8ply/rDwYAN3ggk74DXwYjWwct/K KgrS/CtZOG0hLfVW2WxFXP+MQJY5PD2a X-Received: by 10.36.98.6 with SMTP id d6mr1628359itc.37.1493389732478; Fri, 28 Apr 2017 07:28:52 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.3.85 with HTTP; Fri, 28 Apr 2017 07:28:31 -0700 (PDT) In-Reply-To: References: From: Matthew Kleiman Date: Fri, 28 Apr 2017 10:28:31 -0400 Message-ID: Subject: Re: [pgAdmin4][Patch]: Fixed RM #2315 : Sorting by size is broken To: Khushboo Vashi Cc: Sarah McAlear , Dave Page , pgadmin-hackers , Ashesh Vashi Content-Type: multipart/alternative; boundary=001a1143d74284374c054e3ae4f8 X-Pg-Spam-Score: -2.6 (--) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgadmin-hackers Precedence: bulk Sender: pgadmin-hackers-owner@postgresql.org --001a1143d74284374c054e3ae4f8 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hi Khushboo, That sounds good. Sorry if we weren't clear at first. Have a good holiday weekend! Sarah & Matt On Fri, Apr 28, 2017 at 4:35 AM, Khushboo Vashi < khushboo.vashi@enterprisedb.com> wrote: > Hi Sarah, > > On Thu, Apr 27, 2017 at 7:38 PM, Sarah McAlear > wrote: > >> Hi Kushboo! >> >> We understand your point, but we believe that relying on 2 independent >> functions to deliver the same formatting can become a problem if the PG >> function changes. Our suggestion is to use a single function in our >> javascript code to do this formatting. >> >> It seems reasonable to me and I am going to use a single javascript > function which will support PB also (as per Dave we should add support ti= ll > PB) . > >> If the community believes we can live with this risk, let's move forward= . >> >> Thanks! >> Sarah & Joao >> >> On Thu, Apr 27, 2017 at 1:50 AM, Khushboo Vashi < >> khushboo.vashi@enterprisedb.com> wrote: >> >>> Hi Joao & Sarah, >>> >>> On Wed, Apr 26, 2017 at 8:46 PM, Joao Pedro De Almeida Pereira < >>> jdealmeidapereira@pivotal.io> wrote: >>> >>>> Hi Khushboo! >>>> >>>> Thanks for your reply! >>>> >>>> >>>>> *SQL Files:* >>>>>> >>>>>> - Is there a way to avoid conditionals here? >>>>>> >>>>>> >>>>>> - Maybe we can use the same javascript function to prettify all >>>>>> the sizes >>>>>> >>>>>> >>>>>> In case of collection node (ex: Databases), I have implemented this >>>>>> functionality via putting a formatter for a back-grid column. So, it= is >>>>>> applicable for the entire column not for the individual cell. To app= ly the >>>>>> javascript function on a single cell for the column (string column),= first >>>>>> we need to identify that cell and then apply the function, I think t= his is >>>>>> overhead. Just to avoid conditional sql template I would not prefer = this >>>>>> approach. >>>>> >>>>> >>>> We are not totally sure we understood what you meant on the previous >>>> statement. Are you saying that the conditionals in SQL are used to ens= ure >>>> that we can apply a javascript function at column level instead of cel= l >>>> level? >>>> >>>> Correct. >>> >>>> Our concern is that the templates are being made more complex and >>>> inconsistencies are introduced in the code and the UI. >>>> >>> >>> Inconsistencies in the UI can be avoided through making the >>> size_formatter same as pg_size_pretty function which I will implement. >>> I have checked the pg_size_pretty function code and it supports till TB >>> format, so I think we should keep till TB only. >>> >>> In this particular example, we are allowing the backend to respond >>>> sometimes with prettified data and sometimes without it, so at UI leve= l we >>>> will have inconsistencies between screens or more complex Javascript c= ode >>>> to support sometimes prettifying and sometimes not prettify the same >>>> fields. >>>> >>>> We have separate logic for collection and single node in statistics.js >>> and we are using javascript code for prettifying only for collection no= de. >>> >>> >>>> What we were thinking was to maybe not specify on the SQL level and >>>> have the same format for "Size" everywhere in the UI. >>>> >>>> >>> Here our main concern is inconsistency in "Size" format in the UI that >>> can be avoided as I said earlier. >>> We are using pg_size_pretty function for different fields like Size, >>> Index Size, Table space size, Tuple length, Size of Temporary files in >>> different modules and some of them are cell level and we don't require = to >>> put overhead on cell level fields as sorting is not required for indivi= dual >>> node statistics. >>> >>> >>> >>>> Thanks >>>> Joao & Sarah >>>> >>>> On Tue, Apr 25, 2017 at 11:48 PM, Khushboo Vashi < >>>> khushboo.vashi@enterprisedb.com> wrote: >>>> >>>>> Hi Joao & Sarah, >>>>> >>>>> Thanks for reviewing the patch. >>>>> >>>>> On Tue, Apr 25, 2017 at 8:34 PM, Joao Pedro De Almeida Pereira < >>>>> jdealmeidapereira@pivotal.io> wrote: >>>>> >>>>>> Hello Khushboo, >>>>>> >>>>>> We reviewed the this patch and have some suggestions: >>>>>> >>>>>> *Python:* >>>>>> =E2=80=8B >>>>>> The functionality for adding the "can_prettify" is repeated in >>>>>> multiple places. Maybe this could be extracted into a function. >>>>>> >>>>>> When I have implemented this, my first thought is exactly same as yo= u >>>>> suggested but while looking at the code I felt its not a good idea t= o have >>>>> a function. In case of a function, we need to pass the whole result-s= et as >>>>> well as the list of fields which we need to be prettified. So, only f= or 2 >>>>> lines, I didn't find any reason to make a function. >>>>> >>>>>> *Javascript:* >>>>>> =E2=80=8B >>>>>> >>>>>> - The class Backgrid.SizeFormatter doesn't seem to have any >>>>>> tests. >>>>>> >>>>>> >>>>>> Sure, will do. >>>>> >>>>>> >>>>>> - The function pg_size_pretty displays bytes and Kilobytes >>>>>> differently. >>>>>> - Is it possible to add PB as well? >>>>>> >>>>>> Will check and add PB. >>>>> >>>>>> >>>>>> - >>>>>> - The function is a little bit hard to read, is it possible to >>>>>> refactor using private functions like: >>>>>> >>>>>> Will make it more readable. >>>>> >>>>>> fromRaw: function (rawData, model) { >>>>>> var unitIdx =3D findDataUnitIndex(rawData); >>>>>> if (unitIdx =3D=3D 0) { >>>>>> return rawData + ' ' + this.dataUnits[i]; >>>>>> } >>>>>> return formatOutput(rawData, unitIdx); >>>>>> }, >>>>>> >>>>>> =E2=80=8B >>>>>> >>>>>> >>>>>> - In statistics.js:326 we believe it would make the code more >>>>>> readable if we change the variable "c" to "rawColumn" and "col" t= o "column". >>>>>> >>>>>> >>>>>> I will change the variable name from "c" to "rawColumn" but I thin= k >>>>> "col" is appropriate as we already have columns variable and anyone c= an >>>>> confuse while reading the code (for columns and column). >>>>> >>>>>> >>>>>> *SQL Files:* >>>>>> =E2=80=8B >>>>>> >>>>>> - Is there a way to avoid conditionals here? >>>>>> - Maybe we can use the same javascript function to prettify all >>>>>> the sizes >>>>>> >>>>>> >>>>>> In case of collection node (ex: Databases), I have implemented this >>>>> functionality via putting a formatter for a back-grid column. So, it = is >>>>> applicable for the entire column not for the individual cell. To appl= y the >>>>> javascript function on a single cell for the column (string column), = first >>>>> we need to identify that cell and then apply the function, I think th= is is >>>>> overhead. Just to avoid conditional sql template I would not prefer t= his >>>>> approach. >>>>> >>>>>> >>>>>> Visually we saw a difference between "Databases" statistics and a >>>>>> specific database statistics. In "Databases" statistics the "Size" i= s "7.4 >>>>>> MB" but when you are in the specific database the "Size" is "7420 kB= ". >>>>>> Is this the intended behavior? >>>>>> >>>>>> Only for the Databases (collection node), the client side >>>>> functionality is implemented not for individual node , so this behavi= our is >>>>> different. For the individual node still, we are using pg_size_pretty >>>>> function >>>>> >>>>>> >>>>>> >>>>> >>>>>> Thanks >>>>>> Joao & Sarah >>>>>> >>>>>> On Tue, Apr 25, 2017 at 7:58 AM, Dave Page wrote= : >>>>>> >>>>>>> Ashesh, can you review/commit this please? >>>>>>> >>>>>>> Thanks. >>>>>>> >>>>>>> On Tue, Apr 25, 2017 at 10:18 AM, Khushboo Vashi < >>>>>>> khushboo.vashi@enterprisedb.com> wrote: >>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> Fixed RM #2315 : Sorting by size is broken. >>>>>>>> >>>>>>>> Removed the pg_size_pretty function from query for the collection >>>>>>>> and introduced the client side function to convert size into human= readable >>>>>>>> format. So, the sorting issue is fixed as the algorithm will get t= he actual >>>>>>>> value of size instead of formatted value. >>>>>>>> >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Khushboo >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Sent via pgadmin-hackers mailing list ( >>>>>>>> pgadmin-hackers@postgresql.org) >>>>>>>> To make changes to your subscription: >>>>>>>> http://www.postgresql.org/mailpref/pgadmin-hackers >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Dave Page >>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>> Twitter: @pgsnake >>>>>>> >>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>> The Enterprise PostgreSQL Company >>>>>>> >>>>>> >>>>>> >>>>> Thanks, >>>>> Khushboo >>>>> >>>> >>>> >>> Thanks, >>> Khushboo >>> >> >> > Thanks, > Khushboo > --001a1143d74284374c054e3ae4f8 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi Khushboo,

That sounds good. Sorry if= we weren't clear at first.=C2=A0

Have a good = holiday weekend!

Sarah & Matt


On Fri, Apr 28, 2017 at 4:35 AM, Khushboo Vashi = <kh= ushboo.vashi@enterprisedb.com> wrote:
Hi Sarah,

On Thu, Apr 27, 2017 at 7:38 PM, S= arah McAlear <smcalear@pivotal.io> wrote:
Hi Kushboo!

We unders= tand your point, but we believe that relying on 2 independent functions to = deliver the same formatting can become a problem if the PG function changes= . Our suggestion is to use a single function in our javascript code to do t= his formatting.=C2=A0

It= seems reasonable to me and I am going to use a single javascript function = which will support PB also (as per Dave we should add support till PB) .
<= div>
If the community believes we can live with this risk, let= 9;s move forward.

Thanks!
Sarah & Joao=

On Thu, Apr 27, 2017 at 1:50 AM, Khushboo Vashi &= lt;khu= shboo.vashi@enterprisedb.com> wrote:
Hi Joao & Sa= rah,

O= n Wed, Apr 26, 2017 at 8:46 PM, Joao Pedro De Almeida Pereira <= jdealmeidapereira@pivotal.io> wrote:
Hi Khushboo!

Thanks for= your reply!
=C2=A0
SQL Files:
  • Is there a way to avoid conditiona= ls here?=C2=A0
  • Maybe we can use= the same javascript function to prettify all the sizes

In cas= e of collection node (ex: Databases), I have implemented this functionality= via putting a formatter for a back-grid column. So, it is applicable for t= he entire column not for the individual cell. To apply the javascript funct= ion on a single cell for the column (string column), first we need to ident= ify that cell and then apply the function, I think this is overhead. Just t= o avoid conditional sql template I would not prefer this approach.

We are not to= tally sure we understood what you meant on the previous statement. Are you = saying that the conditionals in SQL are used to ensure that we can apply a = javascript function at column level instead of cell level?=C2=A0
=
Correct.=C2=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">
Our concern is that the templates are being made more complex and inconsis= tencies are introduced in the code and the UI.
=C2=A0
Inconsistencies in the UI can be avoided th= rough making the size_formatter same as pg_size_pretty function which I wil= l implement.
I have checked the pg_size_pretty function code = and it supports till TB format, so I think we should keep till TB only.

In this particular example, we are allowing the backend to re= spond sometimes with prettified data and sometimes without it, so at UI lev= el we will have inconsistencies between screens or more complex Javascript = code to support sometimes prettifying and sometimes not prettify the same f= ields.=C2=A0

We h= ave separate logic for collection and single node in statistics.js and we a= re using javascript code for prettifying only for collection node.
=C2=A0
What we were thinking was to maybe not specify on the= SQL level and have the same format for "Size" everywhere in the = UI.=C2=A0
=C2=A0
Here = our main concern is inconsistency in "Size" format in the UI that= can be avoided as I said earlier.
We are using pg_size_pretty fu= nction for different fields like Size, Index Size, Table space size, Tuple = length, Size of Temporary files in different modules and some of them are c= ell level and we don't require to put overhead on cell level fields as = sorting is not required for individual node statistics.
=C2=A0
= =C2=A0
Thanks
Joao & Sarah

On Tu= e, Apr 25, 2017 at 11:48 PM, Khushboo Vashi <khushboo.vash= i@enterprisedb.com> wrote:
Hi Joao & Sarah,

Thanks for reviewing the patch.

<= div class=3D"gmail_quote">On Tue, Apr 25, 2017 at 8:34 PM, Joao Pedro= De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:
Hello Khushboo,

We reviewed the this patch and have = some suggestions:

Python:

=E2=80=8B
The functionality for adding the "can= _prettify" is repeated in multiple places. Maybe this could be extract= ed into a function.=C2=A0

When I have implemented this, my first thought is exactly same as you sug= gested but =C2=A0while looking at the code I felt its not a good idea to ha= ve a function. In case of a function, we need to pass the whole result-set = as well as the list of fields which we need to be prettified. So, only for = 2 lines, I didn't find any reason to make a function.

Jav= ascript:

=E2=80=8B
  • The class=C2=A0Backgrid.SizeForm= atter doesn't seem to have any tests.=C2=A0
    =

Sure, will do= .=C2=A0
  • The function pg_size_pretty displays byt= es and Kilobytes differently.=C2=A0
  • Is it possible to add PB as= well?
Will check and add= PB.=C2=A0
  • The function is a little bit hard to re= ad, is it possible to refactor using private functions like:
Will make it more readable.
fromRaw: function (rawData, model) {
   var unitIdx =3D findDataUnitIndex(rawData);
   if (unitIdx =3D=3D 0) {
      return rawData + ' ' + this.dataUnits[i];
   }
   return formatOutput(rawData, unitIdx);
},
=E2=80=8B=

  • In statistics.js:326 we believe it would= make the code more readable if we change the variable "c" to &qu= ot;rawColumn" and "col" to "column".
=

I will change the varia= ble name from =C2=A0"c" to =C2=A0"rawColumn" but I thin= k "col" is appropriate as we already have columns variable and an= yone can confuse while reading the code (for columns and column).
<= /div>

SQL Files:

=E2=80=8B
  • Is there a way to avoid condit= ionals here?=C2=A0
  • Maybe we can use the same javascript function to= prettify all the sizes

In case of collection node (ex: Databases), I have implemented th= is functionality via putting a formatter for a back-grid column. So, it is = applicable for the entire column not for the individual cell. To apply the = javascript function on a single cell for the column (string column), first = we need to identify that cell and then apply the function, I think this is = overhead. Just to avoid conditional sql template I would not prefer this ap= proach.

Visually we saw a difference= between "Databases" statistics and a specific database statistic= s. In "Databases" statistics the "Size" is "7.4 MB= " but when you are in the specific database the "Size" is &q= uot;7420 kB".
Is this the intended behavior?

Only for the Databases (collectio= n node), the client side functionality is implemented not for individual no= de , so this behaviour is different. For the individual node still, we are = using pg_size_pretty function
=C2=A0

Thanks
Joao & Sarah

On Tue, Apr 25, 2017 at 7:58 AM, Dave= Page <dpage@pgadmin.org> wrote:
Ashesh, can you review/commit th= is please?

Thanks.

On Tue, Apr 25, 2017 at 10:18= AM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-ha= ckers

=


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

E= nterpriseDB UK: h= ttp://www.enterprisedb.com
The Enterprise PostgreSQL Company


= Thanks,
Khushboo


Thanks,
Khushboo


Thanks,
Khushboo

--001a1143d74284374c054e3ae4f8--