Re: Strange behavior with polygon and NaN
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: gkokolatos@pm.me, pgsql-hackers@lists.postgresql.org
Date: 2020-11-20T22:26:46Z
Lists: pgsql-hackers
Attachments
- v6-0001-add-more-point-tests.patch (text/x-diff) patch v6-0001
- v6-0002-fix-line_closept_point.patch (text/x-diff) patch v6-0002
- v6-0003-saner-slope-handling.patch (text/x-diff) patch v6-0003
Further to this ...
I realized after looking at things some more that one of
line_closept_point's issues is really a bug in line_construct:
it fails to draw a horizontal line through a point with x = Inf,
though surely that's not particularly ill-defined. The reason
is that somebody thought they could dispense with a special case
for m == 0, but then we end up doing
result->C = float8_mi(pt->y, float8_mul(m, pt->x));
and if m = 0 and pt->x = Inf, we get NaN.
It also annoyed me that the code was still using DBL_MAX instead of a
true Inf to represent infinite slope. That's sort of okay as long as
it's just a communication mechanism between line_construct and places
like line_sl, but it's not really okay, because in some places you can
get a true infinity from a slope calculation. Thus in HEAD you get
different results from
regression=# select line(point(1,2),point(1,'inf'));
line
----------
{-1,0,1}
(1 row)
regression=# select line(point(1,2),point(4,'inf'));
line
-------------------------
{Infinity,-1,-Infinity}
(1 row)
which is completely silly: we ought to "round off" that infinitesimal
slope to a true vertical, rather than producing a line representation
we can't do anything with.
So I fixed that too, but then I got a weird regression test diff:
the case of
lseg '[(-10,2),(-10,3)]' ?|| lseg '[(-10,2),(-10,3)]'
was no longer returning true. The reason turned out to be that
lseg_parallel does
PG_RETURN_BOOL(FPeq(lseg_sl(l1), lseg_sl(l2)));
and now lseg_sl is returning true infinities for vertical lines, and
FPeq *gets the wrong answer* when asked to compare Inf to Inf. It
should say equal, surely, but internally it computes a NaN and ends up
with false.
So the attached 0003 patch also fixes FPeq() and friends to give
sane answers for Inf-vs-Inf comparisons. That part seems like
a fairly fundamental bug fix, and so I feel like we ought to
go ahead and apply it before we do too much more messing with
the logic in this area.
(Note that the apparently-large results diff in 0003 is mostly
a whitespace change: the first hunk just reflects slopes coming
out as Infinity not DBL_MAX.)
I'm reposting 0001 and 0002 just to keep the cfbot happy,
they're the same as in my previous message.
regards, tom lane
Commits
-
In geo_ops.c, represent infinite slope as Infinity, not DBL_MAX.
- 9fe649ea295f 14.0 landed
-
Fix FPeq() and friends to get the right answers for infinities.
- 8597a48d01b6 14.0 landed
-
Extend the geometric regression test cases a little.
- a45272b25d6f 14.0 landed
-
Remove underflow error in float division with infinite divisor.
- fac83dbd6fe1 14.0 landed