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

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