Re: [pgAdmin4][Patch]: Fixed RM #2315 : Sorting by size is broken

Hi Joao & George,

On Fri, May 5, 2017 at 9:00 PM, Joao Pedro De Almeida Pereira <
jdealmeidapereira(at)pivotal(dot)io> wrote:

> Hi Khushboo,
>
> We looked at your updated patch:
> - the tests look good!
> - there's a small comment header change needed in size_prettify_spec
>
ok. will change.

> - it looks like the previous and new functions have different behaviors
> (where the new behavior changes units on 10000 of the lower unit, a 9999GB)
>
It is the same behaviour as pg_size_pretty function of PostgreSQL

> - We should clarify that in size_prettify, we were mostly talking about
> name readability in your first patch, and that the original structure was
> better (especially the sizes array)
> At first glance, the new sizePrettify appears to behave like a for loop,
> so that might be the simplest refactor.
>
> I have followed the code structure of the pg_size_pretty function of
PostgreSQL :) .

Thanks,
> Joao and George
>
>

>
> On Thu, May 4, 2017 at 5:02 AM, Khushboo Vashi <
> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>> Hi,
>>
>> Please find the attached updated patch.
>>
>> Thanks,
>> Khushboo
>>
>> On Fri, Apr 28, 2017 at 7:58 PM, Matthew Kleiman <mkleiman(at)pivotal(dot)io>
>> wrote:
>>
>>> 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 <
>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Sarah,
>>>>
>>>> On Thu, Apr 27, 2017 at 7:38 PM, Sarah McAlear <smcalear(at)pivotal(dot)io>
>>>> 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 <
>>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> Hi Joao & Sarah,
>>>>>>
>>>>>> On Wed, Apr 26, 2017 at 8:46 PM, Joao Pedro De Almeida Pereira <
>>>>>> jdealmeidapereira(at)pivotal(dot)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),
>>>>>>>>> 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 <
>>>>>>> khushboo(dot)vashi(at)enterprisedb(dot)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(at)pivotal(dot)io> 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 <dpage(at)pgadmin(dot)org>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Ashesh, can you review/commit this please?
>>>>>>>>>>
>>>>>>>>>> Thanks.
>>>>>>>>>>
>>>>>>>>>> On Tue, Apr 25, 2017 at 10:18 AM, Khushboo Vashi <
>>>>>>>>>> khushboo(dot)vashi(at)enterprisedb(dot)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 actual value of size instead of formatted value.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Khushboo
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> Sent via pgadmin-hackers mailing list (
>>>>>>>>>>> pgadmin-hackers(at)postgresql(dot)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
>>>>>>
>>>>>
>>>>>
>>>> Thanks,
>>>> Khushboo
>>>>
>>>
>>>
>>
>>
>> --
>> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgadmin-hackers
>>
>>
>