mohitgurav20 commented on issue #25708:
URL: https://github.com/apache/datafusion/issues/25708#issuecomment-5818635726
Thanks for opening this issue @jayzhan211! The follow-up context from #25529
/ #25519 makes complete sense.
### Technical Analysis & Observations
1. **Why the Current Guard is Overly Conservative**:
- In #25529, rejecting decorrelation whenever any grouping set omits the
correlated column was necessary to prevent wrong results when expressions above
`Aggregate` read the `NULL`-filled column (e.g. `HAVING i.k IS NULL`) or
evaluate `GROUPING(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 filter `i.k = o.k`
pins `i.k` to a single outer value `o.k`. For non-empty grouping sets like
`(i.j)`, pulling up `i.k` does not alter row counts per outer row—it only
changes the projected `i.k` value in that grouping set from `NULL` to `o.k`.
- Because `EXISTS` (and subqueries where upper nodes do not read the
`NULL`-filled column or `GROUPING()`) only checks row existence, decorrelating
to a join is entirely sound and yields correct results.
2. **Empty Grouping Sets `()` Must Remain Guarded**:
- As noted, empty grouping sets `()` (such as in grand-total aggregations
or `CUBE`/`ROLLUP` with 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.
3. **Comparison of Solution Approaches**:
- **Option 1 (Aliasing correlated key per grouping set)**: Clean and
preserves `NULL` semantics for expressions inspecting `i.k`, but requires
rewriting grouping set expressions inside `Aggregate`.
- **Option 2 (Refined guard check in `PullUpCorrelatedExpr`)**: Checks
whether any upper node in the subquery reads the correlated column or
`GROUPING()` expression. If not (such as in `EXISTS` or standard projections
that don't output the correlated column), decorrelation proceeds safely by
expanding non-empty grouping sets.
### Proposed Implementation Plan
1. **Refine Guard in `PullUpCorrelatedExpr::f_up`**
(`datafusion/optimizer/src/decorrelate.rs`):
- Inspect `LogicalPlan::Aggregate` in `PullUpCorrelatedExpr`.
- Ensure no empty grouping set `()` exists in `group_expr`.
- Validate whether parent projection/having nodes reference the
correlated column or `GROUPING()` expressions before deciding `can_pull_up`.
2. **Testing & Verification**:
- Add unit tests in `decorrelate.rs` and SQL logictests in
`datafusion/sqllogictest/test_files/subquery.slt` covering:
- `EXISTS` subqueries with `GROUPING SETS ((i.k), (i.j))` (re-enabling
decorrelation).
- Subqueries with `HAVING i.k IS NULL` or `GROUPING(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.
I would like to work on this issue!
take
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
