jayzhan211 commented on code in PR #25781:
URL: https://github.com/apache/datafusion/pull/25781#discussion_r4271517379
##########
datafusion/optimizer/src/decorrelate.rs:
##########
@@ -662,18 +727,37 @@ impl PullUpCorrelatedExpr {
.any(|expr| matches!(expr, Expr::Column(c) if c == *col))
});
if required_cols.is_empty() {
- return true;
+ return GroupingSetsPullUp::NoColumnsToAdd;
}
- grouping_sets.iter().all(|grouping_set| match grouping_set {
- GroupingSet::Rollup(_) | GroupingSet::Cube(_) => false,
- GroupingSet::GroupingSets(sets) => sets.iter().all(|set| {
- required_cols.iter().all(|col| {
- set.iter()
- .any(|expr| matches!(expr, Expr::Column(c) if c ==
*col))
- })
- }),
- })
+ let already_covered =
+ grouping_sets.iter().all(|grouping_set| match grouping_set {
+ GroupingSet::Rollup(_) | GroupingSet::Cube(_) => false,
+ GroupingSet::GroupingSets(sets) => sets.iter().all(|set| {
+ required_cols.iter().all(|col| {
+ set.iter()
+ .any(|expr| matches!(expr, Expr::Column(c) if c ==
*col))
+ })
+ }),
+ });
+ if already_covered {
+ return GroupingSetsPullUp::NoColumnsToAdd;
+ }
+
+ let has_empty_set = grouping_sets.iter().any(|grouping_set| match
grouping_set {
+ GroupingSet::Rollup(_) | GroupingSet::Cube(_) => true,
+ GroupingSet::GroupingSets(sets) => sets.iter().any(|set|
set.is_empty()),
+ });
+ let reads_grouping_fn =
aggregate.aggr_expr.iter().any(is_grouping_call);
Review Comment:
`is_grouping_call` never fires: `ResolveGroupingFunction` already rewrote
`GROUPING` into `__grouping_id` bit tests in a projection above the
`Aggregate`, so `aggr_expr` holds no `grouping` call. When `k` is in no set,
nothing above reads `k`, so this returns `SafeToExtend`. Appending `k` makes
the group list `[j, k, x]`, and the stale `__grouping_id & 2 >> 1` now reads
`k`'s bit. Main fails to plan both queries below; this branch returns wrong
rows.
Repro:
```sql
CREATE TABLE gs_inner3(k INT, j INT, x INT) AS VALUES (1, 10, 100), (NULL,
20, 200), (5, 30, 300), (2, 40, 400);
-- expected 1 t, 2 t, 4 f, 5 t, NULL f; returns false for every row
SELECT gs_outer.k, EXISTS (SELECT 1 FROM gs_inner3 WHERE gs_inner3.k =
gs_outer.k GROUP BY GROUPING SETS ((gs_inner3.j), (gs_inner3.x)) HAVING
GROUPING(gs_inner3.j) = 1) FROM gs_outer ORDER BY gs_outer.k;
-- expected 1, 1, 0, 1, 0; returns 0 for every row
SELECT gs_outer.k, (SELECT count(*) FROM (SELECT GROUPING(gs_inner3.j) AS g
FROM gs_inner3 WHERE gs_inner3.k = gs_outer.k GROUP BY GROUPING SETS
((gs_inner3.j), (gs_inner3.x))) t WHERE t.g = 1) FROM gs_outer ORDER BY
gs_outer.k;
```
Fix (and delete `is_grouping_call`). With this, full slt passes and the
`((k), (j))` EXISTS case still decorrelates:
```diff
- let reads_grouping_fn =
aggregate.aggr_expr.iter().any(is_grouping_call);
+ // `GROUPING`/`GROUPING_ID` are resolved to bit tests on
`__grouping_id`
+ // above the aggregate; adding a column shifts those bits.
+ let reads_grouping_id = self
+ .column_refs_above_aggregate
+ .iter()
+ .any(|col| col.name == Aggregate::INTERNAL_GROUPING_ID);
let read_above = required_cols
.iter()
.any(|col| self.column_refs_above_aggregate.contains(*col));
- if has_empty_set || reads_grouping_fn || read_above {
+ if has_empty_set || reads_grouping_id || read_above {
```
--
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]
