From: | Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> |
---|---|
To: | Stas Kelvich <stas(dot)kelvich(at)gmail(dot)com>, Teodor Sigaev <teodor(at)sigaev(dot)ru>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Cube extension kNN support |
Date: | 2015-12-16 13:46:52 |
Message-ID: | 56716B4C.7000001@2ndquadrant.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On 12/16/2015 01:26 PM, Stas Kelvich wrote:
> Hi, thanks for the review.
>
>> 1) (nitpicking) There seem to be some minor whitespace issues, i.e.
>> trailing spaces, empty lines being added/removed, etc.
>
>
> Fixed, I think
>
>> 2) one of the regression tests started to fail
>>
>> SELECT '-1e-700'::cube AS cube;
>>
>> This used to return (0) but now I get (-0).
>
> Actually that problem emerged because of the first problem. I had
extra whitespace in sql file and removed that whitespace from one of the
answers file (cube_1.sql), so diff with both cube.sql and cube_1.sql was
one line length and you saw diff with cube.sql.
> In all systems that available to me (osx/linux/freebsd) I saw that
right answers file is cube_1.sql. But in other OS’es you can get +/- 0
or e27/e027. I edited that answers files manually, so there probably can
be some other typos.
Ah! So that's why I couldn't quickly find the issue in the C code ...
>
>> 3) I wonder why the docs were changed like this:
>>
>> <para>
>> - It does not matter which order the opposite corners of a cube are
>> - entered in. The <type>cube</> functions
>> - automatically swap values if needed to create a uniform
>> - <quote>lower left — upper right</> internal representation.
>> + When corners coincide cube stores only one corner along with a
>> special flag in order to reduce size wasted.
>> </para>
>>
>> Was the old behavior removed? I don't think so - it seems to behave
>> as before, so why to remove this information? Maybe it's not useful?
>> But then why add the bit about optimizing storage of points?
>
> I’ve edited it because the statement was mislead (or at least ambiguous) — cube_in function doesn’t swap coordinates.
> Simple way to see it:
>> select '(1,3),(3,1)'::cube;
> cube
> ---------------
> (1, 3),(3, 1)
>
> But LowerLeft-UpperRight representation should be (1,1),(3,3)
I don't think that's what the comment says, actually. It rather refers
to code like this:
result = Min(LL_COORD(c, n - 1), UR_COORD(c, n - 1));
i.e. if you specifically ask for a particular corner (ll, in this case),
you'll get the proper value.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From | Date | Subject | |
---|---|---|---|
Next Message | Alvaro Herrera | 2015-12-16 14:03:07 | Re: Re: Potential pointer dereference in plperl.c (caused by transforms patch) |
Previous Message | Amit Kapila | 2015-12-16 13:30:04 | Re: [PoC] Asynchronous execution again (which is not parallel) |