kakiuwang-ui commented on code in PR #25294:
URL: https://github.com/apache/datafusion/pull/25294#discussion_r4136003425
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -769,6 +769,39 @@ fn infer_join_predicates_impl<
Ok(())
}
+/// Whether `expr` depends on any of the columns named in `names`.
+///
+/// This is `Expr::column_refs` plus the outer columns that any subquery inside
+/// `expr` correlates on. A subquery records those in
+/// `Subquery::outer_ref_columns` rather than as an `Expr::Column` in the
+/// predicate, and `Expr`'s own traversal does not descend into that field, so
+/// looking only at `column_refs` would report such a predicate as depending on
+/// nothing and let it be pushed past a node that asked to keep those columns.
+fn references_any_column(expr: &Expr, names: &HashSet<String>) -> bool {
+ if expr.column_refs().iter().any(|c| names.contains(&c.name)) {
+ return true;
+ }
+
+ let mut found = false;
+ expr.apply(|e| {
+ let outer_refs = match e {
+ Expr::Exists(exists) => &exists.subquery.outer_ref_columns,
Review Comment:
Good catch — fixed in 479bcf5. `SetComparison` is now handled alongside the
other three, and I added
`user_defined_plan_outer_referenced_column_set_comparison`: `test.a > ANY
(SELECT sq.a FROM sq WHERE test.c = sq.a)`, so the comparison expression names
only `test.a` and the dependency on the protected `test.c` exists purely in
`outer_ref_columns`.
Without the `SetComparison` arm that test fails exactly the way the original
bug did:
```
Invalid (non-executable) plan after Optimizer rule: push_down_filter
In/Exist/SetComparison subquery can only be used in Projection, Filter,
TableScan,
Window functions, Aggregate and Join plan nodes, but was used in [NoopPlan]
```
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -769,6 +769,39 @@ fn infer_join_predicates_impl<
Ok(())
}
+/// Whether `expr` depends on any of the columns named in `names`.
+///
+/// This is `Expr::column_refs` plus the outer columns that any subquery inside
+/// `expr` correlates on. A subquery records those in
+/// `Subquery::outer_ref_columns` rather than as an `Expr::Column` in the
+/// predicate, and `Expr`'s own traversal does not descend into that field, so
+/// looking only at `column_refs` would report such a predicate as depending on
+/// nothing and let it be pushed past a node that asked to keep those columns.
+fn references_any_column(expr: &Expr, names: &HashSet<String>) -> bool {
Review Comment:
Done in 479bcf5 — `Expr::Column` is now matched in the same `apply` walk, so
there is no `column_refs()` `HashSet` and no second traversal. Behavior is
unchanged because `add_column_refs` collects columns with that same
`Expr::apply` walk; the only difference is that the column case can now stop
the traversal early instead of always walking the whole expression first.
--
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]
