From: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> |
---|---|
To: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Cc: | Petr Jelinek <petr(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com> |
Subject: | review: Built-in binning functions |
Date: | 2014-06-21 18:41:43 |
Message-ID: | CAFj8pRDSonorPd-dYOe3C8sroeOKgXDo2A1E0QLM992et06Rfg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Stehule | 2014-06-21 20:11:17 | SQL access to database attributes |
Previous Message | Kevin Grittner | 2014-06-21 18:23:44 | Re: idle_in_transaction_timeout |