[SQL] Support for COVAR_* and REGR_* aggregate functions - #6812
Conversation
mythical-fred
left a comment
There was a problem hiding this comment.
Clean addition of COVAR_POP / COVAR_SAMP / REGR_SXX / REGR_SYY (REGR_COUNT is handled by the existing SqlCountAggFunction path).
Nice touches:
guardedSumreplaces Calcite's internal 2-argSUM(x, guard)with aCASE WHEN guard IS NOT NULL THEN x ENDwrapped inSUM. That's the right lowering — Calcite'sSUM(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 withMUL_WEIGHTand the newQuadSemigroup— symmetric withTripleSemigroupand correct (each field usesDefaultOptSemigroup). - Correct NULL semantics:
bothNonNullmask 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 inCovarTests. - Bonus: the
doVariancecleanup drops a redundantsq.cast(typedZero.getType())after the MUL that was already producingtypedZero.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
left a comment
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Nit: typo — an a CASE statment should read a CASE statement.
ryzhyk
left a comment
There was a problem hiding this comment.
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.
|
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 |
|
Filed fixes for Calcite: apache/calcite#5164 and apache/calcite#5163 |
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
|
I have merged two bug fixes in Calcite and documented the issues reported better. This is again ready for review. |
mythical-fred
left a comment
There was a problem hiding this comment.
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 twocount(*)toCOUNT(*)for casing consistency. Improves the docs.CovarTests.java: added 22 lines — aWHERE FALSEempty-group case intestCovarGlobal(COVAR_POP→NULL,REGR_COUNT→0). 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.
These are all linear aggregates (when not using floating point).
Checklist