From: | Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> |
---|---|
To: | emre(at)hasegeli(dot)com |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: [PATCH] Improve geometric types |
Date: | 2018-07-31 15:22:20 |
Message-ID: | 96386075-e89f-1875-b349-80a0c9d9e718@2ndquadrant.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Luzanov | 2018-07-31 15:30:48 | Re: Usability fail with psql's \dp command |
Previous Message | Emre Hasegeli | 2018-07-31 15:14:15 | Re: [PATCH] Improve geometric types |