Thread: Wrong results from in_range() tests with infinite offset
Dean Rasheed pointed out that in_range for float4/float8 seems to be
doing the wrong thing for infinite offsets, and after some testing
I concur that it is. For example, a sort key of '-infinity' should
be considered to be in-range for a range specified as RANGE BETWEEN
'inf' PRECEDING AND 'inf' PRECEDING; but with the code as it stands,
it isn't. I propose the attached patch, which probably should be
back-patched.
When the current row's value is +infinity, actual computation of
base - offset would yield NaN, making it a bit unclear whether
we should consider -infinity to be in-range. It seems to me that
we should, as that gives more natural-looking results in the test
cases, so that's how the patch does it.
regards, tom lane
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 50ec3d3dde..5c722d85c3 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -1090,11 +1090,18 @@ in_range_float8_float8(PG_FUNCTION_ARGS)
/*
* Deal with infinite offset (necessarily +inf, at this point). We must
* special-case this because if base happens to be -inf, their sum would
- * be NaN, which is an overflow-ish condition we should avoid.
+ * be NaN, which is an overflow-ish condition we must avoid. We take that
+ * sum to be +inf; that is, we consider base + inf to be +inf for any
+ * non-NaN base. Conversely, base - inf is taken to be -inf even if base
+ * is +inf. These choices may seem arbitrary, but they give desirable
+ * windowing behavior with an infinite offset.
*/
if (isinf(offset))
{
- PG_RETURN_BOOL(sub ? !less : less);
+ if (sub)
+ sum = -get_float8_infinity();
+ else
+ sum = get_float8_infinity();
}
/*
@@ -1103,7 +1110,7 @@ in_range_float8_float8(PG_FUNCTION_ARGS)
* produce a suitably signed infinity, which will compare properly against
* val whether or not that's infinity.
*/
- if (sub)
+ else if (sub)
sum = base - offset;
else
sum = base + offset;
@@ -1159,11 +1166,18 @@ in_range_float4_float8(PG_FUNCTION_ARGS)
/*
* Deal with infinite offset (necessarily +inf, at this point). We must
* special-case this because if base happens to be -inf, their sum would
- * be NaN, which is an overflow-ish condition we should avoid.
+ * be NaN, which is an overflow-ish condition we must avoid. We take that
+ * sum to be +inf; that is, we consider base + inf to be +inf for any
+ * non-NaN base. Conversely, base - inf is taken to be -inf even if base
+ * is +inf. These choices may seem arbitrary, but they give desirable
+ * windowing behavior with an infinite offset.
*/
if (isinf(offset))
{
- PG_RETURN_BOOL(sub ? !less : less);
+ if (sub)
+ sum = -get_float8_infinity();
+ else
+ sum = get_float8_infinity();
}
/*
@@ -1172,7 +1186,7 @@ in_range_float4_float8(PG_FUNCTION_ARGS)
* produce a suitably signed infinity, which will compare properly against
* val whether or not that's infinity.
*/
- if (sub)
+ else if (sub)
sum = base - offset;
else
sum = base + offset;
diff --git a/src/test/regress/expected/window.out b/src/test/regress/expected/window.out
index d5fd4045f9..4cb68ca42f 100644
--- a/src/test/regress/expected/window.out
+++ b/src/test/regress/expected/window.out
@@ -1936,6 +1936,42 @@ window w as (order by f_float4 range between
9 | NaN | 9 | 9
(10 rows)
+select id, f_float4, first_value(id) over w, last_value(id) over w
+from numerics
+window w as (order by f_float4 range between
+ 'inf' preceding and 'inf' preceding);
+ id | f_float4 | first_value | last_value
+----+-----------+-------------+------------
+ 0 | -Infinity | 0 | 0
+ 1 | -3 | 0 | 0
+ 2 | -1 | 0 | 0
+ 3 | 0 | 0 | 0
+ 4 | 1.1 | 0 | 0
+ 5 | 1.12 | 0 | 0
+ 6 | 2 | 0 | 0
+ 7 | 100 | 0 | 0
+ 8 | Infinity | 0 | 0
+ 9 | NaN | 9 | 9
+(10 rows)
+
+select id, f_float4, first_value(id) over w, last_value(id) over w
+from numerics
+window w as (order by f_float4 range between
+ 'inf' following and 'inf' following);
+ id | f_float4 | first_value | last_value
+----+-----------+-------------+------------
+ 0 | -Infinity | 8 | 8
+ 1 | -3 | 8 | 8
+ 2 | -1 | 8 | 8
+ 3 | 0 | 8 | 8
+ 4 | 1.1 | 8 | 8
+ 5 | 1.12 | 8 | 8
+ 6 | 2 | 8 | 8
+ 7 | 100 | 8 | 8
+ 8 | Infinity | 8 | 8
+ 9 | NaN | 9 | 9
+(10 rows)
+
select id, f_float4, first_value(id) over w, last_value(id) over w
from numerics
window w as (order by f_float4 range between
@@ -1995,6 +2031,42 @@ window w as (order by f_float8 range between
9 | NaN | 9 | 9
(10 rows)
+select id, f_float8, first_value(id) over w, last_value(id) over w
+from numerics
+window w as (order by f_float8 range between
+ 'inf' preceding and 'inf' preceding);
+ id | f_float8 | first_value | last_value
+----+-----------+-------------+------------
+ 0 | -Infinity | 0 | 0
+ 1 | -3 | 0 | 0
+ 2 | -1 | 0 | 0
+ 3 | 0 | 0 | 0
+ 4 | 1.1 | 0 | 0
+ 5 | 1.12 | 0 | 0
+ 6 | 2 | 0 | 0
+ 7 | 100 | 0 | 0
+ 8 | Infinity | 0 | 0
+ 9 | NaN | 9 | 9
+(10 rows)
+
+select id, f_float8, first_value(id) over w, last_value(id) over w
+from numerics
+window w as (order by f_float8 range between
+ 'inf' following and 'inf' following);
+ id | f_float8 | first_value | last_value
+----+-----------+-------------+------------
+ 0 | -Infinity | 8 | 8
+ 1 | -3 | 8 | 8
+ 2 | -1 | 8 | 8
+ 3 | 0 | 8 | 8
+ 4 | 1.1 | 8 | 8
+ 5 | 1.12 | 8 | 8
+ 6 | 2 | 8 | 8
+ 7 | 100 | 8 | 8
+ 8 | Infinity | 8 | 8
+ 9 | NaN | 9 | 9
+(10 rows)
+
select id, f_float8, first_value(id) over w, last_value(id) over w
from numerics
window w as (order by f_float8 range between
diff --git a/src/test/regress/sql/window.sql b/src/test/regress/sql/window.sql
index fe273aa31e..51ec0bac9a 100644
--- a/src/test/regress/sql/window.sql
+++ b/src/test/regress/sql/window.sql
@@ -524,6 +524,14 @@ window w as (order by f_float4 range between
'inf' preceding and 'inf' following);
select id, f_float4, first_value(id) over w, last_value(id) over w
from numerics
+window w as (order by f_float4 range between
+ 'inf' preceding and 'inf' preceding);
+select id, f_float4, first_value(id) over w, last_value(id) over w
+from numerics
+window w as (order by f_float4 range between
+ 'inf' following and 'inf' following);
+select id, f_float4, first_value(id) over w, last_value(id) over w
+from numerics
window w as (order by f_float4 range between
1.1 preceding and 'NaN' following); -- error, NaN disallowed
@@ -541,6 +549,14 @@ window w as (order by f_float8 range between
'inf' preceding and 'inf' following);
select id, f_float8, first_value(id) over w, last_value(id) over w
from numerics
+window w as (order by f_float8 range between
+ 'inf' preceding and 'inf' preceding);
+select id, f_float8, first_value(id) over w, last_value(id) over w
+from numerics
+window w as (order by f_float8 range between
+ 'inf' following and 'inf' following);
+select id, f_float8, first_value(id) over w, last_value(id) over w
+from numerics
window w as (order by f_float8 range between
1.1 preceding and 'NaN' following); -- error, NaN disallowed
I wrote:
> When the current row's value is +infinity, actual computation of
> base - offset would yield NaN, making it a bit unclear whether
> we should consider -infinity to be in-range. It seems to me that
> we should, as that gives more natural-looking results in the test
> cases, so that's how the patch does it.
Actually, after staring at those results awhile longer, I decided
they were wrong. The results shown here seem actually sane ---
for instance, -Infinity shouldn't "infinitely precede" itself,
I think. (Maybe if you got solipsistic enough you could argue
that that is valid, but it seems pretty bogus.)
regards, tom lane
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 50ec3d3dde..7333ec667a 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -1089,17 +1089,45 @@ in_range_float8_float8(PG_FUNCTION_ARGS)
/*
* Deal with infinite offset (necessarily +inf, at this point). We must
- * special-case this because if base happens to be -inf, their sum would
- * be NaN, which is an overflow-ish condition we should avoid.
+ * special-case this when base is also infinite, first because we must
+ * avoid subtracting inf from inf (the result would be NaN), and second
+ * because we'd like to consider inf + inf to be "greater than inf"
+ * (otherwise we'd have oddities like inf being considered to infinitely
+ * precede itself). That leads to the following code.
*/
- if (isinf(offset))
+ if (isinf(offset) && isinf(base))
{
- PG_RETURN_BOOL(sub ? !less : less);
+ if (sub)
+ {
+ if (less)
+ {
+ /* val <= base - inf: true if base is +inf and val isn't */
+ PG_RETURN_BOOL(base > 0 && (!isinf(val) || val < 0));
+ }
+ else
+ {
+ /* val >= base - inf: true for all val & either sign of base */
+ PG_RETURN_BOOL(true);
+ }
+ }
+ else
+ {
+ if (less)
+ {
+ /* val <= base + inf: true for all val & either sign of base */
+ PG_RETURN_BOOL(true);
+ }
+ else
+ {
+ /* val >= base + inf: true if base is -inf and val isn't */
+ PG_RETURN_BOOL(base < 0 && (!isinf(val) || val > 0));
+ }
+ }
}
/*
* Otherwise it should be safe to compute base +/- offset. We trust the
- * FPU to cope if base is +/-inf or the true sum would overflow, and
+ * FPU to cope if one input is +/-inf or the true sum would overflow, and
* produce a suitably signed infinity, which will compare properly against
* val whether or not that's infinity.
*/
@@ -1158,17 +1186,45 @@ in_range_float4_float8(PG_FUNCTION_ARGS)
/*
* Deal with infinite offset (necessarily +inf, at this point). We must
- * special-case this because if base happens to be -inf, their sum would
- * be NaN, which is an overflow-ish condition we should avoid.
+ * special-case this when base is also infinite, first because we must
+ * avoid subtracting inf from inf (the result would be NaN), and second
+ * because we'd like to consider inf + inf to be "greater than inf"
+ * (otherwise we'd have oddities like inf being considered to infinitely
+ * precede itself). That leads to the following code.
*/
- if (isinf(offset))
+ if (isinf(offset) && isinf(base))
{
- PG_RETURN_BOOL(sub ? !less : less);
+ if (sub)
+ {
+ if (less)
+ {
+ /* val <= base - inf: true if base is +inf and val isn't */
+ PG_RETURN_BOOL(base > 0 && (!isinf(val) || val < 0));
+ }
+ else
+ {
+ /* val >= base - inf: true for all val & either sign of base */
+ PG_RETURN_BOOL(true);
+ }
+ }
+ else
+ {
+ if (less)
+ {
+ /* val <= base + inf: true for all val & either sign of base */
+ PG_RETURN_BOOL(true);
+ }
+ else
+ {
+ /* val >= base + inf: true if base is -inf and val isn't */
+ PG_RETURN_BOOL(base < 0 && (!isinf(val) || val > 0));
+ }
+ }
}
/*
* Otherwise it should be safe to compute base +/- offset. We trust the
- * FPU to cope if base is +/-inf or the true sum would overflow, and
+ * FPU to cope if one input is +/-inf or the true sum would overflow, and
* produce a suitably signed infinity, which will compare properly against
* val whether or not that's infinity.
*/
diff --git a/src/test/regress/expected/window.out b/src/test/regress/expected/window.out
index d5fd4045f9..8043fccbb2 100644
--- a/src/test/regress/expected/window.out
+++ b/src/test/regress/expected/window.out
@@ -1936,6 +1936,42 @@ window w as (order by f_float4 range between
9 | NaN | 9 | 9
(10 rows)
+select id, f_float4, first_value(id) over w, last_value(id) over w
+from numerics
+window w as (order by f_float4 range between
+ 'inf' preceding and 'inf' preceding);
+ id | f_float4 | first_value | last_value
+----+-----------+-------------+------------
+ 0 | -Infinity | |
+ 1 | -3 | 0 | 0
+ 2 | -1 | 0 | 0
+ 3 | 0 | 0 | 0
+ 4 | 1.1 | 0 | 0
+ 5 | 1.12 | 0 | 0
+ 6 | 2 | 0 | 0
+ 7 | 100 | 0 | 0
+ 8 | Infinity | 0 | 7
+ 9 | NaN | 9 | 9
+(10 rows)
+
+select id, f_float4, first_value(id) over w, last_value(id) over w
+from numerics
+window w as (order by f_float4 range between
+ 'inf' following and 'inf' following);
+ id | f_float4 | first_value | last_value
+----+-----------+-------------+------------
+ 0 | -Infinity | 1 | 8
+ 1 | -3 | 8 | 8
+ 2 | -1 | 8 | 8
+ 3 | 0 | 8 | 8
+ 4 | 1.1 | 8 | 8
+ 5 | 1.12 | 8 | 8
+ 6 | 2 | 8 | 8
+ 7 | 100 | 8 | 8
+ 8 | Infinity | |
+ 9 | NaN | 9 | 9
+(10 rows)
+
select id, f_float4, first_value(id) over w, last_value(id) over w
from numerics
window w as (order by f_float4 range between
@@ -1995,6 +2031,42 @@ window w as (order by f_float8 range between
9 | NaN | 9 | 9
(10 rows)
+select id, f_float8, first_value(id) over w, last_value(id) over w
+from numerics
+window w as (order by f_float8 range between
+ 'inf' preceding and 'inf' preceding);
+ id | f_float8 | first_value | last_value
+----+-----------+-------------+------------
+ 0 | -Infinity | |
+ 1 | -3 | 0 | 0
+ 2 | -1 | 0 | 0
+ 3 | 0 | 0 | 0
+ 4 | 1.1 | 0 | 0
+ 5 | 1.12 | 0 | 0
+ 6 | 2 | 0 | 0
+ 7 | 100 | 0 | 0
+ 8 | Infinity | 0 | 7
+ 9 | NaN | 9 | 9
+(10 rows)
+
+select id, f_float8, first_value(id) over w, last_value(id) over w
+from numerics
+window w as (order by f_float8 range between
+ 'inf' following and 'inf' following);
+ id | f_float8 | first_value | last_value
+----+-----------+-------------+------------
+ 0 | -Infinity | 1 | 8
+ 1 | -3 | 8 | 8
+ 2 | -1 | 8 | 8
+ 3 | 0 | 8 | 8
+ 4 | 1.1 | 8 | 8
+ 5 | 1.12 | 8 | 8
+ 6 | 2 | 8 | 8
+ 7 | 100 | 8 | 8
+ 8 | Infinity | |
+ 9 | NaN | 9 | 9
+(10 rows)
+
select id, f_float8, first_value(id) over w, last_value(id) over w
from numerics
window w as (order by f_float8 range between
diff --git a/src/test/regress/sql/window.sql b/src/test/regress/sql/window.sql
index fe273aa31e..51ec0bac9a 100644
--- a/src/test/regress/sql/window.sql
+++ b/src/test/regress/sql/window.sql
@@ -524,6 +524,14 @@ window w as (order by f_float4 range between
'inf' preceding and 'inf' following);
select id, f_float4, first_value(id) over w, last_value(id) over w
from numerics
+window w as (order by f_float4 range between
+ 'inf' preceding and 'inf' preceding);
+select id, f_float4, first_value(id) over w, last_value(id) over w
+from numerics
+window w as (order by f_float4 range between
+ 'inf' following and 'inf' following);
+select id, f_float4, first_value(id) over w, last_value(id) over w
+from numerics
window w as (order by f_float4 range between
1.1 preceding and 'NaN' following); -- error, NaN disallowed
@@ -541,6 +549,14 @@ window w as (order by f_float8 range between
'inf' preceding and 'inf' following);
select id, f_float8, first_value(id) over w, last_value(id) over w
from numerics
+window w as (order by f_float8 range between
+ 'inf' preceding and 'inf' preceding);
+select id, f_float8, first_value(id) over w, last_value(id) over w
+from numerics
+window w as (order by f_float8 range between
+ 'inf' following and 'inf' following);
+select id, f_float8, first_value(id) over w, last_value(id) over w
+from numerics
window w as (order by f_float8 range between
1.1 preceding and 'NaN' following); -- error, NaN disallowed
On Thu, 16 Jul 2020, 22:50 Tom Lane, <tgl@sss.pgh.pa.us> wrote:
I wrote:
> When the current row's value is +infinity, actual computation of
> base - offset would yield NaN, making it a bit unclear whether
> we should consider -infinity to be in-range. It seems to me that
> we should, as that gives more natural-looking results in the test
> cases, so that's how the patch does it.
Actually, after staring at those results awhile longer, I decided
they were wrong. The results shown here seem actually sane ---
for instance, -Infinity shouldn't "infinitely precede" itself,
I think. (Maybe if you got solipsistic enough you could argue
that that is valid, but it seems pretty bogus.)
Hmm, that code looks a bit fishy to me, but I really need to think about it some more. I'll take another look tomorrow, and maybe it'll become clearer.
Regards,
Dean
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On Thu, 16 Jul 2020, 22:50 Tom Lane, <tgl@sss.pgh.pa.us> wrote:
>> Actually, after staring at those results awhile longer, I decided
>> they were wrong. The results shown here seem actually sane ---
>> for instance, -Infinity shouldn't "infinitely precede" itself,
>> I think. (Maybe if you got solipsistic enough you could argue
>> that that is valid, but it seems pretty bogus.)
> Hmm, that code looks a bit fishy to me, but I really need to think about it
> some more. I'll take another look tomorrow, and maybe it'll become clearer.
It's certainly verbose, so I'd like to find a more concise way to
write the logic. But the v2 results seem right.
regards, tom lane
On Fri, 17 Jul 2020 at 01:59, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> > On Thu, 16 Jul 2020, 22:50 Tom Lane, <tgl@sss.pgh.pa.us> wrote:
> >> Actually, after staring at those results awhile longer, I decided
> >> they were wrong. The results shown here seem actually sane ---
> >> for instance, -Infinity shouldn't "infinitely precede" itself,
> >> I think. (Maybe if you got solipsistic enough you could argue
> >> that that is valid, but it seems pretty bogus.)
>
> > Hmm, that code looks a bit fishy to me, but I really need to think about it
> > some more. I'll take another look tomorrow, and maybe it'll become clearer.
>
> It's certainly verbose, so I'd like to find a more concise way to
> write the logic. But the v2 results seem right.
>
I'm finding it hard to come up with a principled argument to say
exactly what the right results should be.
As things stand (pre-patch), a window frame defined as "BETWEEN 'inf'
PRECEDING AND 'inf' PRECEDING", produces the following:
id | f_float4 | first_value | last_value
----+-----------+-------------+------------
0 | -Infinity | |
1 | -3 | |
2 | -1 | |
3 | 0 | |
4 | 1.1 | |
5 | 1.12 | |
6 | 2 | |
7 | 100 | |
8 | Infinity | |
9 | NaN | 9 | 9
(10 rows)
which is clearly wrong, because -Inf obviously infinitely precedes all
the other (non-NaN) values.
With the first version of the patch, that result became
id | f_float4 | first_value | last_value
----+-----------+-------------+------------
0 | -Infinity | 0 | 0
1 | -3 | 0 | 0
2 | -1 | 0 | 0
3 | 0 | 0 | 0
4 | 1.1 | 0 | 0
5 | 1.12 | 0 | 0
6 | 2 | 0 | 0
7 | 100 | 0 | 0
8 | Infinity | 0 | 0
9 | NaN | 9 | 9
(10 rows)
which is definitely better, but the one obvious problem is last_value
for id=8, because all the values in earlier rows infinitely precede
+Inf, so they should be included in the window frame for that row.
With the second version of the patch, the result is
id | f_float4 | first_value | last_value
----+-----------+-------------+------------
0 | -Infinity | |
1 | -3 | 0 | 0
2 | -1 | 0 | 0
3 | 0 | 0 | 0
4 | 1.1 | 0 | 0
5 | 1.12 | 0 | 0
6 | 2 | 0 | 0
7 | 100 | 0 | 0
8 | Infinity | 0 | 7
9 | NaN | 9 | 9
(10 rows)
That fixes last_value for id=8, using the fact that all values less
than +Inf infinitely precede it, and also assuming that +Inf does not
infinitely precede itself, which seems reasonable.
The other change is in the first row, because it now assumes that -Inf
doesn't infinitely precede itself, which seems reasonable from a
consistency point of view.
However, that is also a bit odd because it goes against the documented
contract of in_range(), which is supposed to do the tests
val <= base +/- offset1
val >= base +/- offset2
which for "BETWEEN 'inf' PRECEDING AND 'inf' PRECEDING" become
val = base - Inf
which is -Inf, even if base = -Inf. So I'd say that the window
infinitely preceding -Inf contains -Inf, since -Inf - Inf = -Inf.
But if -Inf infinitely precedes -Inf, it probably also makes sense to
say that +Inf infinitely precedes +Inf for consistency, even though
that really isn't well-defined, since Inf - Inf = NaN. Doing that is
certainly a lot easier to code, because it just needs to return true
if base +/- offset would be NaN, i.e.,
/*
* Deal with cases where both base and offset are infinite, and computing
* base +/- offset would produce NaN. This corresponds to a window frame
* whose boundary infinitely precedes +inf or infinitely follows -inf,
* which is not well-defined. For consistency with other cases involving
* infinities, such as the fact that +inf infinitely follows +inf, we
* choose to assume that +inf infinitely precedes +inf and -inf infinitely
* follows -inf, and therefore that all finite and infinite values are in
* such a window frame.
*/
if (isinf(base) && isinf(offset))
{
if ((base > 0 && sub) || (base < 0 && !sub))
PG_RETURN_BOOL(true);
}
and the result is
id | f_float8 | first_value | last_value
----+-----------+-------------+------------
0 | -Infinity | 0 | 0
1 | -3 | 0 | 0
2 | -1 | 0 | 0
3 | 0 | 0 | 0
4 | 1.1 | 0 | 0
5 | 1.12 | 0 | 0
6 | 2 | 0 | 0
7 | 100 | 0 | 0
8 | Infinity | 0 | 8
9 | NaN | 9 | 9
(10 rows)
which looks about equally sensible. To me, the fact that the window
infinitely preceding -Inf includes -Inf makes more sense, but the
meaning of the window infinitely preceding +Inf is much less obvious,
and not really well-defined.
Regards,
Dean
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> if (isinf(base) && isinf(offset))
> {
> if ((base > 0 && sub) || (base < 0 && !sub))
> PG_RETURN_BOOL(true);
> }
Yeah, I'd experimented with more-or-less that logic before arriving at
my v2 patch. I didn't like the outcome that "inf both infinitely precedes
and infinitely follows itself". Still, it is nicely simple.
To make sense of this behavior, you have to argue that +/-inf are not
in any way concrete values, but represent some sort of infinite ranges;
then there could be some members of the class "inf" that infinitely
precede other members. I thought that was bending the mathematical
concept a bit too far. However, this isn't an area of math that I've
studied in any detail, so maybe it's a standard interpretation.
Still, I think the results my v2 patch gets make more sense than these.
regards, tom lane
I wrote:
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
>> if (isinf(base) && isinf(offset))
>> {
>> if ((base > 0 && sub) || (base < 0 && !sub))
>> PG_RETURN_BOOL(true);
>> }
> Yeah, I'd experimented with more-or-less that logic before arriving at
> my v2 patch. I didn't like the outcome that "inf both infinitely precedes
> and infinitely follows itself". Still, it is nicely simple.
I spent some more time thinking about this, and came to a couple
of conclusions.
First, let's take it as given that we should only special-case
situations where the sum would be computed as NaN. That destroys my
position that, for instance, -inf shouldn't be included in the range
that ends 'inf preceding' itself, because the normal calculation goes
through as -inf <= (-inf - inf) which yields TRUE without forming any
NaN. Although that conclusion seems weird at first glance, there
seems no way to poke a hole in it without rejecting the principle
that inf + inf = inf.
Second, if -inf is included in the range that ends 'inf preceding'
itself, symmetry dictates that it is also included in the range that
begins 'inf following' itself. In that case we'd be trying to compute
-inf >= (-inf + inf) which does involve a NaN, but this argument says
we should return TRUE.
The other three cases where we'd hit NaNs are likewise symmetric with
non-NaN cases that'd return TRUE. Hence, I'm forced to the conclusion
that you've got it right above. I might write the code a little
differently, but const-TRUE-for-NaN-cases seems like the right behavior.
So I withdraw my objection to defining it this way. Unless somebody
else weighs in, I'll commit it like that in a day or two.
regards, tom lane
I wrote:
> The other three cases where we'd hit NaNs are likewise symmetric with
> non-NaN cases that'd return TRUE. Hence, I'm forced to the conclusion
> that you've got it right above. I might write the code a little
> differently, but const-TRUE-for-NaN-cases seems like the right behavior.
> So I withdraw my objection to defining it this way. Unless somebody
> else weighs in, I'll commit it like that in a day or two.
Pushed, but I chickened out of back-patching. The improvement in what
happens for finite comparison values seems somewhat counterbalanced by
the possibility that someone might not like the definition we arrived
at for infinities. So, it's not quite an open-and-shut bug fix, so
I just put it in HEAD (for now anyway).
regards, tom lane
On Tue, 21 Jul 2020 at 03:06, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Pushed, but I chickened out of back-patching. The improvement in what > happens for finite comparison values seems somewhat counterbalanced by > the possibility that someone might not like the definition we arrived > at for infinities. So, it's not quite an open-and-shut bug fix, so > I just put it in HEAD (for now anyway). > Yeah, that seems fair enough, and it's quite an obscure corner-case that has gone unnoticed for quite some time. Regards, Dean