Add alias check for equijoin in from_plan #4755
Conversation
github-actions
bot
added
core
| " RepartitionExec: partitioning=Hash([Column { name: \"t2.t2_id + Int64(1)\", index: 1 }], 2)", | ||
| " ProjectionExec: expr=[t2_id@0 as t2_id, t2_id@0 + CAST(1 AS UInt32) as t2.t2_id + Int64(1)]", | ||
| " RepartitionExec: partitioning=Hash([Column { name: \"t2.t2_id + UInt32(1)\", index: 1 }], 2)", | ||
| " ProjectionExec: expr=[t2_id@0 as t2_id, t2_id@0 + 1 as t2.t2_id + UInt32(1)]", | ||
| " RepartitionExec: partitioning=RoundRobinBatch(2)", | ||
| " MemoryExec: partitions=1, partition_sizes=[1]", |
There was a problem hiding this comment.
The SimplifyExpressions rule failed in this test case before, take effect now.
Other test cases modified in this pr also take effect.
datafusion/expr/src/utils.rs
Outdated
| @@ -586,6 +586,12 @@ pub fn from_plan( | |||
| // The preceding part of expr is equi-exprs, | |||
| // and the struct of each equi-expr is like `left-expr = right-expr`. | |||
| let new_on:Vec<(Expr,Expr)> = expr.iter().take(equi_expr_count).map(|equi_expr| { | |||
| // SimplifyExpression rule may add alias to the equi_expr. | |||
| let equi_expr = match equi_expr { | |||
There was a problem hiding this comment.
Perhaps calling unalias might be appropriate
There was a problem hiding this comment.
We can not call the above unalias, because it is in the optimizer crate and from_plan is in the expr crate.
I found another unalias in the expr crate, and it seems enough to this pr.
https://github.com/apache/arrow-datafusion/blob/41c72cf515389e90c20433dbcc4116a59016a15b/datafusion/expr/src/expr.rs#L711-L717
|
Thank you @ygf11 |
|
Benchmark runs are scheduled for baseline = d4934e8 and contender = 310e871. 310e871 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?
Closes #4754.
Rationale for this change
The
SimplifyExpressionswill give an alias to the result if the new name and old name is not same. For the rebuild of equijoin in from_plan, we assume the simplified result of a binary equality expression is still a binary equality expression, this is not correct and may result in the failure ofSimplifyExpressionsrule.What changes are included in this PR?
Add alias check for equijoin in from_plan.
Are these changes tested?
Yes.
Are there any user-facing changes?