| From: | Emre Hasegeli <emre(at)hasegeli(dot)com> | 
|---|---|
| To: | Teodor Sigaev <teodor(at)sigaev(dot)ru> | 
| Cc: | Oleg Bartunov <obartunov(at)gmail(dot)com>, David Steele <david(at)pgmasters(dot)net>, Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org> | 
| Subject: | Re: [PATCH] we have added support for box type in SP-GiST index | 
| Date: | 2016-03-27 12:35:29 | 
| Message-ID: | CAE2gYzxkO2KDHB3y5WTmz8Rbok=POGiUMFcK08chXapHPB_AYw@mail.gmail.com | 
| Views: | Whole Thread | Raw Message | Download mbox | Resend email | 
| Thread: | |
| Lists: | pgsql-hackers | 
>>> I'll try to explain with two-dimensional example over points. ASCII-art:
>>
>> Thank you for the explanation.  Should we incorporate this with the patch.
>
> added
I have worked on the comments of the patch.  It is attached.  I hope
it looks more clear than it was before.
>>> + cmp_double(const double a, const double b)
>>
>> Does this function necessary?  We can just compare the doubles.
>
> Yes, it compares with limited precision as it does by geometry operations.
> Renamed to point actual arguments.
I meant that we could use FP macros directly instead of this function.
Doing so is also more correct.  I haven't tried to produce the
problem, but this function is not same as using the macros directly.
For differences smaller that the epsilon, it can return different
results.  I have removed it on the attached version.
>>> + boxPointerToRangeBox(BOX *box, RangeBox * rectangle)
>>
>> The "rectangle" variable in here should be renamed.
>
> fixed
I found a bunch of those too and renamed for clarity.  I have also
reordered the arguments of the helper functions to keep them
consistent.
> evalRangeBox() is used to initialize fields of RangeBox in evalRectBox(). If
> evalRangeBox() will palloc its result then we need to copy its result into
> RangeBox struct and free. Let both fucntion use the same interface.
I found it nicer to copy and edit the existing value than creating it
in two steps like this.  It is also attached.
> it works until allthesame branch contains only one inner node. Otherwise
> traversalValue will be freeed several times, see spgWalk().
It just works, when traversalValues is not set. It is also attached.
I have also added the missing regression tests.  While doing that I
noticed that some operators are missing and also added support for
them.  It is also attached with the updated version of my test script.
I hope I haven't changed the patch too much.  I have also pushed the
changes here:
| Attachment | Content-Type | Size | 
|---|---|---|
| q4d-5.patch | application/octet-stream | 44.5 KB | 
| box-index-test.sql | application/octet-stream | 2.8 KB | 
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Andrew Dunstan | 2016-03-27 12:57:35 | Re: Alter or rename enum value | 
| Previous Message | Andres Freund | 2016-03-27 12:18:58 | Re: Move PinBuffer and UnpinBuffer to atomics |