Hi, > On May 16, 2026, at 17:39, Dean Rasheed <[email protected]> wrote: > >> corr() already has a stabilized calculation for the same Sxx * Syy >> denominator scale. This patch factors that into a helper and lets >> regr_r2() use it as a fallback when one of its direct products has >> rounded to zero or infinity. Otherwise, regr_r2() keeps the existing >> direct formula. > > The comments need work -- in particular float8_regr_r2() needs a > comment explaining the new overflow/underflow checks, similar to the > comment in float8_corr(). In fact, doing that, I think it's preferable > to just keep this change local to float8_regr_r2(), rather than > refactoring into a helper function for just a few lines of code. > > This new check in float8_regr_r2(): > > + if (Sxy == 0 && !isnan(Sxx) && !isnan(Syy)) > + PG_RETURN_FLOAT8(0.0); > > seems pointless. It's optimising for a special case that will very > rarely occur in practice, and which is handled fine by the general > code. We don't want to slow down the common code path for such rare > special cases. > > I noticed that this new overflow test case: > > +SELECT regr_r2(1e154::float8 * g, 1e154::float8 * g) > + FROM generate_series(1, 2) g; > + regr_r2 > +--------- > + 1 > +(1 row) > > only produces 1 because it's run with a reduced extra_float_digits > value. I think it's better to use the test values "1e100 + g * 1e95", > which still trigger the overflow on HEAD, but more reliably produce 1, > regardless of the extra_float_digits setting, making it less likely > that there will be variations between platforms. That's also more > consistent with the other nearby test cases.
Thanks for the thorough review and feedback — I learned a lot from it! > Attached is a v2 patch with those changes, plus a little more tidying > up of the regression tests. v2 LGTM. Thanks for the updates and test cleanup. -- Best regards, Chengpeng Yan
