Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d41N8-00065b-VJ for pgadmin-hackers@arkaria.postgresql.org; Fri, 28 Apr 2017 08:35:27 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1d41N8-0008Kz-9a for pgadmin-hackers@arkaria.postgresql.org; Fri, 28 Apr 2017 08:35:26 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1d41Ms-0007rw-2a for pgadmin-hackers@postgresql.org; Fri, 28 Apr 2017 08:35:10 +0000 Received: from mail-it0-x235.google.com ([2607:f8b0:4001:c0b::235]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1d41Mn-0005gn-2H for pgadmin-hackers@postgresql.org; Fri, 28 Apr 2017 08:35:09 +0000 Received: by mail-it0-x235.google.com with SMTP id x188so31426391itb.0 for ; Fri, 28 Apr 2017 01:35:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=zb4j6eMyK+ljZ4kwusdUvVJ6x2k8FuZe10YRh/ClMn8=; b=jpnsJDDWi1Ea9tmv4zRlLxiXzwPEKMYPy1FzftBxYIAyo+NJhG4N7jfze/wwMDi98y BazpZx7q2OnnRWujkxd0CEIdC20YLc4sS+4AZ8fsOihHPytOp5wEQzPFpy0U6+v40MxZ 750fMPPK6rVxso1bPn7TFsJzwOTQapaPLJhIziCSdREaRfW1ouyFoEBRDXvT+spN9fsk nRjP3eRDcX0wuMj7Xw0v4jMPA4BeJ25oZ14CHqidYwfRD45EyPT63ByeMwNtPVXYxl8q +b6mtGtjsboiHRE+SaqLwaCkPaYdUA82R44OhHcQq79bEsdcH+dpwb5A3OjEwAwgISo+ seEw== 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=zb4j6eMyK+ljZ4kwusdUvVJ6x2k8FuZe10YRh/ClMn8=; b=sEH83wAAmhStgcDrIjsk+2KH9sNNyGSlm+JCi+SNJgxU9A4p/Iiq7XbE1CfrilyRY6 HPStS62yH0/sxjrZi0xCFmP5Y+yYAZRIzSLiE8Ky4DYHJtBop0Uxf6oXKlJGAzUU8HDz qA9dccgjg2PzEWCEHvRCGJV4zJaRVV+VBqU5i21pgnIolazs83GsxRMtsL2vKgnzedq6 xF5TpgQkysgW1Ek9/cjiAZaT/KIOTqTxESDGZHHR4ODdJm8Y+BM3u1w6+ICRlq7ON45+ jEZrPYvcS2OEcS8WtU0nmlRP2xIRTHCamxTX2/61w3iYtMzG9GNNf/ysAXt45N7RJrI9 0gkw== X-Gm-Message-State: AN3rC/7OTBg3UTqYtnTjNUNsFcgKVNPmClAevvXGQBI+TDaSZGeziEDM GWIGi/G6yAYTrKFOv0/T6+L0YibU9xaL X-Received: by 10.202.181.67 with SMTP id e64mr3735101oif.89.1493368502762; Fri, 28 Apr 2017 01:35:02 -0700 (PDT) MIME-Version: 1.0 Received: by 10.74.153.229 with HTTP; Fri, 28 Apr 2017 01:35:02 -0700 (PDT) In-Reply-To: References: From: Khushboo Vashi Date: Fri, 28 Apr 2017 14:05:02 +0530 Message-ID: Subject: Re: [pgAdmin4][Patch]: Fixed RM #2315 : Sorting by size is broken To: Sarah McAlear Cc: Joao Pedro De Almeida Pereira , Dave Page , pgadmin-hackers , Ashesh Vashi Content-Type: multipart/alternative; boundary=001a113cde7a20729a054e35f30d 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 --001a113cde7a20729a054e35f30d Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 till 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 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. >>>> >>>> >>> 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 ensu= re >>> that we can apply a javascript function at column level instead of cell >>> 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 level= we >>> will have inconsistencies between screens or more complex Javascript co= de >>> 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 nod= e. >> >> >>> What we were thinking was to maybe not specify on the SQL level and hav= e >>> 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 t= o >> put overhead on cell level fields as sorting is not required for individ= ual >> 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 you >>>> suggested but while looking at the code I felt its not a good idea to= have >>>> a function. In case of a function, we need to pass the whole result-se= t as >>>> well as the list of fields which we need to be prettified. So, only fo= r 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" to= "column". >>>>> >>>>> >>>>> I will change the variable name from "c" to "rawColumn" but I think >>>> "col" is appropriate as we already have columns variable and anyone ca= n >>>> 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 i= s >>>> applicable for the entire column not for the individual cell. To apply= the >>>> javascript function on a single cell for the column (string column), f= irst >>>> we need to identify that cell and then apply the function, I think thi= s is >>>> overhead. Just to avoid conditional sql template I would not prefer th= is >>>> approach. >>>> >>>>> >>>>> Visually we saw a difference between "Databases" statistics and a >>>>> specific database statistics. In "Databases" statistics the "Size" is= "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 behavio= ur 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 th= e actual >>>>>>> value of size instead of formatted value. >>>>>>> >>>>>>> >>>>>>> Thanks, >>>>>>> Khushboo >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.o= r >>>>>>> g) >>>>>>> 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 --001a113cde7a20729a054e35f30d Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi Sarah,

On Thu, Apr 27, 2017 at 7:38 PM, Sarah McAlear <smcalear@pi= votal.io> wrote:
Hi Kushboo!

We understand your point, but we be= lieve that relying on 2 independent functions to deliver the same formattin= g 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.=C2=A0

It seems reasonable to me and I am = going to use a single javascript function which will support PB also (as pe= r Dave we should add support till PB) .
If the community believes we can live wit= h this risk, let's move forward.

Thanks!<= br>Sarah & Joao

On Thu, Apr 27,= 2017 at 1:50 AM, Khushboo Vashi <khushboo.vashi@enterprised= b.com> wrote:
Hi Joao & Sarah,

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

Thanks for your reply!
=C2=A0
SQL Files:
    <= li style=3D"margin-left:15px">Is there a way to avoid conditionals here?=C2= =A0
  • Maybe we can use the same j= avascript function to prettify all the sizes

In case of collec= tion node (ex: Databases), I have implemented this functionality via puttin= g a formatter for a back-grid column. So, it is applicable for the entire c= olumn not for the individual cell. To apply the javascript function on a si= ngle cell for the column (string column), first we need to identify that ce= ll and then apply the function, I think this is overhead. Just to avoid con= ditional sql template I would not prefer this approach.
<= /div>

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 ensure that we can apply a javascript = function at column level instead of cell level?=C2=A0

<= /span>
Correct.=C2=A0
Our concern is that the templat= es are being made more complex and inconsistencies are introduced in the co= de and the UI.
=C2=A0
= Inconsistencies in the UI can be avoided through making the size_formatter = same as pg_size_pretty function which I will implement.
I hav= e 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 allo= wing the backend to respond sometimes with prettified data and sometimes wi= thout it, so at UI level we will have inconsistencies between screens or mo= re complex Javascript code to support sometimes prettifying and sometimes n= ot prettify the same fields.=C2=A0

We have separate logic for collection and single node in = statistics.js and we are using javascript code for prettifying only for col= lection node.
=C2=A0
What we were thinking was to maybe not specify on th= e 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 tha= t can be avoided as I said earlier.
We are using pg_size_pretty f= unction 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 individual node statistics.
=C2=A0
=C2=A0
Thanks
Joao & Sarah
=

On Tue, Apr 25= , 2017 at 11:48 PM, Khushboo Vashi <khushboo.vashi@enterpris= edb.com> wrote:
Hi Joao & Sarah,

Thank= s for reviewing the patch.

On Tue, Apr 25, 2017 at 8:34 PM, Joao Pedro De Almei= da Pereira <jdealmeidapereira@pivotal.io> wrote:<= br>
Hello= Khushboo,

We reviewed the this patch and have some sugg= estions:

= 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.

Javascript:

=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>

S= QL 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 T= ue, Apr 25, 2017 at 7:58 AM, Dave Page <dpage@pgadmin.org> w= rote:
Ashesh, can you review/commit this please?

Thanks.

On Tue, Apr 25, 2= 017 at 10:18 AM, Khushboo Vashi <khushboo.vashi@enterprisedb= .com> wrote:
Hi,

Fixed RM #2315 : Sor= ting by size is broken.

Removed the pg_size_pretty= function from query for the collection and introduced the client side func= tion to convert size into human readable format. So, the sorting issue is f= ixed as the algorithm will get the actual value of size instead of formatte= d value.=C2=A0
=C2=A0

Thanks,
<= div>Khushboo




--
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<= /a>
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Ente= rprise PostgreSQL Company


= Thanks,
Khushboo


Thanks,
Khushboo


Thanks,=
Khushboo
--001a113cde7a20729a054e35f30d--