Introduce TreeNode::exists() API, avoid copying expressions
#10008
Conversation
github-actions
bot
added
logical-expr
|
@alamb, I think there are other places where |
alamb
changed the title
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @peter-toth -- this looks great! (and thank you for keeping the PRs small).
I have some comment suggestions, but I also think we could add them as a follow on PR too
|
|
||
| fn contains_outer_reference(inner_plan: &LogicalPlan) -> bool { | ||
| inner_plan | ||
| .expressions() |
| } | ||
| } | ||
| _ if plan.expressions().iter().any(|expr| expr.contains_outer()) => { | ||
| _ if plan.contains_outer_reference() => { |
alamb
changed the title
TreeNode::exists() API, avoid copying expressions
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
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. |
|
Thank you @peter-toth for all your help |
2 participants
Which issue does this PR close?
Part of #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: #9913 (comment). Since #9913 there is no need to call
LogicalPlan::expressions()(that copies the plan's expressions) at certain places, but iterating over the expressions withLogicalPlan::apply_expressions()is enough.What changes are included in this PR?
TreeNode::exists()API.contains_outer_referencehelper with a better solution.Are these changes tested?
Yes, with existing UTs.
Are there any user-facing changes?
No.