Обсуждение: review: Built-in binning functions
I did review of this patch, that add three functions varwidth_bucket for types: anyelement, double and bigint
* This patch respects PostgreSQL coding rules* patch contains documentation and tests
* all tests was passed
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)"
"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)
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@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.Right I did mean to mention that thresholds array must be sorted, but forgot about it when submitting.
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.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...)* 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.
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 patchPavel
2014-06-26 22:43 GMT+02:00 Petr Jelinek <petr@2ndquadrant.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