peter-toth commented on code in PR #10868:
URL: https://github.com/apache/datafusion/pull/10868#discussion_r1635203931
##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -515,14 +522,15 @@ fn build_common_expr_project_plan(
let mut fields_set = BTreeSet::new();
let mut project_exprs = common_exprs
.into_iter()
- .map(|(expr_id, expr)| {
+ .enumerate()
+ .map(|(index, (expr_id, expr))| {
Review Comment:
> We shouldn't worry about that scenario because we make sure that all
expressions are properly aliased before writing them. The closest example I can
find of a similar scenario is this or maybe even this. You'll see that the CSE
is aliased to its original name and the name can be given to other expressions.
The problem is with the extracted expressions' aliases in the newly added
`Projection` nodes.
> So for the example you gave it would be the "constant folding"
responsibility to alias #0 in the intermediate projection to something else.
No, contant folding should only touch the "original" top level project node
and change `(a + 1 + 1) as "c4"` to `(a + 2) as "c4"`.
> I can try passing common_exprs between different optimization calls if
that will make you feel safer about the change, but that may be challenging
given DataFusion's language of choice (aka our beloved Rust).
I think what we should do is to check the schema and choose an alias that
doesn't exist yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
