Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1d3Dwi-0000Xf-Kq for pgadmin-hackers@arkaria.postgresql.org; Wed, 26 Apr 2017 03:48:52 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1d3Dwh-00077k-KO for pgadmin-hackers@arkaria.postgresql.org; Wed, 26 Apr 2017 03:48:51 +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 1d3Dwh-00077b-1z for pgadmin-hackers@postgresql.org; Wed, 26 Apr 2017 03:48:51 +0000 Received: from mail-oi0-x22d.google.com ([2607:f8b0:4003:c06::22d]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1d3Dwc-0006ds-6k for pgadmin-hackers@postgresql.org; Wed, 26 Apr 2017 03:48:50 +0000 Received: by mail-oi0-x22d.google.com with SMTP id y11so162923942oie.0 for ; Tue, 25 Apr 2017 20:48:45 -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=2fXwM1SVApFsPnJRqB/zINjUPanyFwVjmhy7bZflY54=; b=WEjpDwjKPRmizr4joTnYp/ZM9NdrG5+PTo8Hu6gC4HsxzDRQd4LWV7Y/PR7WobZDkD DW2s5vXO8yIHHUzryBV5eEyo9sd1SYAbII0a9GbavA3AIa0aint6kHaJrnKjuLgOO1rB HFcGTqKHQrpMHuc/PtqAu1SQgajxfnXN1eYW6q9JfYQyqMQxzdFJjXQ2JWMKjTJDA1uB fi/Wt5ieJ56feLkOKhBpKuLysndpagxTBwP02NVdXuZQoS1XK9k9na2aRM1uF2eBfvcq Cl3Gb85zMowNVb3+n1QCJJijcP8mGc2HUREzQlfTUXzYPDCNNcGz9mTghL5cMTEd1fjm HS9Q== 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=2fXwM1SVApFsPnJRqB/zINjUPanyFwVjmhy7bZflY54=; b=YyCYnTZpXjQdVA7YG7YWB7R7a6f4z+Zwr/T8LNdFu+9V6U51Vca4Hmc8dh2Kzjuv7s g4MFHSBBSLdu9roD1SkIVAPVoOZp3gnZ5ZKYO9QfUxkfloXN6mDEXdHdodFo0v4ZV1tq nju6ulB8cUVoIj10nmTjqIV+HTgCf7YqdOuFNT+4u2RO33euyiFnuVGhazVNAJnbwUM9 PdbrHoRflWfVycUwKzrzpbhFv8Ko/9v9GiT9BoRW+zzE1DtoZpJmaipJ1Q+JUy+tBp7R 4FsCR1kqMdTPeiYJOAW5ADHW1UmNCiSlhn8/BWPOsDEcLixwv22k34d2a6dvaQOv0N4f o0zA== X-Gm-Message-State: AN3rC/7tT2+Ks4eC+gWeqRSAQtIRFVsH45XC/3WZ/cSYxB9fh/eRx+Fp jfQqFD4+Of5i8zi67i0AmsROy8j+gIBh X-Received: by 10.157.31.74 with SMTP id x10mr21772983otx.3.1493178523780; Tue, 25 Apr 2017 20:48:43 -0700 (PDT) MIME-Version: 1.0 Received: by 10.74.153.229 with HTTP; Tue, 25 Apr 2017 20:48:43 -0700 (PDT) In-Reply-To: References: From: Khushboo Vashi Date: Wed, 26 Apr 2017 09:18:43 +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=001a1141bc687f1b72054e09b7bf 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 --001a1141bc687f1b72054e09b7bf Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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-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), 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 approach. > > Visually we saw a difference between "Databases" statistics and a specifi= c > 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 i= s 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 readable >>> format. So, the sorting issue is fixed as the algorithm will get the ac= tual >>> 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 --001a1141bc687f1b72054e09b7bf Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi Joao & Sarah,

Thanks for reviewi= ng the patch.

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

<= /div>
We reviewed the this patch and have some suggestions:
<= br>

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 suggested = but =C2=A0while looking at the code I felt its not a good idea to have a fu= nction. 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 bytes and Kiloby= tes 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 read, is it possible to refa= ctor using private functions like:
W= ill 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 variable nam= e from =C2=A0"c" to =C2=A0"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 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 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.=

Visually we saw a difference between &quo= t;Databases" statistics and a specific database statistics. In "D= atabases" statistics the "Size" is "7.4 MB" but wh= en you are in the specific database the "Size" is "7420 kB&q= uot;.
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
=C2=A0

Thanks
Joao & Sarah

On Tue, Apr 25, 2017 at 7:58 AM, Dave Page &= lt;dpage@pgadmin.org= > wrote:
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 : Sorting by size is b= roken.

Removed the pg_size_pretty function from qu= ery for the collection and introduced the client side function to convert s= ize into human readable format. So, the sorting issue is fixed as the algor= ithm 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
Twitter: @pgsnake

En= terpriseDB UK: ht= tp://www.enterprisedb.com
The Enterprise PostgreSQL Company


Thanks,=
Khushboo
--001a1141bc687f1b72054e09b7bf--