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

Hi Alexander,

On 2012-11-04 11:41:48 -0800, Jeff Davis 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.
> * Comment should indicate more explicitly that bounds_adjacent is
> sensitive to the argument order.
> * 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).
> * I could be getting confused, but I think that line 645 of
> rangetypes_spgist.c should be inverted (!bounds_adjacent(...)).
> * 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.

Do you have time to address these comments or should the patch marked
as returned with Feedback? Afaics there hasn't been progress since
CF2...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services