Re: [PATCH] Improve geometric types

At Thu, 2 Aug 2018 11:50:55 +0200, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote in <ce3cf95a-4751-c168-54ae-636c486e06cd(at)2ndquadrant(dot)com>
>
>
> On 08/01/2018 01:40 PM, Emre Hasegeli wrote:
> >> 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.
> > The comparison functions handle NaNs. The arithmetic functions handle
> > returning error on underflow, overflow and division by zero. I
> > assumed we want to return error on those in any case, but we don't
> > want to handle NaNs at every place.
> >
> >> 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).
> > It is attached separately.
> >
>
> OK, thanks.
>
> So, have we reached conclusion about all the bits I mentioned on 7/31?
> The delta and float8/double cast are fixed, and for computeDistance
> (i.e. doing comparisons directly or using float8_lt), the code may
> seem a bit inconsistent, but it is in fact correct as the NaNs are
> handled elsewhere. That seems reasonable, but perhaps a comment
> pointing that out would be nice.

I'm not confident on replacing double to float8 partially in gist
code. After the 0002 patch applied, I see most of problematic
usage of double or bare arithmetic on dimentional values in
gistproc.c.

> static inline float
> non_negative(float val)
> {
> if (val >= 0.0f)
> return val;
> else
> return 0.0f;
> }

It is used as "non_negative(overlap)", where overlap is float4,
which is calculated using float8_mi. Float4 makes sense only if
we need to store a large number of it to somewhere but they are
just working varialbles. Couldn't we eliminate float4 that
doesn't have a requirement to do so?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center