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

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.

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
>