You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "peter-toth (via GitHub)" <gi...@apache.org> on 2024/04/09 08:50:48 UTC
[PR] Introduce TreeNode::exists() API [arrow-datafusion]
peter-toth opened a new pull request, #10008:
URL: https://github.com/apache/arrow-datafusion/pull/10008
## Which issue does this PR close?
Part of https://github.com/apache/arrow-datafusion/issues/8913.
## Rationale for this change
This PR adds `TreeNode::exists()` API as a common pattern to check the presence of a matching node for a predicate in a tree.
This PR also solves the issue found by @alamb here: https://github.com/apache/arrow-datafusion/pull/9913#discussion_r1554563916. Since https://github.com/apache/arrow-datafusion/pull/9913 there is no need to call `LogicalPlan::expressions()` (that copies the plan's expressions) at certain places, but iterating over the expressions with `LogicalPlan::apply_expressions()` is enough.
## What changes are included in this PR?
- Adds the `TreeNode::exists()` API.
- Replaces `contains_outer_reference` helper with a better solution.
## Are these changes tested?
Yes, with existing UTs.
## Are there any user-facing changes?
No.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduce TreeNode::exists() API, avoid copying expressions [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10008:
URL: https://github.com/apache/arrow-datafusion/pull/10008#discussion_r1557373552
##########
datafusion/expr/src/logical_plan/plan.rs:
##########
@@ -1321,6 +1321,20 @@ impl LogicalPlan {
| LogicalPlan::Extension(_) => None,
}
}
+
+ pub fn contains_outer_reference(&self) -> bool {
Review Comment:
If we are going to make this a public functions, I think it should have comments. For example:
```suggestion
/// If this node's expressions contains any references to an outer subquery
pub fn contains_outer_reference(&self) -> bool {
```
##########
datafusion/optimizer/src/analyzer/subquery.rs:
##########
@@ -233,13 +233,6 @@ fn check_inner_plan(
}
}
-fn contains_outer_reference(inner_plan: &LogicalPlan) -> bool {
- inner_plan
- .expressions()
Review Comment:
❤️ for removing this copy 🚀
##########
datafusion/optimizer/src/decorrelate.rs:
##########
@@ -91,7 +91,7 @@ impl TreeNodeRewriter for PullUpCorrelatedExpr {
_ => Ok(Transformed::no(plan)),
}
}
- _ if plan.expressions().iter().any(|expr| expr.contains_outer()) => {
+ _ if plan.contains_outer_reference() => {
Review Comment:
And another copy gone!
##########
datafusion/common/src/tree_node.rs:
##########
@@ -292,6 +292,20 @@ pub trait TreeNode: Sized {
)
}
+ fn exists<F: FnMut(&Self) -> bool>(&self, mut f: F) -> bool {
Review Comment:
Can we please add some documentation to this method?
Perhaps something like
```suggestion
/// Returns true if `f` returns true for node in the tree.
///
/// Stops recursion as soon as a matching node is found
fn exists<F: FnMut(&Self) -> bool>(&self, mut f: F) -> bool {
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduce TreeNode::exists() API [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #10008:
URL: https://github.com/apache/arrow-datafusion/pull/10008#issuecomment-2044486015
@alamb, I think there are other places where `LogicalPlan::expressions()` can be replaced with the faster `LogicalPlan::apply_expressions()` but the change is non-trivial, so let's start with this smaller PR 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduce `TreeNode::exists()` API, avoid copying expressions [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10008:
URL: https://github.com/apache/arrow-datafusion/pull/10008#issuecomment-2045348306
Thank *you* @peter-toth for all your help
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduce `TreeNode::exists()` API, avoid copying expressions [arrow-datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #10008:
URL: https://github.com/apache/arrow-datafusion/pull/10008#issuecomment-2045322132
> I took the liberty of adding the doc comments directly to this PR. Thanks again @peter-toth
Thanks @alamb for the doc comments and the review! Actually, I wanted to ask you to do that as I'm travelling this week and have limited access to github.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduce `TreeNode::exists()` API, avoid copying expressions [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #10008:
URL: https://github.com/apache/arrow-datafusion/pull/10008
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org