Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d3cJq-0004Vl-CU for pgadmin-hackers@arkaria.postgresql.org; Thu, 27 Apr 2017 05:50:22 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1d3cJp-00057z-V6 for pgadmin-hackers@arkaria.postgresql.org; Thu, 27 Apr 2017 05:50:22 +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 1d3cJa-0004f2-DG for pgadmin-hackers@postgresql.org; Thu, 27 Apr 2017 05:50:06 +0000 Received: from mail-oi0-x22e.google.com ([2607:f8b0:4003:c06::22e]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1d3cJW-0007A9-Mk for pgadmin-hackers@postgresql.org; Thu, 27 Apr 2017 05:50:05 +0000 Received: by mail-oi0-x22e.google.com with SMTP id x184so26215384oia.1 for ; Wed, 26 Apr 2017 22:50:02 -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=0LrMSjNs/vbXXAigsytB/Zjpcg6zCuYXMlXj+uDU6p4=; b=G0/zIQkDl09rAgOLTVb+Uyo+csbX5VqdCbA6BfKtCicNKPYyveFgyaETh8SbHxxEQx xQlFKf/uMMMkgS+oP5iQLTh5+yfDcSEAROX+3HCgKXxCXHrpTRpAZuman1gk0oP+UVon jAMnonJrkazTBxzwjCgGqSUGnwpJ+3shPsP80XHC3VIcVRLbTLHMt6y1z0TnJC8GH/4w ZB0ADHuf30jjJOwIQoOZ4HqfE/CfyX7VM+uXeyCEkfmwodREsbqJ+4urlVNY5Th3ofNZ afezd3VYbakV9on92ZdhZGcY1n4saH5vsCLNOvcMvRYQz1MKzGA7GYsunWD/hAiKgs+j jd7A== 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=0LrMSjNs/vbXXAigsytB/Zjpcg6zCuYXMlXj+uDU6p4=; b=ols08vUdValtnJtCbKa1/u+bVPBsm+eQ714eAIAjbtYM1qeZ4+n8CLac1HaPoZhwOu qBuw9dD3ZvQK2hE9Jha0ugn/EMIoLOOpfCMvTSinsf0fogZBQpdzrYa01yCpLWIja5Zj Mp0xFErc+baLp3MzWxn4GMHo8C1pgktwtUDAxqiR7h37AQgrEkw44WA9MCq5bKJQ78uM E9OuJjCPvP5UZiPPTpWa8BTKfQHx076v2IrmtefG5d2JI81Bju/5YViy9eFQ9MK2oxrU AuZHNsJrjZnNDT87EnzsH42KM7BLUg7NEmvW7w87c7imLIE6gdl38H8qDGto7xRhsYy0 SouQ== X-Gm-Message-State: AN3rC/4T/mPZFRHaC3oZ3mMid1x9TqItdK1Fqowb0OD3678fAfbCR3LM DLY/uw4dpnCxNGNfR8nTHk8M0bMXvmtb X-Received: by 10.202.181.67 with SMTP id e64mr1624998oif.89.1493272201554; Wed, 26 Apr 2017 22:50:01 -0700 (PDT) MIME-Version: 1.0 Received: by 10.74.153.229 with HTTP; Wed, 26 Apr 2017 22:50:01 -0700 (PDT) In-Reply-To: References: From: Khushboo Vashi Date: Thu, 27 Apr 2017 11:20:01 +0530 Message-ID: Subject: Re: [pgAdmin4][Patch]: Fixed RM #2315 : Sorting by size is broken To: Joao Pedro De Almeida Pereira Cc: Dave Page , pgadmin-hackers , Ashesh Vashi Content-Type: multipart/alternative; boundary=001a113cde7a2083ac054e1f8747 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 --001a113cde7a2083ac054e1f8747 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 apply = the >>> javascript function on a single cell for the column (string column), fi= rst >>> 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 thi= s >>> 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 ensure > 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 w= e > will have inconsistencies between screens or more complex Javascript code > to support sometimes prettifying and sometimes not prettify the same > fields. > > We have separate logic for collection and single node in statistics.js an= d we are using javascript code for prettifying only for collection node. > 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 individual 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 h= ave >> 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 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 can >> 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 apply t= he >> javascript function on a single cell for the column (string column), fir= st >> 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 >> 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 behaviour 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 readab= le >>>>> format. So, the sorting issue is fixed as the algorithm will get the = 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 --001a113cde7a2083ac054e1f8747 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi Joao & Sarah,

On Wed, Apr 26, 2017 a= t 8:46 PM, Joao Pedro De Almeida Pereira <jdealmeidapereira@piv= otal.io> wrote:
Hi Khushboo!

Thanks for your reply= !
=C2=A0
SQL = Files:
  • Is there a way to avoid c= onditionals here?=C2=A0
  • Maybe w= e can use the same javascript function to prettify all the sizes
<= br>In case of collection node (ex: Databases), I have implemented this func= tionality via putting a formatter for a back-grid column. So, it is applica= ble for the entire column not for the individual cell. To apply the javascr= ipt 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 overhea= d. Just to avoid conditional sql template I would not prefer this approach.=

We a= re 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

Correct.=C2=A0
Our concern is that the templates are being made more complex and = inconsistencies are introduced in the code and the UI.
=C2=A0
Inconsistencies in the UI can be avoided t= hrough making the size_formatter same as pg_size_pretty function which I wi= ll 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 U= I level we will have inconsistencies between screens or more complex Javasc= ript code to support sometimes prettifying and sometimes not prettify the s= ame fields.=C2=A0

We hav= e separate logic for collection and single node in statistics.js and we are= using javascript code for prettifying only for collection node.
= =C2=A0
What we were thinking was to maybe not specify on t= he SQL level and have the same format for "Size" everywhere in th= e UI.=C2=A0
=C2=A0
Here our m= ain concern is inconsistency in "Size" format in the UI that can = be avoided as I said earlier.
We are using pg_size_pretty functio= n for different fields like Size, Index Size, Table space size, Tuple lengt= h, Size of Temporary files in different modules and some of them are cell l= evel and we don't require to put overhead on cell level fields as sorti= ng is not required for individual node statistics.
=C2=A0
=C2=A0
Thanks
Joao & Sarah
<= br>
On Tue, Apr 25, 2017 at 11:48 PM, Khushboo Va= shi <khushboo.vashi@enterprisedb.com> wro= te:
H= i Joao & Sarah,

Thanks for reviewing the patch.

On Tue, Ap= r 25, 2017 at 8:34 PM, Joao Pedro De Almeida Pereira <= jdealmeid= apereira@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.

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

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, 2= 017 at 7:58 AM, Dave Page <dpage@pgadmin.org> 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 t= he 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 the actual value of size instead of formatted value.=C2=A0
= =C2=A0

Thanks,
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
Twitte= r: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgr= eSQL Company


= Thanks,
Khushboo


Thanks,=
Khushboo
--001a113cde7a2083ac054e1f8747--