fix(sql): avoid intermediate overflow in corr() denominator

[jovfer]

What

Follow-up to #7188, based on @RoySerbi's original contribution.

Fixes numerical-stability bugs in corr() where the denominator can overflow or underflow while computing the product of the two sums of squared deviations. The branch keeps the original contribution commits, merges current master, and adds compatibility fixes for the migrated fluent assertQuery(...) test style.

The fix applies to:

• CorrGroupByFunctionFactory
• AbstractBivariateStatWindowFunctionFactory.computeCorr
• AbstractBivariateStatWindowFunctionFactory.computeCorrWelford

Why

The Pearson denominator is sqrt(sumXX * sumYY), a product of two sums of squared deviations.

• Large-magnitude inputs (values near +/-1e153): each sum is finite (~1e306) but their product overflows to +Infinity, so sqrt(...) is infinite and the final division returns 0.0 instead of the true correlation.
• Small-magnitude inputs (values near +/-1e-150): each sum is finite (~1e-300) but their product underflows to 0.0, so sqrt(0.0) = 0 and the division returns NaN.

Example:

CREATE TABLE t(x DOUBLE, y DOUBLE);
INSERT INTO t VALUES (1e153, 1e153), (-1e153, -1e153);
SELECT corr(x, y) FROM t; -- should be 1.0

Both shapes exist in the window-function implementations as well: for partition-only and ordered running frames (computeCorrWelford) and for removable sliding frames (computeCorr).

How

• Prefer the single-rounding sqrt(a * b) denominator when the product is finite and non-zero, preserving existing bit-exact behavior for normal inputs.
• Fall back to sqrt(a) * sqrt(b) when the product would overflow to +Infinity or underflow to 0.0 while both factors are non-zero. A genuine zero factor (zero variance) still yields NaN.
• Clamp the final Pearson result to [-1, 1] to absorb the small rounding drift possible in the fallback path.
• Add a removable rows-frame regression that reaches the naive computeCorr path (the prior window tests only reached computeCorrWelford) and correct the test comment that misnamed the exercised paths.
• Add small-magnitude underflow regressions for the group-by and both window compute paths.

Trade-offs

• The split-sqrt fallback uses two sqrt roundings instead of one, so for perfectly-correlated inputs whose variance product over- or under-flows, the result can land ~1 ULP off the exact value. The [-1, 1] clamp absorbs the overshoot in the overflow tail (so ~1e153 reads exactly 1.0), but the underflow tail lands just below 1.0 -- corr() of two points near +/-1e-150 returns 0.9999999999999999 rather than 1.0. This is the accurate finite result for the fallback; the alternative (exact 1.0 via power-of-two rescaling) was not judged worth the added complexity and edge cases for a 1-ULP difference at these magnitudes.
• Normal-magnitude inputs are unaffected: they keep the single-rounding sqrt(product) path and its prior bit-exact results.

Out of scope / follow-up

• regr_r2() has the identical denominator exposure ((sumXY * sumXY) / (sumX * sumY), no over/underflow guard and no clamp), so it returns 0 at ~1e153 and can drift above 1.0. Left out here to keep this PR scoped to corr(); tracked in regr_r2() denominator overflows/underflows like corr() did #7328.

Tests

mvn -pl core -DskipTests test-compile
mvn -pl core -Dtest=io.questdb.test.griffin.engine.functions.groupby.CorrGroupByFunctionFactoryTest,io.questdb.test.griffin.engine.window.WindowFunctionTest#testCorrLargeMagnitudeOverflow+testCorrSmallMagnitudeUnderflow surefire:test

Notes

Original PR: #7188

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 89ed672d-d670-496c-bc0e-951d00a5b1e7

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch sm_corr_stability_7188

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[bluestreak01]

Numerical fix itself is correct (verified: overflow prod→+Inf, fallback sqrt(a)*sqrt(b) recovers the denominator, r=1.000…2 → clamp → 1.0), and the out-of-diff callsites are clean. Two things should change before merge.

1. The naive computeCorr path is the modified code with zero coverage — and the test comment claims the opposite

WindowFunctionTest.testCorrLargeMagnitudeOverflow says it "Exercises both the naive (partition-only) and Welford (order-by running) code paths." Both claims are wrong:

• corr(y,x) over (partition by i) → BivarStatOverPartitionFunction.preparePass2 → computeCorrWelford
• corr(y,x) over (partition by i order by ts) → BivarStatOverUnboundedPartitionRowsFrameFunction → computeCorrWelford

Both routes use Welford. The naive computeCorr (reached only from removable rows between N preceding… / range … preceding frames) is exercised by neither. This matches the coverage bot exactly: AbstractBivariateStatWindowFunctionFactory.java 17/20, the 3 uncovered lines being precisely computeCorr's new fallback + two clamp returns.

Please add a removable-frame regression, e.g. corr(y,x) over (order by ts rows between 1 preceding and current row) (and/or a range … preceding frame) over the ±1e153 data, and correct the comment to name the real paths (both Welford).

2. The fix is asymmetric — the underflow tail is left degraded

The split-sqrt fallback only fires on overflow (!Double.isFinite(prod)). The mirror-image case — tiny non-zero variances whose product underflows to 0.0 — stays finite, so the code takes sqrt(0.0)=0 and returns NaN, although sqrt(a)*sqrt(b) recovers the value. Verified: for sumX=sumY=sumXY=1e-300 (two points, true corr=1.0) the code returns NaN; the split form returns 1.0. Reachable with inputs near 1e-150 — no more exotic than the 1e153 case the PR targets. Either widen the condition to also use the split form when prod==0.0 && sumX!=0 && sumY!=0, or add a comment explicitly scoping the small-magnitude tail out.

Minor

• PR has no labels — add SQL and Bug.
• No Fixes #NNN in the body (only links PR #7188); add the tracking issue if one exists.

The count<=0→count<=1 change, the dropped sumX==0||sumY==0 short-circuit, and the assertMemoryLeak+.noLeakCheck() usage all verified equivalent/correct — no action needed there.

[jovfer]

L3 Review

Re-reviewed the current diff from scratch and validated it against a fresh build: CorrGroupByFunctionFactoryTest (16/16) and WindowFunctionTest#testCorr* (9/9) pass, so the exact asserted values (1.0, -1.0, 0.9999999999999999) reproduce and no existing corr test regresses under the new clamp. Both concerns from the earlier review round are resolved in the current diff — the naive computeCorr path now has a removable-rows-frame regression with corrected routing comments, and the underflow asymmetry is fixed by widening splitDenom to the prod == 0.0 case with underflow tests.

No blocking issues. The numerical fix is correct on every path I traced: the split-sqrt fallback recovers the denominator in both the overflow (~1e153, product → +Inf) and underflow (~1e-150, product → 0.0) regimes; the split itself can neither overflow (sqrt(MAX)*sqrt(MAX)=MAX) nor underflow to 0 (sqrt(MIN_VALUE)≈2.2e-162, product ≥ MIN_VALUE > 0); NaN passes through the clamp correctly; and all 12 callsites of the changed functions store the returned double verbatim (covar paths are byte-identical, the regression-function subclasses share only the map layout). The count <= 0 → count <= 1 GroupBy change is behaviour-preserving: a 1-row group already returned NaN via the old sumX == 0 || sumY == 0 guard, since the Welford sums are exactly 0 at count == 1.

Moderate

Extract the triplicated denominator + clamp logic. The same ~12-line split-sqrt + clamp block now lives in three places:

• CorrGroupByFunctionFactory.java:117-134 (getDouble)
• AbstractBivariateStatWindowFunctionFactory.java:87-102 (computeCorr)
• AbstractBivariateStatWindowFunctionFactory.java:117-131 (computeCorrWelford)

This is subtle floating-point logic with a bit-exactness contract that must stay synchronised across all copies — a future tweak to one clamp/threshold has three edit sites that must remain identical. The // See computeCorr() for the rationale... cross-reference comments (lines 116, 123) are prose-DRY standing in for code-DRY. The two window statics can delegate to a private static today with zero friction; the GroupBy copy belongs in a std util (e.g. Numbers.pearsonFromSums(sumXY, a, b)). The scoped-out follow-up #7328 (regr_r2, mathematically corr²) will need this exact logic — a fourth copy — so extracting now tees that up cleanly. Not blocking, but it's the one item with real maintenance cost.

Minor

1. Underflow comment says the prior result was NaN; for the GroupBy path it was +Infinity. CorrGroupByFunctionFactoryTest.java:292 ("sqrt(0.0) made the denominator 0.0 and corr() returned NaN"). On master the GroupBy path is sumXY / Math.sqrt(0.0) with sumXY = 2e-300 > 0, i.e. +Infinity, not NaN. "NaN" is accurate only for the window paths (which had the denom == 0.0 ? NaN guard). The PR body's "the division returns NaN" has the same imprecision. The regression is real either way; just fix the stated prior symptom.

2. The description overstates "normal inputs unaffected / bit-exact". The [-1, 1] clamp runs on every path, not only the fallback, so it does change normal-magnitude results that previously overshot the bound (master can return e.g. -1.0000000000140592 for perfectly-correlated finite data; this PR returns -1.0). That's a correctness improvement, but only the denominator is bit-exact, not the returned r. Reword so the clamp isn't framed as fallback-only.

3. Double.isFinite vs the codebase's Numbers.isFinite. Three new sites use Double.isFinite(prod) (CorrGroupByFunctionFactory.java:118, AbstractBivariateStatWindowFunctionFactory.java:88,118); every other finite check in these files uses Numbers.isFinite (semantically identical). Consistency nit.

4. boolean splitDenom lacks the is/has prefix. Same-file neighbours are all prefixed (isCorrelation, isFinitePair, isSample); isSplitDenom would match. Disappears if the helper is extracted.

5. GroupBy getDouble omits the >= 0 clamp the window methods apply. computeCorr/computeCorrWelford clamp the variance factors to >= 0 before the product; getDouble does not. Not currently reachable (the Welford increment (x-oldMean)*(x-newMean) is provably non-negative and the Chan merge only adds non-negative terms), so it's a parity/robustness gap rather than a live bug — worth either a matching one-line clamp or a comment stating why the GroupBy sums can't go negative.

6. No extreme-magnitude RANGE-frame coverage. The new tests reach the naive computeCorr only through ROWS frames + whole-partition. The RANGE removable frames (BivarStatOverRangeFrameFunction, BivarStatOverPartitionRangeFrameFunction) feed the same computeResultNaive → computeCorr, so the numeric fix is covered, but the RANGE ring-buffer accumulation is only tested at normal magnitude (testCorrOverPartitionRangeFrame). A single ±1e153 RANGE case would close it. (The coverage bot's 3 uncovered window lines are unrelated — they're merged-in setMemoryTracker overrides on the no-partition RANGE class, not the corr fix.)

7. Title is implementation-focused. "avoid intermediate overflow in corr() denominator" describes the mechanism; an impact-focused phrasing (e.g. "fix corr() returning 0 or NaN for very large or very small values") reads better in release notes. Borderline.

Optional: the splitDenom underflow trigger is prod == 0.0; prod < Double.MIN_NORMAL would also route the subnormal-product band (where sqrt(prod) has already lost mantissa bits) through the more-accurate split path. Effectively unreachable (needs data magnitudes ~1e-80) and the clamp hides it for perfect correlation, so genuinely optional.

Considered but not issues

• regr_r2 carries the identical overflow/underflow defect — real, but intentionally scoped out and disclosed, tracked in regr_r2() denominator overflows/underflows like corr() did #7328. Extracting the shared helper above would make that follow-up trivial.
• "GroupBy could return garbage from negative variance" — dismissed: the Welford sums are provably non-negative; not reachable (kept only as the softened parity nit Impliment the http://reactive-streams.org/ Speck #5).
• "Clamp masks degenerate zero-variance as a plausible 1.0 on the naive path" — no triggering input is constructible; cancellation drives cXX/cYY negative → clamped to 0 → NaN in both old and new code. Pre-existing naive-formula limitation, not introduced here.
• Performance — negligible: the common path adds ~3-4 not-taken compares + one isFinite intrinsic behind a perfectly-predicted branch, against per-row map lookups and native ring-buffer I/O; prod is reused, not recomputed; the chosen shape is the cheapest that keeps normal-path bit-exactness.

Summary

No blocking issues; the blast radius is fully contained (all 12 callsites verified, no callers outside the two files, covar/regression paths untouched) and the tests pass against a real build. The only substantive item is the triplicated Pearson logic, which the imminent regr_r2 work makes worth extracting now; everything else is comment/description-accuracy and consistency nits. The disclosed tradeoff (underflow tail reads 0.9999999999999999, 1 ULP low, because the split uses two sqrt roundings and the clamp only bounds the > 1 side) is stated plainly in the PR body.

[jovfer]

addressed the moderate above about code duplications.

[jovfer]

/azp run macwin

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mtopolnik]

[PR Coverage check]

😍 pass : 15 / 15 (100.00%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/griffin/engine/functions/groupby/CorrGroupByFunctionFactory.java	2	2	100.00%
🔵	io/questdb/std/Numbers.java	11	11	100.00%
🔵	io/questdb/griffin/engine/functions/window/AbstractBivariateStatWindowFunctionFactory.java	2	2	100.00%

[bluestreak01]

Approved (level 3 review). Numerical fix verified correct on every path — overflow (+Inf product → split-sqrt) and underflow (0 product → split-sqrt) both recover the denominator; genuine zero variance and NaN propagate correctly; the [-1,1] clamp is mathematically sound. Blast radius fully contained: all 3 new callers + 6 internal computeCorr* callsites SAFE, covar paths byte-identical, corrFromSums is a new symbol with no out-of-diff callers. Both prior-round blocking concerns and the triplication moderate are resolved. Only minor doc/style/consistency nits remain (NumbersTest alphabetical ordering, prior-symptom comment says NaN where GroupBy master returned +Infinity, Double.isFinite vs Numbers.isFinite, splitDenom naming, clamp framing in PR body).