kosiew commented on code in PR #25294:
URL: https://github.com/apache/datafusion/pull/25294#discussion_r4134207051
##########
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:
Could we check ordinary `Expr::Column` references in the existing
`expr.apply` walk instead of calling `column_refs()` first? That would preserve
the behavior while avoiding the temporary `HashSet` and second traversal.
##########
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:
`references_any_column` handles `Exists`, `InSubquery`, and
`ScalarSubquery`, but it misses `Expr::SetComparison`, which also stores
correlated columns in `Subquery::outer_ref_columns`. Please include
`SetComparison` here and add an `ANY` or `ALL` regression test where the
comparison expression itself does not reference the protected column.
--
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]
