Just to check where we stand on this: Are you going to send a finalized
version of this patch, based on the one I sent earlier, or should I pick
up that version and try to get it into committable state?
On 23.07.2012 10:37, Alexander Korotkov wrote:
> On Fri, Jul 20, 2012 at 3:48 PM, Heikki Linnakangas<
> heikki.linnakangas@enterprisedb.com> wrote:
>
>> On 13.07.2012 02:00, Alexander Korotkov wrote:
>>
>>> Done. There are separate patch for get rid of TrickFunctionCall2 and
>>> version of SP-GiST for ranges based on that patch.
>>>
>>
>> Looking at the SP-GiST patch now..
>>
>> It would be nice to have an introduction, perhaps as a file comment at the
>> top of rangetypes_spgist.c, explaining how the quad tree works. I have a
>> general idea of what a quad tree is, but it's not immediately obvious how
>> it maps to SP-GiST. What is stored on a leaf node and an internal node?
>> What is the 'prefix' (seems to be the centroid)? How are ranges mapped to
>> 2D points? (the function comment of getQuadrant() is a good start for that
>> last one)
>>
>
> I've added some comments at the top of rangetypes_spgist.c.
>
> In spg_range_quad_inner_**consistent(), if in->hasPrefix == true, ISTM that
>> in all cases where 'empty' is true, 'which' is set to 0, meaning that there
>> can be no matches in any of the quadrants. In most of the case-branches,
>> you explicitly check for 'empty', but even in the ones where you don't, I
>> think you end up setting which=0 if empty==true. I'm not 100% sure about
>> the RANGESTRAT_ADJACENT case, though. Am I missing something?
>>
>
> Ops., it was a bug: RANGESTRAT_ADJACENT shoud set which=0 if empty==true,
> while RANGESTRAT_CONTAINS and RANGESTRAT_CONTAINED_BY not. Corrected.
>
> It would be nice to avoid the code duplication between the new
>> bounds_adjacent() function, and the range_adjacent_internal(). Perhaps move
>> bounds_adjacent() to rangetypes.c and use it in range_adjacent_internal()
>> too?
>
>
> Done.
>
> ------
> With best regards,
> Alexander Korotkov.
>
-- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com