jayzhan211 commented on code in PR #26640:
URL: https://github.com/apache/datafusion/pull/26640#discussion_r4400542838
##########
datafusion/optimizer/src/decorrelate.rs:
##########
@@ -910,6 +936,34 @@ impl PullUpCorrelatedExpr {
}
}
+/// Whether the emptiness of `plan`'s output follows from the emptiness of its
+/// input alone, so that `plan` cannot tell apart two non-empty inputs.
Removing
+/// a `Limit` below such a node changes which rows it is given, never whether
it
+/// is given any, and so never whether the subquery is empty.
+///
+/// A `Projection`, a `SubqueryAlias`, a `Sort`, a `Window` and a `Repartition`
+/// each output one row per input row. A `DISTINCT` and a grouped `Aggregate`
output rows
+/// exactly when their input has rows, and an `Aggregate` without a grouping
+/// always outputs its one row, so no `Aggregate` can be emptied by the rows a
+/// `Limit` held back.
+///
+/// A `Filter` is what this leaves out: it reads values, so the rows the
`Limit`
+/// kept decide whether anything survives it. A `Join`, a `Union` and an
+/// `Unnest` can empty a non-empty input too. Every other variant is treated
the
+/// same way, so that a new one does not silently become droppable.
Review Comment:
Optional cleanup: a `Union` can't empty a non-empty input, because its
output is non-empty whenever any input is. It's left off the list because the
`Union` arm already stops the pull-up when it holds an outer reference. Maybe:
```diff
-/// kept decide whether anything survives it. A `Join`, a `Union` and an
-/// `Unnest` can empty a non-empty input too. Every other variant is
treated the
-/// same way, so that a new one does not silently become droppable.
+/// kept decide whether anything survives it. A `Join` and an `Unnest` can
empty
+/// a non-empty input too. Every other variant is treated the same way, so
that
+/// a new one does not silently become droppable.
```
##########
datafusion/optimizer/src/decorrelate.rs:
##########
@@ -910,6 +936,34 @@ impl PullUpCorrelatedExpr {
}
}
+/// Whether the emptiness of `plan`'s output follows from the emptiness of its
+/// input alone, so that `plan` cannot tell apart two non-empty inputs.
Removing
+/// a `Limit` below such a node changes which rows it is given, never whether
it
+/// is given any, and so never whether the subquery is empty.
+///
+/// A `Projection`, a `SubqueryAlias`, a `Sort`, a `Window` and a `Repartition`
+/// each output one row per input row. A `DISTINCT` and a grouped `Aggregate`
output rows
+/// exactly when their input has rows, and an `Aggregate` without a grouping
+/// always outputs its one row, so no `Aggregate` can be emptied by the rows a
+/// `Limit` held back.
+///
+/// A `Filter` is what this leaves out: it reads values, so the rows the
`Limit`
+/// kept decide whether anything survives it. A `Join`, a `Union` and an
+/// `Unnest` can empty a non-empty input too. Every other variant is treated
the
+/// same way, so that a new one does not silently become droppable.
+fn emptiness_follows_input(plan: &LogicalPlan) -> bool {
+ matches!(
+ plan,
+ LogicalPlan::Projection(_)
+ | LogicalPlan::SubqueryAlias(_)
+ | LogicalPlan::Sort(_)
+ | LogicalPlan::Window(_)
+ | LogicalPlan::Repartition(_)
+ | LogicalPlan::Distinct(Distinct::All(_))
+ | LogicalPlan::Aggregate(_)
Review Comment:
@raghavyadav01 thanks for the fix! One regression I hit: an `EXISTS` with a
`MATERIALIZED` CTE whose `LIMIT` sits at the root of the continuation now fails
to plan, but on `main` it returns the right rows. The `Cte` arm pushes `false`
for itself and then calls `continuation.rewrite(self)`, so the `Limit` inside
sees a blocking node above it. A `Cte` outputs exactly its continuation's rows,
and only the continuation is rewritten, so its emptiness follows its input.
Repro (`main`: `1, 2`; this PR: `Physical plan does not support logical
expression Exists`):
```sql
SET datafusion.sql_parser.dialect = 'PostgreSQL';
CREATE TABLE a(k INT, x INT) AS VALUES (1, 10), (2, 20), (3, 30);
CREATE TABLE b(k INT, v INT) AS VALUES (1, 100), (1, 200), (2, 300);
SELECT a.k FROM a WHERE EXISTS (
WITH c AS MATERIALIZED (SELECT * FROM b)
SELECT 1 FROM c WHERE c.k = a.k LIMIT 1);
```
Fix:
```diff
| LogicalPlan::Distinct(Distinct::All(_))
| LogicalPlan::Aggregate(_)
+ | LogicalPlan::MaterializedCte(MaterializedCtePlan::Cte(_))
)
```
Could you add these two cases to `cte_materialized.slt` (just before "A body
that refers to the outer query…")? The first must decorrelate, and the second
must stay refused (on `main` it wrongly returns `1`):
```sql
# A LIMIT at the root of the continuation does not change whether it is
empty.
query I rowsort
SELECT k FROM t WHERE EXISTS (
WITH r AS MATERIALIZED (SELECT k AS rk FROM t)
SELECT 1 FROM r WHERE r.rk = t.k + 1 LIMIT 1)
----
1
2
3
# A filter above the LIMIT reads the row the LIMIT kept, so it stays
correlated.
query error Physical plan does not support logical expression Exists
SELECT k FROM t WHERE EXISTS (
WITH r AS MATERIALIZED (SELECT k AS rk, v AS rv FROM t)
SELECT 1 FROM (SELECT * FROM r WHERE r.rk = t.k LIMIT 1) s WHERE s.rv > 0)
```
--
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]
