From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Cc: Dean Rasheed <dean.a.rasheed@gmail.com>
Subject: Re: Wrong results from in_range() tests with infinite offset
Date: Thu, 16 Jul 2020 17:50:16 -0400
Message-ID: <3464257.1594936216@sss.pgh.pa.us> (raw)
In-Reply-To: <3393130.1594925893@sss.pgh.pa.us>
References: <3393130.1594925893@sss.pgh.pa.us>
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
Attachments:fix-infinite-in_range-cases-2.patch (text/x-diff, 8.5K) downloaddiff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.cindex 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.outindex 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.sqlindex 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
=
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org, dean.a.rasheed@gmail.com
Subject: Re: Wrong results from in_range() tests with infinite offset
In-Reply-To: <3464257.1594936216@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox