MohamedAbdeen21 commented on code in PR #10868:
URL: https://github.com/apache/datafusion/pull/10868#discussion_r1634855644
##########
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:
I can't give you a proof of why no collisions will happen as I'm yet trying
to fully understand how this implementation works with all of the different
optimization functions.
But FWIW I followed every reference of `expr_id`, `expr_identifier()` and
`common_exprs` closely and made sure that this change will work for all the
cases where the previous identifier did.
The only problem I faced was with `try_optimize_aggregate` where the
aggregations are handled by different functions, keeping track of the [last
index](https://github.com/apache/datafusion/pull/10868/files#diff-351499880963d6a383c92e156e75019cd9ce33107724a9635853d7d4cd1898d0R332)
seems to have solved the issue though.
I see no point in indicating to the user which rule generated that alias. If
the user is that interested, he can always run `EXPLAIN VERBOSE`.
If the `#` is a problem, we can drop it and just use the number or even
brackets, but it's simply just a short placeholder alias that is used by a few
other popular engines.
--
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]
