At Fri, 03 Aug 2018 13:38:40 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyot...@lab.ntt.co.jp> wrote in <20180803.133840.180843182.horiguchi.kyot...@lab.ntt.co.jp> > At Thu, 2 Aug 2018 11:50:55 +0200, Tomas Vondra > <tomas.von...@2ndquadrant.com> wrote in > <ce3cf95a-4751-c168-54ae-636c486e0...@2ndquadrant.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; > > } > > This takes float(4) and it is used as "non_negative(overlap)", > where overlap is float4, which is calculated using float8_mi. > Float4 makes sense if we store a large number of values somewhere > but they are just working varialbles. Couldn't we eliminate > float(4) whthout any specifc requirement?
I'm fine with the 0002-geo-float-v14 except the above. regards. -- Kyotaro Horiguchi NTT Open Source Software Center