implement rewrite for ExtractEquijoinPredicate and avoid clone in filter #10165
Conversation
| let (equijoin_predicates, non_equijoin_expr) = | ||
| split_eq_and_noneq_join_predicate( | ||
| expr, | ||
| left_schema, | ||
| right_schema, | ||
| )?; | ||
|
|
||
| let optimized_plan = (!equijoin_predicates.is_empty()).then(|| { | ||
| let mut new_on = on.clone(); |
There was a problem hiding this comment.
here to avoid clone of vector on
| left: left.clone(), | ||
| right: right.clone(), | ||
| on: new_on, |
There was a problem hiding this comment.
here to avoid clone of arc
| } | ||
| } | ||
|
|
||
| fn split_eq_and_noneq_join_predicate( | ||
| filter: &Expr, |
There was a problem hiding this comment.
here we directly split filter to avoid further clone
| @@ -141,13 +193,13 @@ fn split_eq_and_noneq_join_predicate( | |||
| if can_hash(&left_expr_type) && can_hash(&right_expr_type) { | |||
| accum_join_keys.push((left_expr, right_expr)); | |||
| } else { | |||
| accum_filters.push(expr.clone()); | |||
| "extract_equijoin_predicate" | ||
| } | ||
| /// split with ownership | ||
| fn split_conjunction_own(expr: Expr) -> Vec<Expr> { |
There was a problem hiding this comment.
here implement a ownership version of split conjunction to avoid clone while passing reference
There was a problem hiding this comment.
I think there is already a function that does this: https://docs.rs/datafusion/latest/datafusion/logical_expr/utils/fn.split_conjunction_owned.html
There was a problem hiding this comment.
Thanks @Lordworms -- I think this PR looks very good 👌 .
I left a few suggestions that I think could make it better if you have some time. Otherwise we can do them as follow on PRs
| "extract_equijoin_predicate" | ||
| } | ||
| /// split with ownership | ||
| fn split_conjunction_own(expr: Expr) -> Vec<Expr> { |
There was a problem hiding this comment.
I think there is already a function that does this: https://docs.rs/datafusion/latest/datafusion/logical_expr/utils/fn.split_conjunction_owned.html
| @@ -67,66 +88,97 @@ impl OptimizerRule for ExtractEquijoinPredicate { | |||
| }) => { | |||
There was a problem hiding this comment.
You might be able to avoid a level of indenting if you did the entire match here, like:
match plan {
LogicalPlan::Join(Join {
left,
right,
mut on,
filter: Some(expr),
join_type,
join_constraint,
}) => {
Then you could avoid reassembling LogicalPlan::Join as well
…ter (apache#10165) * implement rewrite for ExtractEquijoinPredicate and avoid clone in filter * fix clippy * optimize code
Which issue does this PR close?
part of #9637
Closes #.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?