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
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
| @@ -233,13 +233,6 @@ fn check_inner_plan( | |||
| } | |||
| } | |||
|
|
|||
| fn contains_outer_reference(inner_plan: &LogicalPlan) -> bool { | |||
| inner_plan | |||
| .expressions() | |||
| @@ -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() => { | |||
alamb
changed the title
TreeNode::exists() API, avoid copying expressions
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 |
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.