Re: review: Built-in binning functions

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
>