From 8597a48d01b6cc0b09ff626253ac93c67e5516d5 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 21 Nov 2020 16:46:43 -0500 Subject: [PATCH] Fix FPeq() and friends to get the right answers for infinities. "FPeq(infinity, infinity)" returned false, on account of getting NaN when it subtracts the two inputs. Fix that by adding a separate check for exact equality. FPle() and FPge() similarly got the wrong answer for two like-signed infinities. In those cases, we can just rearrange the comparisons to avoid potentially subtracting infinities. While the sibling functions FPne() etc accidentally gave the right answers even with the internal NaN results, it seems best to make similar adjustments to them to avoid depending on this. FPeq() has to be converted to an inline function to avoid double evaluations of its arguments, and I did the same for the others just for consistency. In passing, make the handling of NaN cases in line_eq() and point_eq_point() simpler and easier to reason about, and perhaps faster. This results in just one visible regression test change: slope() now gives DBL_MAX for two inputs of (inf,1e300), which is consistent with what it does for (1e300,inf), so that seems like a bug fix. Discussion: https://postgr.es/m/CAGf+fX70rWFOk5cd00uMfa__0yP+vtQg5ck7c2Onb-Yczp0URA@mail.gmail.com --- src/backend/utils/adt/geo_ops.c | 39 ++++++++++-------- src/include/utils/geo_decls.h | 57 ++++++++++++++++++++++---- src/test/regress/expected/geometry.out | 2 +- 3 files changed, 72 insertions(+), 26 deletions(-) diff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.c index a7db7839588..82286ef87a3 100644 --- a/src/backend/utils/adt/geo_ops.c +++ b/src/backend/utils/adt/geo_ops.c @@ -1155,9 +1155,6 @@ line_horizontal(PG_FUNCTION_ARGS) /* * Check whether the two lines are the same - * - * We consider NaNs values to be equal to each other to let those lines - * to be found. */ Datum line_eq(PG_FUNCTION_ARGS) @@ -1166,21 +1163,28 @@ line_eq(PG_FUNCTION_ARGS) LINE *l2 = PG_GETARG_LINE_P(1); float8 ratio; - if (!FPzero(l2->A) && !isnan(l2->A)) + /* If any NaNs are involved, insist on exact equality */ + if (unlikely(isnan(l1->A) || isnan(l1->B) || isnan(l1->C) || + isnan(l2->A) || isnan(l2->B) || isnan(l2->C))) + { + PG_RETURN_BOOL(float8_eq(l1->A, l2->A) && + float8_eq(l1->B, l2->B) && + float8_eq(l1->C, l2->C)); + } + + /* Otherwise, lines whose parameters are proportional are the same */ + if (!FPzero(l2->A)) ratio = float8_div(l1->A, l2->A); - else if (!FPzero(l2->B) && !isnan(l2->B)) + else if (!FPzero(l2->B)) ratio = float8_div(l1->B, l2->B); - else if (!FPzero(l2->C) && !isnan(l2->C)) + else if (!FPzero(l2->C)) ratio = float8_div(l1->C, l2->C); else ratio = 1.0; - PG_RETURN_BOOL((FPeq(l1->A, float8_mul(ratio, l2->A)) && - FPeq(l1->B, float8_mul(ratio, l2->B)) && - FPeq(l1->C, float8_mul(ratio, l2->C))) || - (float8_eq(l1->A, l2->A) && - float8_eq(l1->B, l2->B) && - float8_eq(l1->C, l2->C))); + PG_RETURN_BOOL(FPeq(l1->A, float8_mul(ratio, l2->A)) && + FPeq(l1->B, float8_mul(ratio, l2->B)) && + FPeq(l1->C, float8_mul(ratio, l2->C))); } @@ -1930,15 +1934,16 @@ point_ne(PG_FUNCTION_ARGS) /* * Check whether the two points are the same - * - * We consider NaNs coordinates to be equal to each other to let those points - * to be found. */ static inline bool point_eq_point(Point *pt1, Point *pt2) { - return ((FPeq(pt1->x, pt2->x) && FPeq(pt1->y, pt2->y)) || - (float8_eq(pt1->x, pt2->x) && float8_eq(pt1->y, pt2->y))); + /* If any NaNs are involved, insist on exact equality */ + if (unlikely(isnan(pt1->x) || isnan(pt1->y) || + isnan(pt2->x) || isnan(pt2->y))) + return (float8_eq(pt1->x, pt2->x) && float8_eq(pt1->y, pt2->y)); + + return (FPeq(pt1->x, pt2->x) && FPeq(pt1->y, pt2->y)); } diff --git a/src/include/utils/geo_decls.h b/src/include/utils/geo_decls.h index d4617558f16..1b96990dcc4 100644 --- a/src/include/utils/geo_decls.h +++ b/src/include/utils/geo_decls.h @@ -18,25 +18,66 @@ #ifndef GEO_DECLS_H #define GEO_DECLS_H +#include + #include "fmgr.h" /*-------------------------------------------------------------------- * Useful floating point utilities and constants. - *------------------------------------------------------------------- + *-------------------------------------------------------------------- * - * XXX: They are not NaN-aware. + * "Fuzzy" floating-point comparisons: values within EPSILON of each other + * are considered equal. Beware of normal reasoning about the behavior of + * these comparisons, since for example FPeq does not behave transitively. + * + * Note that these functions are not NaN-aware and will give FALSE for + * any case involving NaN inputs. + * + * Also note that these will give sane answers for infinite inputs, + * where it's important to avoid computing Inf minus Inf; we do so + * by eliminating equality cases before subtracting. */ #define EPSILON 1.0E-06 #ifdef EPSILON #define FPzero(A) (fabs(A) <= EPSILON) -#define FPeq(A,B) (fabs((A) - (B)) <= EPSILON) -#define FPne(A,B) (fabs((A) - (B)) > EPSILON) -#define FPlt(A,B) ((B) - (A) > EPSILON) -#define FPle(A,B) ((A) - (B) <= EPSILON) -#define FPgt(A,B) ((A) - (B) > EPSILON) -#define FPge(A,B) ((B) - (A) <= EPSILON) + +static inline bool +FPeq(double A, double B) +{ + return A == B || fabs(A - B) <= EPSILON; +} + +static inline bool +FPne(double A, double B) +{ + return A != B && fabs(A - B) > EPSILON; +} + +static inline bool +FPlt(double A, double B) +{ + return A + EPSILON < B; +} + +static inline bool +FPle(double A, double B) +{ + return A <= B + EPSILON; +} + +static inline bool +FPgt(double A, double B) +{ + return A > B + EPSILON; +} + +static inline bool +FPge(double A, double B) +{ + return A + EPSILON >= B; +} #else #define FPzero(A) ((A) == 0) #define FPeq(A,B) ((A) == (B)) diff --git a/src/test/regress/expected/geometry.out b/src/test/regress/expected/geometry.out index 1ffa440a7c9..1d2508987d8 100644 --- a/src/test/regress/expected/geometry.out +++ b/src/test/regress/expected/geometry.out @@ -190,7 +190,7 @@ SELECT p1.f1, p2.f1, slope(p1.f1, p2.f1) FROM POINT_TBL p1, POINT_TBL p2; (Infinity,1e+300) | (-5,-12) | 0 (Infinity,1e+300) | (1e-300,-1e-300) | 0 (Infinity,1e+300) | (1e+300,Infinity) | NaN - (Infinity,1e+300) | (Infinity,1e+300) | 0 + (Infinity,1e+300) | (Infinity,1e+300) | 1.79769313486e+308 (Infinity,1e+300) | (NaN,NaN) | NaN (Infinity,1e+300) | (10,10) | 0 (NaN,NaN) | (0,0) | NaN