Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d3Oh1-0004kj-PF for pgadmin-hackers@arkaria.postgresql.org; Wed, 26 Apr 2017 15:17:24 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1d3Oh0-0003ZX-MV for pgadmin-hackers@arkaria.postgresql.org; Wed, 26 Apr 2017 15:17:22 +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 1d3Ogl-0002jl-Bf for pgadmin-hackers@postgresql.org; Wed, 26 Apr 2017 15:17:07 +0000 Received: from mail-ua0-x230.google.com ([2607:f8b0:400c:c08::230]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1d3Ogf-0003T6-9f for pgadmin-hackers@postgresql.org; Wed, 26 Apr 2017 15:17:06 +0000 Received: by mail-ua0-x230.google.com with SMTP id f10so2413892uaa.2 for ; Wed, 26 Apr 2017 08:17:00 -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=o2aT/SBAhiZztKSs6Iq+KTqsW2qHcexYS5o1EE472Y8=; b=QFkm55DOo0JGZolKX75Uxo24ln79Zc8lzbjgO4MP+0KZTVd6QEoRhVsF8upd7X7qCe y/LwT039yX/tnOwborIXMf23JXusbztJuz+Zeb115pShHT0Uw+W0PPcnXJzNERPsWGqf tOMz3jcVLla8lofOP7CPQs7Gn/pM2nVlCx4Sg/HCmteI8keW4/siM1rlL1AfvRhVlHEW xB6egrTInUYNuQ9qTRsYcFyDWOzDNYADk0uh9EdBWWDtBSX7eSbyOyF4XlUPj27edtwp 8CqIQ1xgzGrmA9ieR6zxvFzclSQqbxjM6Vn/acbAIvUhCRf17fJBe6gtJFvLN14xWbAQ lnvg== 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=o2aT/SBAhiZztKSs6Iq+KTqsW2qHcexYS5o1EE472Y8=; b=tPTZCdhxfqXjRM5NAmfZ7drm8b0FhxAfXRN4FL/aTb6m65ZqbDQTskjzYkOXnSqNzY whSTfn+FfWC/Xui5vFrIh+fzLs9IYxZ7c9Y6ehUVRQUfvWGjGWEdEGC0TzzcpR4DF5gN /kamgruMbzsioxcmQLV2fXdHwGEzVQTpKdTFgRROSSlUpWxtdOk9/NXGkoQ6qBxzwJEx Ew6Ngcg4nnQiS9rW3ifxL7GvEjVvEuplKVhHxr2mKtYEzt3bIiP1agl85v2SGCIOtwej 4/Gh/Wp3B8e6iEudWdMtYh50DlROmxUHvKAbPIBMQahCBoT725cdWiC5Asw8hhmugSK/ QCaA== X-Gm-Message-State: AN3rC/5DlnkuZTiA9sw8KAsftVe78mz1UR9Op0toPFN4V5sLqGKcKniI mId2ceqYiQibzNQrW1UpImrXPSyiXYbo X-Received: by 10.176.4.45 with SMTP id 42mr155669uav.105.1493219818647; Wed, 26 Apr 2017 08:16:58 -0700 (PDT) MIME-Version: 1.0 Received: by 10.103.10.199 with HTTP; Wed, 26 Apr 2017 08:16:58 -0700 (PDT) In-Reply-To: References: From: Joao Pedro De Almeida Pereira Date: Wed, 26 Apr 2017 11:16:58 -0400 Message-ID: Subject: Re: [pgAdmin4][Patch]: Fixed RM #2315 : Sorting by size is broken To: Khushboo Vashi Cc: Dave Page , pgadmin-hackers , Ashesh Vashi Content-Type: multipart/alternative; boundary=001a11480dfadcc38a054e135489 X-Pg-Spam-Score: -1.9 (-) 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 --001a11480dfadcc38a054e135489 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 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. > > 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? Our concern is that the templates are being made more complex and inconsistencies are introduced in the code and the UI. 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 code to support sometimes prettifying and sometimes not prettify the same fields. What we were thinking was to maybe not specify on the SQL level and have the same format for "Size" everywhere in the UI. 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 ha= ve > a function. In case of a function, we need to pass the whole result-set a= s > 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 "c= olumn". >> >> >> 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 th= e > javascript function on a single cell for the column (string column), firs= t > we need to identify that cell and then apply the function, I think this i= s > 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 readabl= e >>>> format. So, the sorting issue is fixed as the algorithm will get the a= ctual >>>> 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 > --001a11480dfadcc38a054e135489 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
H= i Khushboo!

Thanks for your reply!
=C2=A0
SQL Files:
  • Is there a way to avoid conditional= s 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 this functionality = via putting a formatter for a back-grid column. So, it is applicable for th= e entire column not for the individual cell. To apply the javascript functi= on on a single cell for the column (string column), first we need to identi= fy that cell and then apply the function, I think this is overhead. Just to= avoid conditional sql template I would not prefer this approach.

We are not totally su= re we understood what you meant on the previous statement. Are you saying t= hat the conditionals in SQL are used to ensure that we can apply a javascri= pt function at column level instead of cell level?=C2=A0

Our concern is that the templates are being made more complex and in= consistencies are introduced in the code and the UI. In this particular exa= mple, we are allowing the backend to respond sometimes with prettified data= and sometimes without it, so at UI level we will have inconsistencies betw= een screens or more complex Javascript code to support sometimes prettifyin= g and sometimes not prettify the same fields.=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
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.
<= div class=3D"gmail_extra">
O= n 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:

<= /div>

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.

Javascri= pt:

=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 d= isplays bytes and Kilobytes differently.=C2=A0
  • Is it possible t= o add PB as well?
Will ch= eck and add PB.=C2=A0
  • The function is a= little bit hard to read, is it possible to refactor using private function= s like:
Will make it more rea= dable.
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).

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 databas= e statistics. In "Databases" statistics the "Size" is &= quot;7.4 MB" but when you are in the specific database the "Size&= quot; is "7420 kB".
Is this the intended behavior?

Only for the Databases= (collection node), the client side functionality is implemented not for in= dividual node , so this behaviour is different. For the individual node sti= ll, we are using pg_size_pretty function
=C2= =A0

Thanks
Joa= o & Sarah
<= div class=3D"m_7923040603575588723gmail-h5">

=
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@enterprisedb.com> wrote:
Hi,<= div>
Fixed RM #2315 : Sorting by size is broken.
Removed the pg_size_pretty function from query for the collect= ion and introduced the client side function to convert size into human read= able format. So, the sorting issue is fixed as the algorithm will get the a= ctual value of size instead of formatted value.=C2=A0
=C2=A0
<= /div>

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.blogs= pot.com
Twitter: @pgsnake

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


= Thanks,
Khushboo

--001a11480dfadcc38a054e135489--