Horton

Thread

Commits

  1. Fix some corner cases for window ranges with infinite offsets.

  1. Wrong results from in_range() tests with infinite offset

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-07-16T18:58:13Z

    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

  2. Re: Wrong results from in_range() tests with infinite offset

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-07-16T21:50:16Z

    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

  3. Re: Wrong results from in_range() tests with infinite offset

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2020-07-16T23:47:14Z

    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

  4. Re: Wrong results from in_range() tests with infinite offset

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-07-17T00:59:57Z

    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

  5. Re: Wrong results from in_range() tests with infinite offset

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2020-07-18T08:16:39Z

    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

  6. Re: Wrong results from in_range() tests with infinite offset

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-07-18T14:06:13Z

    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

  7. Re: Wrong results from in_range() tests with infinite offset

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-07-18T21:28:52Z

    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

  8. Re: Wrong results from in_range() tests with infinite offset

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-07-21T02:06:32Z

    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

  9. Re: Wrong results from in_range() tests with infinite offset

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2020-07-21T08:06:09Z

    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