Horton

Thread

Commits

  1. Sync behavior of var_samp and stddev_samp for single NaN inputs.

  2. Fix behavior of float aggregates for single Inf or NaN inputs.

  1. Definitional issue: stddev_pop (and related) for 1 input

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-11T18:06:06Z

    Before v12, stddev_pop() had the following behavior with just a
single input value:

regression=# SELECT stddev_pop('42'::float8);
 stddev_pop 
------------
          0
(1 row)

regression=# SELECT stddev_pop('inf'::float8);
 stddev_pop 
------------
        NaN
(1 row)

regression=# SELECT stddev_pop('nan'::float8);
 stddev_pop 
------------
        NaN
(1 row)

As of v12, though, all three cases produce 0.  I am not sure what
to think about that with respect to an infinity input, but I'm
quite sure I don't like it for NaN input.

It looks like the culprit is the introduction of the "Youngs-Cramer"
algorithm in float8_accum: nothing is done to Sxx at the first iteration,
even if the input is inf or NaN.  I'd be inclined to force Sxx to NaN
when the first input is NaN, and perhaps also when it's Inf.
Alternatively we could clean up in the finalization routine by noting
that Sx is Inf/NaN, but that seems messier.  Thoughts?

(I came across this by noting that the results don't agree with
numeric accumulation, which isn't using Youngs-Cramer.)

			regards, tom lane

  2. Re: Definitional issue: stddev_pop (and related) for 1 input

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-12T19:53:38Z

    I wrote:
> Before v12, stddev_pop() had the following behavior with just a
> single input value:
> ...
> As of v12, though, all three cases produce 0.  I am not sure what
> to think about that with respect to an infinity input, but I'm
> quite sure I don't like it for NaN input.

While I'm still not sure whether there's an academic argument that
zero is a reasonable stddev value for a single input that is Inf,
it seems to me that backwards compatibility is a sufficient reason
for going back to producing NaN for that.

Hence, attached are some proposed patches.  0001 just adds test
cases demonstrating the current behavior; then 0002 makes the
proposed code change.  It's easy to check that the test case results
after 0002 match what v11 produces.

0003 deals with a different problem which I noted in [1]: the numeric
variants of var_samp and stddev_samp also do the wrong thing for a
single special input.  Their disease is that they produce NaN for a
single NaN input, where it seems more sensible to produce NULL.
At least, NULL is what we get for the same case with the float
aggregates, so we have to change one or the other set of functions
if we want consistency.

I propose back-patching 0001/0002 as far as v12, since the failure
to match the old outputs seems like a pretty clear bug/regression.
However, I'd be content to apply 0003 only to HEAD.  That misbehavior
is very ancient, and the lack of complaints suggests that few people
really care about this fine point.

			regards, tom lane

[1] https://www.postgresql.org/message-id/606717.1591924582%40sss.pgh.pa.us

  3. Re: Definitional issue: stddev_pop (and related) for 1 input

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2020-06-13T10:06:12Z

    On Fri, 12 Jun 2020 at 20:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > Before v12, stddev_pop() had the following behavior with just a
> > single input value:
> > ...
> > As of v12, though, all three cases produce 0.  I am not sure what
> > to think about that with respect to an infinity input, but I'm
> > quite sure I don't like it for NaN input.
>
> While I'm still not sure whether there's an academic argument that
> zero is a reasonable stddev value for a single input that is Inf,
> it seems to me that backwards compatibility is a sufficient reason
> for going back to producing NaN for that.

Yeah, it was an oversight, not considering that case. I think the
academic argument could equally well be made that the result should be
NaN if any input is Inf or NaN, even if there's only one input (it's
effectively "Inf - Inf" or "NaN - NaN"), so I agree that backwards
compatibility clinches it.

> Hence, attached are some proposed patches.  0001 just adds test
> cases demonstrating the current behavior; then 0002 makes the
> proposed code change.  It's easy to check that the test case results
> after 0002 match what v11 produces.

Those both look reasonable to me.

> 0003 deals with a different problem which I noted in [1]: the numeric
> variants of var_samp and stddev_samp also do the wrong thing for a
> single special input.  Their disease is that they produce NaN for a
> single NaN input, where it seems more sensible to produce NULL.
> At least, NULL is what we get for the same case with the float
> aggregates, so we have to change one or the other set of functions
> if we want consistency.

Hmm, yeah it's a bit annoying that they're different. NULL seems like
the more logical result -- sample standard deviation isn't defined for
a sample of 1, so why should it be different if that one value is NaN.

The patch looks reasonable, except I wonder if all compilers are smart
enough to realise that totCount is always initialised.

> I propose back-patching 0001/0002 as far as v12, since the failure
> to match the old outputs seems like a pretty clear bug/regression.
> However, I'd be content to apply 0003 only to HEAD.  That misbehavior
> is very ancient, and the lack of complaints suggests that few people
> really care about this fine point.

Makes sense.

Regards,
Dean

  4. Re: Definitional issue: stddev_pop (and related) for 1 input

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-13T15:56:00Z

    Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> The patch looks reasonable, except I wonder if all compilers are smart
> enough to realise that totCount is always initialised.

I think they should be, since that if-block ends with a return;
the only way to get to the use of totCount is for both parts of the
first if-condition to be executed.

In any case, we do have an effective policy of ignoring
uninitialized-variable warnings from very old/stupid compilers.
locust and prairiedog, which I think use the same ancient gcc
version, emit a couple dozen such warnings.  If they are the only
ones that complain about this new code, I'll not worry.

Thanks for looking at the patch!

			regards, tom lane