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

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)or
>>>>>>> g)
>>>>>>> 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