Re: [PATCH] we have added support for box type in SP-GiST index

Thank you, pushed

Emre Hasegeli wrote:
>>>> 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:
>
> https://github.com/hasegeli/postgres/commits/box-spgist
>

--
Teodor Sigaev E-mail: teodor(at)sigaev(dot)ru
WWW: http://www.sigaev.ru/