Uh oh!
There was an error while loading. Please reload this page.
fix: don't unifies projection if expr is non-trival - #8454
Conversation
There was a problem hiding this comment.
Thank you for the contribution @haohuaijin . This change makes sense to me
cc @mustafasrepo and @Dandandan
| let mut column_ref_map: HashMap<Column, usize> = HashMap::new(); | ||
| // Collect the column references usage in the outer projection. | ||
| projection.expr().iter().for_each(|(expr, _)| { |
There was a problem hiding this comment.
I may be mistaken and it may not matter, but I think this will prevent pushing down exprs that refer to the same column more than once
Projection exprs=[b as c, b as d]
Projection exprs = [a as b]
Into
Projection exprs=[a as c, a as d]
There was a problem hiding this comment.
We don't prevent Column and Literal push_down. We prevent other express push_down, used to prevent multiple evaluation of the same expression.
if column_ref_map.iter().any(|(column, count)| {*count > 1 && !is_expr_trivial(&child.expr()[column.index()].0.clone())}){returnOk(None);}
mustafasrepo
left a comment
There was a problem hiding this comment.
Thanks @haohuaijin for this PR. It is LGTM!.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Alex Huang <huangweijun1001@gmail.com>
alamb
commented
Dec 8, 2023
Thank you @haohuaijin,. @Weijun-H and @mustafasrepo |
* fix: don't unifies projection if expr is non-trival * Update datafusion/core/src/physical_optimizer/projection_pushdown.rs Co-authored-by: Alex Huang <huangweijun1001@gmail.com> --------- Co-authored-by: Alex Huang <huangweijun1001@gmail.com>
Which issue does this PR close?
Closes#8453
Rationale for this change
What changes are included in this PR?
don't unifies projection if the expression in non-trival. see discussion in #8296
Are these changes tested?
Are there any user-facing changes?
add physical plan check in
.slttest