Re: BUG #19826: corr()/regr_r2() of constant input: NULL serially, 1 with partial aggregation

Venu K <venu2805k(at)gmail(dot)com> writes:
> v2 attached; make check passes on master.

I studied this a bit, and remained unhappy about one point: it's
not clear that float8_regr_combine() has the same behavior for
overflow cases as float8_regr_accum(). Maybe it does, but I don't
think readers should have to spend time convincing themselves of that.
I suggest adjusting it to use the same block of overflow-handling
logic that float8_regr_accum() has, as in v3 attached. Does that
seem sane to you?

> In practice a partial state that has seen an Inf or NaN already has
> Sxy = NaN, so I couldn't get a different result from SQL either way, but
> I don't think the combine function should depend on that.

Right, I'm with you on that. I suspect that the last few lines in
my v3,

+ if (isinf(Sxx))
+ Sxx = get_float8_nan();
+ if (isinf(Syy))
+ Syy = get_float8_nan();
+ if (isinf(Sxy))
+ Sxy = get_float8_nan();

are dead code for the same reason, but again that doesn't seem
like something to depend on, especially in a spot like this
that can't be at all performance-critical.

> One existing expected output changes: the direct float8_regr_combine()
> call in aggregates.sql passes two states with the same common X (7) but
> different means, so Sxx is now 400 rather than 6280. Such states can't
> come from real input; I can change the test input instead if you prefer.

Yeah, it seems clear that that test case is just made-up data,
probably put there just so that float8_regr_combine() shows as having
some coverage in our coverage tests. I wonder whether the new test
doesn't render it useless even for that. It might be better to change
it to exercise something your new test still doesn't reach, like an
overflow error. I didn't do anything about that in v3 though.

regards, tom lane