namanjain24-sudo commented on code in PR #25529:
URL: https://github.com/apache/datafusion/pull/25529#discussion_r4056685929
##########
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:
Thanks, you're right that the comment gave the wrong reason, and the NULL
fill is the real one. Applied both your wording and the doc paragraph in
f763afd, with the `HAVING` case added.
I checked the rows behind it first, for `k = 1`:
```
GROUPING SETS ((k), (j)) -> (1, NULL), (NULL, 10)
+ HAVING k IS NULL -> (NULL, 10) EXISTS true
GROUPING SETS ((k), (j, k)) -> (1, 10), (1, NULL)
+ HAVING k IS NULL -> no rows EXISTS false
```
One detail on your note: `false` for every row is the `HAVING` query. The
one already in the file answers `true, true, false, true, false` for `1, 2, 4,
5, NULL` on main, which is correct, so it is only the "fails to plan now"
limitation, as your comment says. The `HAVING` variant is the one main gets
wrong, and it is in the file now with that stated.
Agreed on the alias-based fix 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]
