Re: [PATCH] Improve geometric types

On 07/31/2018 05:14 PM, Emre Hasegeli wrote:
>> 1) common_entry_cmp is still handling 'delta' as double, although the
>> CommonEntry was modified to use float8. IMHO it should also simply call
>> float8_cmp_internal instead of doing comparisons.
> 
> I am changing it to define delta as "float8" and to use float8_cmp_internal().
> 
>> 2) gist_box_picksplit does this
>>
>>     int     m = ceil(LIMIT_RATIO * (float8) nentries);
>>
>> instead of
>>
>>     int     m = ceil(LIMIT_RATIO * (double) nentries);
>>
>> which seems rather unnecessary, considering the only point of the cast was
>> probably to do more accurate multiplication. And it seems pointless to cast
>> it to float8 but then not use float8_mul().
> 
> I am removing the cast.
> 
>> 3) computeDistance does this:
>>
>>      if (point->y > box->high.y)
>>          result = float8_mi(point->y, box->high.y);
>>      else if (point->y < box->low.y)
>>          result = float8_mi(box->low.y, point->y);
>>
>> which seems suspicious. Shouldn't the comparisons be done by float8_lt and
>> float8_gt too? That's what we do elsewhere.
> 
> I assumed the GiST code already handles NaNs correctly and tried not
> to change its behavior.  It may be a good idea to revert existing NaN
> handling in favour of using the inline functions every time.  Should I
> do that?

Ah, so there's an assumption that NaNs are handled earlier and never 
reach this place? That's probably a safe assumption. I haven't thought 
about that, it simply seemed suspicious that the code mixes direct 
comparisons and float8_mi() calls.

> 
>> 4) I think we should just get rid of GEODEBUG entirely. The preceding
>> patches removes about 20 out of 27 occurrences anyway, so let's ditch the
>> remaining few.
> 
> I agree.  Shall I append it to this patch?
> 

Not sure, I'll leave that up to you. I don't mind doing it in a separate 
patch (I'd probably prefer that over mixing it into unrelated patch).

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services