Skip to content

[SQL] Support for COVAR_* and REGR_* aggregate functions - #6812

Merged
mihaibudiu merged 1 commit into
feldera:mainfrom
mihaibudiu:covar
Aug 9, 2026
Merged

[SQL] Support for COVAR_* and REGR_* aggregate functions#6812
mihaibudiu merged 1 commit into
feldera:mainfrom
mihaibudiu:covar

Conversation

@mihaibudiu

Copy link
Copy Markdown
Contributor

These are all linear aggregates (when not using floating point).

Checklist

  • Unit tests added/updated
  • Documentation updated

@mythical-fred mythical-fred left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean addition of COVAR_POP / COVAR_SAMP / REGR_SXX / REGR_SYY (REGR_COUNT is handled by the existing SqlCountAggFunction path).

Nice touches:

  • guardedSum replaces Calcite's internal 2-arg SUM(x, guard) with a CASE WHEN guard IS NOT NULL THEN x END wrapped in SUM. That's the right lowering — Calcite's SUM(x, y) is a private convention that never really worked here.
  • Linear implementation when args are non-FP: 5-tuple accumulator (sumAB, sumA, sumB, n, weight-1), computed in post via covariance(). Non-linear fallback for FP uses a 4-tuple with MUL_WEIGHT and the new QuadSemigroup — symmetric with TripleSemigroup and correct (each field uses DefaultOptSemigroup).
  • Correct NULL semantics: bothNonNull mask ignores pairs where either side is NULL; COVAR_SAMP with n=1 yields DIV_NULL(_, 0)=NULL; REGR_COUNT=0 group returns NULL for cp/sxx/syy and 0 for rc. All tested in CovarTests.
  • Bonus: the doVariance cleanup drops a redundant sq.cast(typedZero.getType()) after the MUL that was already producing typedZero.getType(). Silent simplification.

Tests (CovarTests + testRegrValue + testWindowCovariance) cover global, grouped, filtered, empty-partition, mixed-NULL, integer, and double paths. Postgres cross-check is called out in the class doc.

LGTM.

@mythical-fred mythical-fred left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving on the new commit: the covariance family (COVAR_POP/COVAR_SAMP/REGR_SXX/REGR_SYY) implementation looks solid — linear-path decomposition into (Sum(a*b), Sum(a), Sum(b), Count) with bothNonNull masking and DIV_NULL in the post step correctly yields SQL NULL semantics (empty group → NULL for all except REGR_COUNT which is 0; single row → COVAR_SAMP NULL). CovarTests exercises grouped/global/FILTER/DOUBLE paths and the empty-set edge case. New QuadSemigroup mirrors the existing pair/tri variants — clean.

The guardedSum refactor in expandCovariance (SUM(CASE WHEN guard IS NOT NULL THEN value END)) is the right fix for REGR_SLOPE/INTERCEPT/R2 expansions that Calcite otherwise emits as internal 2-arg SUM(x, guard).

One tiny typo in the new comment (inline).

final SqlNode sum1;
final SqlNode count;
// Calcite's version of this expansion generates internal 2-argument
// calls SUM(x, guard); we generate an a CASE statment.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: typo — an a CASE statment should read a CASE statement.

@ryzhyk ryzhyk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch contains some adversarial tests that fail along with fixes for two of the issues that cause the failures. These two seem like real bugs. The other two issues claude found -- I'm less clear, will leave it to you to decide how much of this to adopt.

@mihaibudiu
mihaibudiu marked this pull request as draft August 6, 2026 20:21
@mihaibudiu

Copy link
Copy Markdown
Contributor Author

Marking as draft until two Calcite bugs are fixed: https://issues.apache.org/jira/browse/CALCITE-7696 and https://issues.apache.org/jira/browse/CALCITE-7695

@mihaibudiu mihaibudiu added the Calcite Fix required in Calcite label Aug 6, 2026
@mihaibudiu

mihaibudiu commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Filed fixes for Calcite: apache/calcite#5164 and apache/calcite#5163

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
@mihaibudiu

Copy link
Copy Markdown
Contributor Author

I have merged two bug fixes in Calcite and documented the issues reported better. This is again ready for review.

@mihaibudiu
mihaibudiu marked this pull request as ready for review August 8, 2026 22:55
@mihaibudiu
mihaibudiu requested a review from ryzhyk August 8, 2026 22:59
@mihaibudiu
mihaibudiu added this pull request to the merge queue Aug 9, 2026

@mythical-fred mythical-fred left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased tip 4bf3d61e71 over v0.330.0. Delta vs prior approved tip abe6e08d:

  • docs/sql/aggregates.md: expanded the "widen your type" note to call out quadratic-blowup aggregates (VAR_*, REGR_*), and lowercased two count(*) to COUNT(*) for casing consistency. Improves the docs.
  • CovarTests.java: added 22 lines — a WHERE FALSE empty-group case in testCovarGlobal (COVAR_POPNULL, REGR_COUNT0). Nice edge-case coverage.

Core aggregator logic (linear 5-tuple / non-linear 4-tuple + QuadSemigroup), guardedSum rewrite, and NULL semantics all identical to the approved version. Re-approving on the current base.

Merged via the queue into feldera:main with commit 1333091 Aug 9, 2026
1 check passed
@mihaibudiu
mihaibudiu deleted the covar branch August 9, 2026 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Calcite Fix required in Calcite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants