Patch looks good, but there ara some review notices:
1 gmake installcheck fails:
*** /.../pgsql/contrib/cube/expected/cube_1.out 2015-12-01 17:49:01.768764000
+0300
--- /.../pgsql/contrib/cube/results/cube.out 2015-12-01 17:49:12.190818000
+0300
***************
*** 1382,1388 ****
(1 row)
-- Test of distances
! --
SELECT cube_distance('(1,1)'::cube, '(4,5)'::cube);
cube_distance
---------------
--- 1382,1388 ----
(1 row)
-- Test of distances
! --
SELECT cube_distance('(1,1)'::cube, '(4,5)'::cube);
cube_distance
Seems, there a extra space at the end of string
2 Pls, don use in C-code magick constants like 'case 16:'. Use macros to define
some human-readable name (g_cube_distance())
3 Switch in g_cube_distance(): default switch path should generate a error. It
just simplifies a degbug process, may be in future.
4 Docs: pls, don't use a strings with unlimited length.
Stas Kelvich wrote:
Hello.
That is updated version of the patch with proper update scripts.
Also i’ve noted that documentation states the wrong thing:
“It does not matter which order the opposite corners of a cube are entered in. The cube
functions automatically swap values if needed to create a uniform "lower left — upper
right" internal representation."
But in practice cubes stored "as is" and that leads to problems with getting
cubes sorted along specific dimension directly from index.
As a simplest workaround i’ve deleted that sentence from docs and implemented two
coordinate getters -> and ~>. First one returns
coordinate of cube as it stored, and second returns coordinate of cube
normalised to (LL,UR)-form.
Other way to fix thing is to force ’normalization’ while creating cube. But
that can produce wrong sorts with already existing data.
On 09 Jul 2015, at 16:40, Alexander Korotkov <aekorot...@gmail.com> wrote:
Hi!
On Sat, May 9, 2015 at 6:53 AM, Stas Kelvich <stas.kelv...@gmail.com> wrote:
Patch is pretty ready, last issue was about changed extension interface, so
there should be migration script and version bump.
Attaching a version with all migration stuff.
I can't see cube--1.0--1.1.sql in the patch. Did forget to include it?
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
--
Teodor Sigaev E-mail: teo...@sigaev.ru
WWW: http://www.sigaev.ru/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
