Stop copying LogicalPlan and Exprs in DecorrelatePredicateSubquery#10318
Stop copying LogicalPlan and Exprs in DecorrelatePredicateSubquery#10318alamb merged 2 commits intoapache:mainfrom
DecorrelatePredicateSubquery#10318Conversation
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
I think it is easier to see what is actually changed in this PR using whitespace blind diff: https://github.com/apache/datafusion/pull/10318/files?w=1
| .iter() | ||
| .any(|expr| matches!(expr, Expr::InSubquery(_) | Expr::Exists(_))); | ||
| if !has_subqueries { | ||
| return Ok(Transformed::no(LogicalPlan::Filter(filter))); |
There was a problem hiding this comment.
in the common case where there are no subqueries there will be no rewriting / cloning (which is the same as main
| } | ||
|
|
||
| // iterate through all exists clauses in predicate, turning each into a join | ||
| let mut cur_input = filter.input.as_ref().clone(); |
There was a problem hiding this comment.
this is one avoided clone of the input plan
| query, | ||
| where_in_expr: Some(expr), | ||
| negated: false, | ||
| } => in_subquery(expr, query.subquery.clone()), |
There was a problem hiding this comment.
these are clones of Arcs so while this PR removes them and it is less cloning realistically it doesn't matter
| use arrow::record_batch::RecordBatch; | ||
|
|
||
| use datafusion_common::Result; | ||
| use datafusion_common::{internal_err, Result}; |
There was a problem hiding this comment.
This is the change from #10317 to get the CI to pass
alamb
changed the title
DecorrelatePredicateSubqueryDecorrelatePredicateSubquery
waynexia
left a comment
waynexia
left a comment
There was a problem hiding this comment.
LGTM, thanks! Except for avoiding clones, using if let to reduce one nest block is also great 👍
|
Thanks for the review @waynexia |
2 participants
Which issue does this PR close?
Part of #9637
Closes #10289
Rationale for this change
Reduce copying in
What changes are included in this PR?
DecorrelatePredicateSubqueryAre these changes tested?
covered by existing tests
Are there any user-facing changes?
No