Re: Strange behavior with polygon and NaN - Mailing list pgsql-hackers

Thank you for the review, Georgios and Tom.

At Tue, 10 Nov 2020 14:30:08 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> I spent some time looking this over, and have a few thoughts:
> 
> 1. I think it's useful to split the test changes into two patches,
> as I've done below: first, just add the additional row in point_tbl
> and let the fallout from that happen, and then in the second patch
> make the code changes.  This way, it's much clearer what the actual
> behavioral changes are.  Some of them don't look right, either.
> For instance, in the very first hunk in geometry.out, we have
> this:
> 
> - (Infinity,1e+300) | {1,0,5}                               |                NaN |                NaN
> + (Infinity,1e+300) | {1,0,5}                               |           Infinity |           Infinity
> 
> which seems right, and also this:

For example, ('Infinity', 1e300) <-> {1,0,5}, that is:

   line "x = -5" <-> point(1e300, Inf)

So sqrt((1e300 - 5)^2 + Inf^2) = Inf, which looks right.


> - (1e+300,Infinity) | {1,-1,0}                              |           Infinity |           Infinity
> - (1e+300,Infinity) | {-0.4,-1,-6}                          |           Infinity |           Infinity
> - (1e+300,Infinity) | {-0.000184615384615,-1,15.3846153846} |           Infinity |           Infinity
> + (1e+300,Infinity) | {1,-1,0}                              |                NaN |                NaN
> + (1e+300,Infinity) | {-0.4,-1,-6}                          |                NaN |                NaN
> + (1e+300,Infinity) | {-0.000184615384615,-1,15.3846153846} |                NaN |                NaN
> 
> which does not.  Why aren't these distances infinite as well?
> 
> For instance, {1,-1,0} is the line "x = y".  We could argue about
> whether it'd be sensible to return zero for the distance between that
> and the point (inf,inf), but surely any point with one inf and one
> finite coordinate must be an infinite distance away from that line.
> There's nothing ill-defined about that situation.

Mmm... (swinging my arms to mimic lines..)
dist(x = y, (1e300, Inf)) looks indeterminant to me..

The calcuation is performed in the following steps.

1. construct the perpendicular line for the line.
     perpine(1e300, 'Infinity') => {-1, -1, Inf}

2. calculate the cross point.
     corsspoint({-1, -1, Inf}, {1,-1,0}) => (Inf, NaN)

3. calculte the distance from the crosspoint to the point.
     point_dt((Inf, NaN), (1e300, 'Infinity'))
   = HYPOT(Inf - 1e300, NaN - Inf);
   = HYPOT(Inf, NaN);

4. HYPOT changed the behavior by the patch

   Before: HYPOT(Inf, NaN) = Inf
   After : HYPOT(Inf, NaN) = NaN    - Result A


So if we will "fix" that, we should fix any, some, or all of 1-3.

1. seems to have no other way than the result.

2. crosspoint (x = - y + Inf, x = y)  could be (Inf, Inf)?

3.    point_dt((Inf, Inf), (1e300, Inf))
   = HYPOT(Inf - 1e300, Inf - Inf)
   = HYPOT(Inf, -NaN)
   = NaN.                           - Result B

  I'm not sure why Inf - Inf is negative, but |Inf-Inf| = NaN is
  reasonable.

That is, we don't get a "reasonable" result this way.


The formula for the distance((x0,y0) - (ax + by + c = 0)) is

  |ax0 + by0 + c|/sqrt(a^2 + b^2)

 where a = -1, b = -1, c = Inf, x0 = 1e300, y0 = Inf,

    abs(-1 * 1e300 + -1 * Inf + Inf) / sqrt(1 + 1)
  = abs(-1e300 -Inf + Inf) / C
  = NaN.                           - Result C

All of the Result A - C is NaN. At last NaN looks to be the right
result.

By the way that the formula is far simple than what we are doing
now. Is there any reason to take the above steps for the calculation?


> 2. Rather than coding around undesirable behavior of float8_min,
> it seems like it's better to add a primitive to float.h that
> does what you want, ie "NaN if either input is NaN, else the
> smaller input".  This is more readable, and possibly more efficient
> (depending on whether the compiler is smart enough to optimize
> away redundant isnan checks).  I did that in the attached.

Sounds reasonable. I found that I forgot to do the same thing to y
coordinate.

> 3. Looking for other calls of float8_min, I wonder why you did not
> touch the bounding-box calculations in box_interpt_lseg() or
> boxes_bound_box().

While doing that, I didn't make changes just by looking a code locally
since I thought that that might be looked as overdone.  Maybe, for
example box_interpt_lseg, even if bounding-box check overlooked NaNs,
I thought that the following calcualaions reflect any involved NaNs to
the result. (But I'm not confident that that is perfect, though..)

> 4. The line changes feel a bit weird, like there's no clear idea
> of what a "valid" or "invalid" line is.  For instance the first
> hunk in line_construct():
> 
> +        /* Avoid creating a valid line from an invalid point */
> +        if (unlikely(isnan(pt->y)))
> +            result->C = get_float8_nan();
> 
> Why's it appropriate to set C and only C to NaN?

Not limited to here, I intended to reduce the patch footprint as much
as possible and it seemed that only set C to NaN is sufficient. (But
I'm not con<snip..>) I don't object to make that change more
comprehensively. Do we go that direction?

> 5. But actually there's a bigger issue with that particular hunk.
> This code branch is dealing with "draw a vertical line through this
> point", so why should we care what the point's y coordinate is --- that
> is, why is this particular change appropriate at all?  The usual rule as

The calculation mess comes from omitting a part of the component
values during calculation.  So:

+     <para>
+       NaN and Infinity make geometric functions and operators behave
+       inconsistently. Geometric operators or functions that return a boolean
+       return false for operands that contain NaNs. Number-returning functions
+       and operators return NaN in most cases but sometimes return a valid
+       value if no NaNs are met while actual calculation.  Object-returning one
+       yield an object that contain NaNs depending to the operation.  Likewise

The code is following this policy. A point containing NaN yields an
"invalid" line, that is, a line containg NaN.

> I understand it is that if a function's result is determined by some of
> its arguments independently of what another argument's value is, then it
> doesn't matter if that one is NaN, you can still return the same result.

That's true looking from pure calculation point of view, which caused
some of the messes.

> 6. I'm a bit uncomfortable with the use of "bool isnan" in a couple
> of places.  I think it's confusing to use that alongside the isnan()
> macro.  Moreover, it's at least possible that some platforms implement
> isnan() in a way that would break this usage.  The C spec specifically
> says that isnan() is a macro not a function ... but it doesn't commit
> to it being a macro-with-arguments.  I think "anynan" or something
> like that would be a better choice of name.

Ooo! Rright. I agreed to that. Will fix them.

> [ a bit later... ] Indeed, I get a compile failure on gaur:
> 
> geo_ops.c: In function 'lseg_closept_lseg':
> geo_ops.c:2906:17: error: called object 'isnan' is not a function
> geo_ops.c:2906:32: error: called object 'isnan' is not a function
> geo_ops.c:2916:16: error: called object 'isnan' is not a function
> geo_ops.c:2924:16: error: called object 'isnan' is not a function
> geo_ops.c: In function 'box_closept_point':
> geo_ops.c:2989:16: error: called object 'isnan' is not a function
> geo_ops.c:2992:16: error: called object 'isnan' is not a function
> geo_ops.c:3004:16: error: called object 'isnan' is not a function
> geo_ops.c:3014:16: error: called object 'isnan' is not a function
> make: *** [geo_ops.o] Error 1
> 
> So that scenario isn't hypothetical.  Please rename the variables.

lol! gaur looks like coal mine canary.

1. Won't fix the dist_pl/lp's changed behavior.
2. (already fixed?) Will find other instances.
3. Will do more comprehensive NaN-detection (as another patch)
4. Ditto.
5. Keep the curent state. Do we revert that?
6. Will fix.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center