public inbox for [email protected]
help / color / mirror / Atom feedFrom: Matthew Kleiman <[email protected]>
To: Khushboo Vashi <[email protected]>
Cc: Sarah McAlear <[email protected]>
Cc: Dave Page <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Cc: Ashesh Vashi <[email protected]>
Subject: Re: [pgAdmin4][Patch]: Fixed RM #2315 : Sorting by size is broken
Date: Fri, 28 Apr 2017 10:28:31 -0400
Message-ID: <CAFS4TJZy69PqvdPOebPzhsL=B+knAg062AF=RDuTbV3wzEy3xw@mail.gmail.com> (raw)
In-Reply-To: <CAFOhELeXJ=KN0Rm21RP-vvGGXBDVaVqyBXzGz_oiqsA085JekQ@mail.gmail.com>
References: <CAFOhELcc_ctZkKJYH1c+7pd3zGM3uy=FQh+nEya+yczZJ4d+BA@mail.gmail.com>
<CA+OCxozrW_YNjrHRUC4qrzBkxLocVqmoBXnMH=NKxCrGn+0H2g@mail.gmail.com>
<CAE+jja=Q0bQXLz4Dd4i=Gnx_P=y7nL+bS+j1JDpxhoAmh=ZV8A@mail.gmail.com>
<CAFOhELfypnuq+f+SwXhr_4rk29U3oL7rVGOtZHDY_vxXG-0UfQ@mail.gmail.com>
<CAE+jjamuoGSNd3T90Pfi5PsDzyvoFWHFopDjRH=PxhjTZ5fDLg@mail.gmail.com>
<CAFOhELfoGC+D15ujbJEk884sJ9jezSkUUOLeUVT6T=u0Of+M-w@mail.gmail.com>
<CAGRPzo_UrpwbkOxiApzPK84edbd5L7A=O0QZabvZz3v421GDpQ@mail.gmail.com>
<CAFOhELeXJ=KN0Rm21RP-vvGGXBDVaVqyBXzGz_oiqsA085JekQ@mail.gmail.com>
List-Unsubscribe: <mailto:[email protected]?body=unsub%20pgadmin-hackers>
Hi Khushboo,
That sounds good. Sorry if we weren't clear at first.
Have a good holiday weekend!
Sarah & Matt
On Fri, Apr 28, 2017 at 4:35 AM, Khushboo Vashi <
[email protected]> wrote:
> Hi Sarah,
>
> On Thu, Apr 27, 2017 at 7:38 PM, Sarah McAlear <[email protected]>
> 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 <
>> [email protected]> wrote:
>>
>>> Hi Joao & Sarah,
>>>
>>> On Wed, Apr 26, 2017 at 8:46 PM, Joao Pedro De Almeida Pereira <
>>> [email protected]> 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), 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.
>>>>>
>>>>>
>>>> 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 we
>>>> 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
>>> 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
>>> 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 <
>>>> [email protected]> wrote:
>>>>
>>>>> Hi Joao & Sarah,
>>>>>
>>>>> Thanks for reviewing the patch.
>>>>>
>>>>> On Tue, Apr 25, 2017 at 8:34 PM, Joao Pedro De Almeida Pereira <
>>>>> [email protected]> wrote:
>>>>>
>>>>>> Hello Khushboo,
>>>>>>
>>>>>> We reviewed the this patch and have some suggestions:
>>>>>>
>>>>>> *Python:*
>>>>>>
>>>>>> 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:*
>>>>>>
>>>>>>
>>>>>> - 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 = findDataUnitIndex(rawData);
>>>>>> if (unitIdx == 0) {
>>>>>> return rawData + ' ' + this.dataUnits[i];
>>>>>> }
>>>>>> return formatOutput(rawData, unitIdx);
>>>>>> },
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> - 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:*
>>>>>>
>>>>>>
>>>>>> - 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
>>>>>> 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 <[email protected]> wrote:
>>>>>>
>>>>>>> Ashesh, can you review/commit this please?
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>> On Tue, Apr 25, 2017 at 10:18 AM, Khushboo Vashi <
>>>>>>> [email protected]> 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 actual
>>>>>>>> value of size instead of formatted value.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Khushboo
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Sent via pgadmin-hackers mailing list (
>>>>>>>> [email protected])
>>>>>>>> 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
>
view thread (15+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [pgAdmin4][Patch]: Fixed RM #2315 : Sorting by size is broken
In-Reply-To: <CAFS4TJZy69PqvdPOebPzhsL=B+knAg062AF=RDuTbV3wzEy3xw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox