jayzhan211 commented on code in PR #25529:
URL: https://github.com/apache/datafusion/pull/25529#discussion_r4056658177
##########
datafusion/sqllogictest/test_files/subquery.slt:
##########
@@ -2744,3 +2744,93 @@ b 400
statement ok
DROP TABLE metrics;
+
+# Regression test for #25519: a correlated filter that sits below an aggregate
+# with a grouping set must not be pulled above it. The pull up adds the
+# correlated column to every set, so `ROLLUP(k)`, which is
+# `GROUPING SETS ((k), ())`, turns into `GROUPING SETS ((k), (k, k))`. The
empty
+# set is gone, and with it the grand total row the subquery returns for every
+# outer row, including the rows whose filter matches nothing.
+statement ok
+CREATE TABLE gs_outer(k INT) AS VALUES (1), (2), (NULL), (4), (5);
+
+statement ok
+CREATE TABLE gs_inner(k INT, j INT) AS VALUES (1, 10), (NULL, 20), (5, 30),
(2, 40);
+
+# ROLLUP holds the empty set, so the subquery stays correlated.
+statement error DataFusion error: This feature is not implemented: Physical
plan does not support logical expression Exists
+SELECT gs_outer.k, EXISTS (SELECT 1 FROM gs_inner WHERE gs_inner.k =
gs_outer.k GROUP BY ROLLUP(gs_inner.k)) FROM gs_outer;
+
+# So does CUBE.
+statement error DataFusion error: This feature is not implemented: Physical
plan does not support logical expression Exists
+SELECT gs_outer.k, EXISTS (SELECT 1 FROM gs_inner WHERE gs_inner.k =
gs_outer.k GROUP BY CUBE(gs_inner.k)) FROM gs_outer;
+
+# And an explicit grouping set that lists the empty set.
+statement error DataFusion error: This feature is not implemented: Physical
plan does not support logical expression Exists
+SELECT gs_outer.k, EXISTS (SELECT 1 FROM gs_inner WHERE gs_inner.k =
gs_outer.k GROUP BY GROUPING SETS ((gs_inner.k), ())) FROM gs_outer;
+
+# A set that groups by another column does not carry the correlated column
either.
Review Comment:
Comment gives the empty-set rationale, which doesn't apply: `((k), (j))` has
no empty set. The reason is the NULL fill — the pull up turns `(j)` into `(j,
k)`, so `gs_inner.k` is non-NULL where the original set fills it with NULL.
Suggested wording plus a case that shows it (main returns `false` for every
row; should be `true` for 1, 2, 5):
```diff
-# A set that groups by another column does not carry the correlated column
either.
+# A set that leaves out the correlated column fills it with NULL. The pull
up
+# would turn `(j)` into `(j, k)`, and `k` would then carry a value in the
rows
+# where the subquery returns NULL. Anything above the aggregate that reads
`k`
+# sees the difference, so the subquery stays correlated.
+#
+# Known limitation: when nothing reads `k`, as here, the pull up was correct
+# before this guard and the query now fails to plan. Telling the two cases
apart
+# needs the correlated column added to each set under an alias.
statement error DataFusion error: This feature is not implemented: Physical
plan does not support logical expression Exists
SELECT gs_outer.k, EXISTS (SELECT 1 FROM gs_inner WHERE gs_inner.k =
gs_outer.k GROUP BY GROUPING SETS ((gs_inner.k), (gs_inner.j))) FROM gs_outer;
+
+# The same sets with a HAVING that reads the NULL filled column. For k = 1
the
+# `(j)` set yields the row `(NULL, 10)`, which passes the HAVING, so EXISTS
is
+# true. With `(j, k)` that row has `k = 1` and is filtered out.
+statement error DataFusion error: This feature is not implemented: Physical
plan does not support logical expression Exists
+SELECT gs_outer.k, EXISTS (SELECT 1 FROM gs_inner WHERE gs_inner.k =
gs_outer.k GROUP BY GROUPING SETS ((gs_inner.k), (gs_inner.j)) HAVING
gs_inner.k IS NULL) FROM gs_outer;
```
The doc on `grouping_sets_cover_pull_up_cols` has the same gap, it only
explains the empty set:
```diff
/// `ROLLUP` and `CUBE` always contain the empty set, which yields a
row for
/// outer rows the correlated filter matches nothing for, so they are
only safe
/// when there is nothing to add.
+ ///
+ /// A non-empty set that leaves a column out fills it with NULL. Adding
the
+ /// column would give it a value that a HAVING or a projection above the
+ /// aggregate can read, so such a set is rejected as well.
```
The alias-based fix is fine as a follow-up.
--
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]
