review: Built-in binning functions

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

2014-06-22 13:02 GMT+02:00 Petr Jelinek <petr@2ndquadrant.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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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@2ndquadrant.com>: