Refactor: Add LogicalPlan::observe_expressions to walk expressions
#4906
Conversation
| @@ -233,42 +233,60 @@ impl LogicalPlan { | |||
| /// logical plan node. This does not include expressions in any | |||
| /// children | |||
| pub fn expressions(self: &LogicalPlan) -> Vec<Expr> { | |||
| let mut exprs = vec![]; | |||
| self.observe_expressions(|e| exprs.push(e.clone())); | |||
There was a problem hiding this comment.
The basic idea is to refactor the current code to clone all expressions to just call a function
|
|
||
| fn collect_subqueries(expr: &Expr, sub: &mut Vec<Arc<LogicalPlan>>) { | ||
| match expr { |
alamb
force-pushed
the
alamb/walk_all_exprs
branch
from
112bd27
to
9f1b465
Compare
alamb
changed the title
alamb
changed the title
Add LogicalPlan::observe_expressions to walk expressions
| // use a synthetic plan so the visitor sees a | ||
| // LogicalPlan::Subquery (even though it is | ||
| // actually a Subquery alias) | ||
| let synthetic_plan = LogicalPlan::Subquery(subquery.clone()); |
There was a problem hiding this comment.
while this behavior is strange it is consistent with what collect_subqueries was doing
| @@ -396,7 +396,7 @@ fn push_down_all_join( | |||
| .chain(once(keep_condition.into_iter().reduce(Expr::and).unwrap())) | |||
| .collect() | |||
| } else { | |||
| plan.expressions() | |||
| expr | |||
There was a problem hiding this comment.
drive by cleanup to avoid (yet another) copy of the expressions
| /// Conveniece function for using a mutable function as an expression visiitor | ||
| /// | ||
| /// TODO make this match names in physical plan | ||
| pub fn walk_expr_down<F, E>(expr: &Expr, f: F) -> std::result::Result<(), E> |
|
Benchmark runs are scheduled for baseline = d37dccf and contender = d49c805. d49c805 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Inspired by the bug in #4900 and discussion with @avantgardnerio on #4701 (comment)
Rationale for this change
I realized while reading #4900 there were likely other places in Expr that could have subqueries so there may be other bugs lurking similar to #4898
However, there was no good way to walk all expressions in a LogicalPlan other than to call
LogicalPlan::expressions()which clones them allWhat changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?