Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jwBlo-0007a8-Co for pgsql-hackers@arkaria.postgresql.org; Thu, 16 Jul 2020 21:50:24 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1jwBlm-0002he-Er for pgsql-hackers@arkaria.postgresql.org; Thu, 16 Jul 2020 21:50:22 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jwBlm-0002hX-4H for pgsql-hackers@lists.postgresql.org; Thu, 16 Jul 2020 21:50:22 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jwBlj-0003GJ-NS for pgsql-hackers@lists.postgresql.org; Thu, 16 Jul 2020 21:50:21 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 06GLoGHD3464258; Thu, 16 Jul 2020 17:50:16 -0400 From: Tom Lane To: pgsql-hackers@lists.postgresql.org cc: Dean Rasheed Subject: Re: Wrong results from in_range() tests with infinite offset In-reply-to: <3393130.1594925893@sss.pgh.pa.us> References: <3393130.1594925893@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Thu, 16 Jul 2020 14:58:13 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <3464223.1594936170.0@sss.pgh.pa.us> Date: Thu, 16 Jul 2020 17:50:16 -0400 Message-ID: <3464257.1594936216@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <3464223.1594936170.1@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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="fix-infinite-in_range-cases-2.patch"; charset="us-ascii" Content-ID: <3464223.1594936170.2@sss.pgh.pa.us> Content-Description: fix-infinite-in_range-cases-2.patch Content-Transfer-Encoding: quoted-printable 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 <=3D base - inf: true if base is +inf and val isn't */ + PG_RETURN_BOOL(base > 0 && (!isinf(val) || val < 0)); + } + else + { + /* val >=3D base - inf: true for all val & either sign of base */ + PG_RETURN_BOOL(true); + } + } + else + { + if (less) + { + /* val <=3D base + inf: true for all val & either sign of base */ + PG_RETURN_BOOL(true); + } + else + { + /* val >=3D 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, an= d * produce a suitably signed infinity, which will compare properly again= st * 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 <=3D base - inf: true if base is +inf and val isn't */ + PG_RETURN_BOOL(base > 0 && (!isinf(val) || val < 0)); + } + else + { + /* val >=3D base - inf: true for all val & either sign of base */ + PG_RETURN_BOOL(true); + } + } + else + { + if (less) + { + /* val <=3D base + inf: true for all val & either sign of base */ + PG_RETURN_BOOL(true); + } + else + { + /* val >=3D 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, an= d * produce a suitably signed infinity, which will compare properly again= st * val whether or not that's infinity. */ diff --git a/src/test/regress/expected/window.out b/src/test/regress/expec= ted/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 disallowe= d = @@ -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 disallowe= d = ------- =_aaaaaaaaaa0--