Re: review: Built-in binning functions

review: https://commitfest.postgresql.org/action/patch_view?id=1484

Hello

I did review of this patch, that add three functions varwidth_bucket for
types: anyelement, double and bigint

* This patch respects PostgreSQL coding rules
* it can applied without any issues
* there are no new compile warnings
* patch contains documentation and tests
* all tests was passed

* there was no any objection in related discussion. Functionality is clean
and based on current functionality of width_bucket.

My comments:

* I miss in documentation description of implementation - its is based on
binary searching, and when second parameter is unsorted array, then it
returns some nonsense without any warning.

* Description for anyelement is buggy twice times

"varwidth_bucket(5.35::numeric, ARRAY[1, 3, 4, 6]::numeric)"

probably should be "varwidth_bucket(5.35::numeric, ARRAY[1, 3, 4,
6]::numeric[])"

BUT it is converted to double precision, function with polymorphic
parameters is not used. So it not respects a widh_buckets model:

postgres=# \dfS width_bucket
List of functions
Schema │ Name │ Result data type │
Argument data types │ Type
────────────┼──────────────┼──────────────────┼───────────────────────────────────────────────────────────────┼────────
pg_catalog │ width_bucket │ integer │ double precision, double
precision, double precision, integer │ normal
pg_catalog │ width_bucket │ integer │ numeric, numeric, numeric,
integer │ normal
(2 rows)

There should be a interface for numeric type too. I am sure so important
part of code for polymorphic type can be shared.

Regards

Pavel

Hi,

On 21/06/14 20:41, Pavel Stehule wrote:
> review: https://commitfest.postgresql.org/action/patch_view?id=1484
>

Thanks for review.

>
> My comments:
>
> * I miss in documentation description of implementation - its is based
> on binary searching, and when second parameter is unsorted array, then
> it returns some nonsense without any warning.
>

Right I did mean to mention that thresholds array must be sorted, but
forgot about it when submitting.

> * Description for anyelement is buggy twice times
>
> "varwidth_bucket(5.35::numeric, ARRAY[1, 3, 4, 6]::numeric)"
>
> probably should be "varwidth_bucket(5.35::numeric, ARRAY[1, 3, 4,
> 6]::numeric[])"
>
> BUT it is converted to double precision, function with polymorphic
> parameters is not used. So it not respects a widh_buckets model:
>
> postgres=# \dfS width_bucket
> List of functions
> Schema │ Name │ Result data type │
> Argument data types │ Type
> ────────────┼──────────────┼──────────────────┼───────────────────────────────────────────────────────────────┼────────
> pg_catalog │ width_bucket │ integer │ double precision,
> double precision, double precision, integer │ normal
> pg_catalog │ width_bucket │ integer │ numeric, numeric,
> numeric, integer │ normal
> (2 rows)
>
> There should be a interface for numeric type too. I am sure so important
> part of code for polymorphic type can be shared.
>

I wonder if it would be acceptable to just create pg_proc entry and
point it to generic implementation (that's what I originally had, then I
changed pg_proc entry to polymorphic types...)

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

2014-06-22 13:02 GMT+02:00 Petr Jelinek <petr(at)2ndquadrant(dot)com>:

> Hi,
>
> On 21/06/14 20:41, Pavel Stehule wrote:
>
>> review: https://commitfest.postgresql.org/action/patch_view?id=1484
>>
>>
> Thanks for review.
>
>
>
>> My comments:
>>
>> * I miss in documentation description of implementation - its is based
>> on binary searching, and when second parameter is unsorted array, then
>> it returns some nonsense without any warning.
>>
>>
> Right I did mean to mention that thresholds array must be sorted, but
> forgot about it when submitting.
>
>
> * Description for anyelement is buggy twice times
>>
>> "varwidth_bucket(5.35::numeric, ARRAY[1, 3, 4, 6]::numeric)"
>>
>> probably should be "varwidth_bucket(5.35::numeric, ARRAY[1, 3, 4,
>> 6]::numeric[])"
>>
>> BUT it is converted to double precision, function with polymorphic
>> parameters is not used. So it not respects a widh_buckets model:
>>
>> postgres=# \dfS width_bucket
>> List of functions
>> Schema │ Name │ Result data type │
>> Argument data types │ Type
>> ────────────┼──────────────┼──────────────────┼─────────────
>> ──────────────────────────────────────────────────┼────────
>> pg_catalog │ width_bucket │ integer │ double precision,
>> double precision, double precision, integer │ normal
>> pg_catalog │ width_bucket │ integer │ numeric, numeric,
>> numeric, integer │ normal
>> (2 rows)
>>
>> There should be a interface for numeric type too. I am sure so important
>> part of code for polymorphic type can be shared.
>>
>>
> I wonder if it would be acceptable to just create pg_proc entry and point
> it to generic implementation (that's what I originally had, then I changed
> pg_proc entry to polymorphic types...)
>

probably not. But very simple wrapper is acceptable.

Pavel

>
> --
> Petr Jelinek http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>

Hi,

On 2014-06-21 20:41:43 +0200, Pavel Stehule wrote:
> review: https://commitfest.postgresql.org/action/patch_view?id=1484

Can you please not start new threads for reviews of smaller features?
Doing so makes following the discussion much harder. I'm fine with
changing the subject if the reply headers are left intact...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Here is v2,

with fixed documentation and numeric version of the implementation.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hello

I recheck this patch

1. applied cleanly and compilation was without warnings and errors
2. all tests was passed ok
3. documentation was rebuild without issues
4. I don't see any issue in code quality - it is well commented, well
formatted, with regress tests

It is ready for commit

Regards

Pavel

2014-06-26 22:43 GMT+02:00 Petr Jelinek <petr(at)2ndquadrant(dot)com>:

> Here is v2,
>
> with fixed documentation and numeric version of the implementation.
>
>
> --
> Petr Jelinek http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>