Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d3k6V-0000kS-Sp for pgadmin-hackers@arkaria.postgresql.org; Thu, 27 Apr 2017 14:09:08 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1d3k6V-0006hT-FB for pgadmin-hackers@arkaria.postgresql.org; Thu, 27 Apr 2017 14:09:07 +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 1d3k6U-0006ey-Ee for pgadmin-hackers@postgresql.org; Thu, 27 Apr 2017 14:09:06 +0000 Received: from mail-io0-x22e.google.com ([2607:f8b0:4001:c06::22e]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1d3k6O-0007wg-GN for pgadmin-hackers@postgresql.org; Thu, 27 Apr 2017 14:09:06 +0000 Received: by mail-io0-x22e.google.com with SMTP id p80so23300431iop.3 for ; Thu, 27 Apr 2017 07:08:59 -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=cvxWo4+vQwqfHg4YVyYkPMp5UVWnKfH5Ap3I9Cbb/yU=; b=WOOfzy1mbCCK5BkV8VoGNMy/wYITl2nEY/lwy3Bz7NVCjNAibVEwdhgYWfHcDcJgg1 aMhKKcgXlD0+Ap+UFm0t2CyJcJ6nonWSGANPSpURk2XBL+yD8/tmk4ttdXDzjzpLtl/p fpGc+KAN/QcWsnlqL5X6z/V0N5017Wa6a1toOQdzSptfwROZ+T4s5x42WBYmWGMINgTD c3hphsQOnKVPioKjB8Z5YcevlrKkGHN01akYof9hTVXdAO6mqmvBBlDaIWY8MgoXt57y SO8v3GQn1eRWedV+DMIW+u/s0pMVqvcA3nnfZjCx/TrIT1vvH2kfnSbB4AhdvK5oMg8O 19/g== 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=cvxWo4+vQwqfHg4YVyYkPMp5UVWnKfH5Ap3I9Cbb/yU=; b=qbpVxUy5zRYEgSCUZ2C6k4idAC8SMtMYoiAiNlyQvDWlNNPUkt9m297l/Js1d0PdJD 1hvmO4X6Iyq/Wo/Mt0RyuUb12BVZj6ZOgZwXrX2duokZgHuhohk2B4w541esOBH2uz8i KSat2Z5zlnZ/Dll+jfQucqvmS16pazTAlscUGAUlzMvYOKIlXuNLSaJCUhHOTeE6sd0O vWMYike686g3e/Dobe949cqQKScoFQ6bKEUmgvUuxAYrRjhGUPXnJ6VekvZh/k+kwj4u OQ+AwF7+CIkPnkhUJF3M3krHiLBP69Z0kEbovH1AwDohTn/B9l4URzGkHbe/yUpCYHVz 8oRw== X-Gm-Message-State: AN3rC/6056fQHyTgY4xFtZW8Sm6SUhjyZN+hKdKIRmVaU5ZcVBly2129 W3Y/Zp1m/GiRbeZrK9EuVK9HdRLXHlKW X-Received: by 10.107.185.214 with SMTP id j205mr5521903iof.3.1493302137699; Thu, 27 Apr 2017 07:08:57 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.169.216 with HTTP; Thu, 27 Apr 2017 07:08:37 -0700 (PDT) In-Reply-To: References: From: Sarah McAlear Date: Thu, 27 Apr 2017 10:08:37 -0400 Message-ID: Subject: Re: [pgAdmin4][Patch]: Fixed RM #2315 : Sorting by size is broken To: Khushboo Vashi Cc: Joao Pedro De Almeida Pereira , Dave Page , pgadmin-hackers , Ashesh Vashi Content-Type: multipart/alternative; boundary=94eb2c0769c475e575054e267ffe 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 --94eb2c0769c475e575054e267ffe Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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. 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 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. >>> >>> >> 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 ensur= e >> 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_formatte= r > 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 cod= e >> 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 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 ca= n > be avoided as I said earlier. > We are using pg_size_pretty function for different fields like Size, Inde= x > Size, Table space size, Tuple length, Size of Temporary files in differen= t > 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 multipl= e >>>> 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-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 = 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. >>> >>>> >>>> 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 functionalit= y >>> 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 an= d >>>>>> introduced the client side function to convert size into human reada= ble >>>>>> 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.or= 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 > --94eb2c0769c475e575054e267ffe Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi Kushboo!

We understand your point, b= ut we believe that relying on 2 independent functions to deliver the same f= ormatting can become a problem if the PG function changes. Our suggestion i= s to use a single function in our javascript code to do this formatting.=C2= =A0

If the community believes we can live with thi= s risk, let's move forward.

Thanks!
Sa= rah & Joao

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

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

Thanks for your reply!
=C2=A0
SQL Files:
  • Is there a way to avoid conditionals here?=C2=A0
  • Maybe we can use the same javascript function to pre= ttify 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 individ= ual 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 fun= ction, I think this is overhead. Just to avoid conditional sql template I w= ould not prefer this approach.
<= div>
We are not totally sure we understood what you me= ant 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 i= nconsistencies are introduced in the code and the UI.
=C2=A0
Inconsistencies in the UI can be avo= ided through making the size_formatter same as pg_size_pretty function whic= h I will implement.
I have checked the pg_size_pretty functio= n code and it supports till TB format, so I think we should keep till TB on= ly.

In this par= ticular example, we are allowing the backend to respond sometimes with pret= tified data and sometimes without it, so at UI level we will have inconsist= encies between screens or more complex Javascript code to support sometimes= prettifying and sometimes not prettify the same fields.=C2=A0
We have separate logic for co= llection 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 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" for= mat 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 cel= l level fields as sorting is not required for individual node statistics.
=C2=A0
=C2=A0
Thanks
Joao & Sarah

On Tue, Ap= r 25, 2017 at 11:48 PM, Khushboo Vashi <khushboo.vashi@enter= prisedb.com> wrote:
Hi Joao & Sarah,

T= hanks for reviewing the patch.

On Tue, Apr 25, 2017 at 8:34 PM, Joao Pedro De Alm= eida Pereira <jdealmeidapereira@pivotal.io> wrote= :
Hel= lo Khushboo,

We reviewed the this patch and have some su= ggestions:

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>

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 <dpa= ge@pgadmin.org> wrote:
Ashesh, can you review/commit this please?<= div>
Thanks.

On Tue, = Apr 25, 2017 at 10:18 AM, Khushboo Vashi <khushboo.vashi@ent= erprisedb.com> wrote:
Hi,

Fixed RM #2315 : Sorting by siz= e is broken.

Removed the pg_size_pretty function f= rom query for the collection and introduced the client side function to con= vert 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,
Khushbo= o




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

EnterpriseDB= UK: http://www.e= nterprisedb.com
The Enterprise PostgreSQL Company


= Thanks,
Khushboo


Thanks,
Khushboo

--94eb2c0769c475e575054e267ffe--