Repository navigation
Decorrelate subqueries whose grouping sets leave out the correlated column #25708
Description
Activity
take
Thanks for opening this issue @jayzhan211! The follow-up context from #25529 / #25519 makes complete sense.
Technical Analysis & Observations
-
Why the Current Guard is Overly Conservative:
- In fix: keep a correlated filter below an aggregate with a grouping set #25529, rejecting decorrelation whenever any grouping set omits the correlated column was necessary to prevent wrong results when expressions above
Aggregateread theNULL-filled column (e.g.HAVING i.k IS NULL) or evaluateGROUPING(i.k). - However, for predicates like
WHERE EXISTS (SELECT 1 FROM i WHERE i.k = o.k GROUP BY GROUPING SETS ((i.k), (i.j))), the correlated filteri.k = o.kpinsi.kto a single outer valueo.k. For non-empty grouping sets like(i.j), pulling upi.kdoes not alter row counts per outer row—it only changes the projectedi.kvalue in that grouping set fromNULLtoo.k. - Because
EXISTS(and subqueries where upper nodes do not read theNULL-filled column orGROUPING()) only checks row existence, decorrelating to a join is entirely sound and yields correct results.
- In fix: keep a correlated filter below an aggregate with a grouping set #25529, rejecting decorrelation whenever any grouping set omits the correlated column was necessary to prevent wrong results when expressions above
-
Empty Grouping Sets
()Must Remain Guarded:- As noted, empty grouping sets
()(such as in grand-total aggregations orCUBE/ROLLUPwith empty sets) yield 1 output row even when the subquery input is empty. A standard inner/left join cannot produce a row on an empty match without unmatched row indicator handling (the count bug mechanism). Thus, empty grouping sets()must stay rejected.
- As noted, empty grouping sets
-
Comparison of Solution Approaches:
- Option 1 (Aliasing correlated key per grouping set): Clean and preserves
NULLsemantics for expressions inspectingi.k, but requires rewriting grouping set expressions insideAggregate. - Option 2 (Refined guard check in
PullUpCorrelatedExpr): Checks whether any upper node in the subquery reads the correlated column orGROUPING()expression. If not (such as inEXISTSor standard projections that don't output the correlated column), decorrelation proceeds safely by expanding non-empty grouping sets.
- Option 1 (Aliasing correlated key per grouping set): Clean and preserves
Proposed Implementation Plan
-
Refine Guard in
PullUpCorrelatedExpr::f_up(datafusion/optimizer/src/decorrelate.rs):- Inspect
LogicalPlan::AggregateinPullUpCorrelatedExpr. - Ensure no empty grouping set
()exists ingroup_expr. - Validate whether parent projection/having nodes reference the correlated column or
GROUPING()expressions before decidingcan_pull_up.
- Inspect
-
Testing & Verification:
- Add unit tests in
decorrelate.rsand SQL logictests indatafusion/sqllogictest/test_files/subquery.sltcovering:EXISTSsubqueries withGROUPING SETS ((i.k), (i.j))(re-enabling decorrelation).- Subqueries with
HAVING i.k IS NULLorGROUPING(i.k)(verifying they safely fall back or remain guarded). - Empty grouping sets
GROUPING SETS ((), (i.k))ensuring correct rejection.
- Run standard lint suite (
cargo fmt --all,cargo clippy --all-targets --all-features -- -D warnings,./dev/rust_lint.sh) and subquery test suite.
- Add unit tests in
I would like to work on this issue!
take
-
Thanks @mohitgurav20 — looks like we grabbed this within the same minute, I'd taken it just before your comment landed, so I'll run with it. Appreciate the analysis though.
Thanks a lot for the warm reply @namanjain24-sudo! Absolutely no problem at all — timing was super close!
Since I had already spent time digging into
decorrelate.rsand setting up local test cases for Option 2, I'd really love to contribute to this fix. Would you be open to collaborating on this or working together on the PR (e.g. co-authoring)? Otherwise, I'd be more than happy to review your PR when it's ready!Either way, appreciate the awesome community spirit!
I'll take it solo for now since I've already got the context built up, but a review once the PR's up would be great — will ping you here.
Reacted by Mohit Guravyeahh suree !..
Reacted by namanjain24-sudoPR up: #25781. @mohitgurav20 would appreciate a review when you get a chance.
Reacted by Mohit Gurav- added 3 commits that reference this issue
on Sep 30, 2026
Is your feature request related to a problem or challenge?
Follow-up to #25529 (fix for #25519).
To stop wrong results, #25529 keeps a correlated filter below an aggregate with a grouping set unless every set already groups by each column the pull up would add. That guard is conservative: it also rejects non-empty sets that leave out the correlated column. On
mainthose queries were answered correctly, and with #25529 they fail to plan:Because the filter
i.k = o.kfixesi.kto one value per outer row, addingi.kto a non-empty set that lacks it leaves the rows of each outer row unchanged. Only two things change: the value ofi.kin those rows (NULL becomeso.k) and__grouping_id. So the old rewrite was wrong only when something above the aggregate reads the NULL-filled column orGROUPING(). One such case isHAVING i.k IS NULL, which is covered insubquery.slt.Describe the solution you'd like
Decorrelate these queries again without bringing back the wrong results. Two options:
ROLLUP/CUBEalways contain one) or when a node above the aggregate reads the NULL-filled column orGROUPING().Empty sets must stay rejected: they yield a row for outer rows that match nothing, and a join cannot produce that row.
Describe alternatives you've considered
Keep the current guard. The queries fail to plan instead of returning wrong results.
Additional context
The guard is in the
Aggregatearm ofPullUpCorrelatedExpr::f_up,datafusion/optimizer/src/decorrelate.rs. The known limitation is documented indatafusion/sqllogictest/test_files/subquery.slt.