Re: SP-GiST for ranges based on 2d-mapping and quad-tree

Hi!

On Sun, Nov 4, 2012 at 11:41 PM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:

> On Fri, 2012-11-02 at 12:47 +0400, Alexander Korotkov wrote:
>
> > Right version of patch is attached.
> >
> * In bounds_adjacent, there's no reason to flip the labels back.
>

Fixed.

> * Comment should indicate more explicitly that bounds_adjacent is
> sensitive to the argument order.
>

Fixed.

> * In bounds_adjacent, it appears that "bound1" corresponds to "B" in the
> comment above, and "bound2" corresponds to "A" in the comment above. I
> would have guessed from reading the comment that bound1 corresponded to
> A. We should just use consistent names between the comment and the code
> (e.g. boundA and boundB).
>

Fixed.

> * I could be getting confused, but I think that line 645 of
> rangetypes_spgist.c should be inverted (!bounds_adjacent(...)).
>

Good catch! Actually, code was in direct contradiction with comment. Fixed.

> * I think needPrevious should have an explanatory comment somewhere. It
> looks like you are using it to store some state as you descend the tree,
> but it doesn't look like it's used to reconstruct the value (because we
> already have the value anyway). Since it's being used for a purpose
> other than what's intended, that should be explained.
>

Yes, it's a some kind of hack now. Comment is added.

------
With best regards,
Alexander Korotkov.