jayzhan211 commented on code in PR #11020:
URL: https://github.com/apache/datafusion/pull/11020#discussion_r1647485139
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -1916,6 +1916,7 @@ pub fn create_aggregate_expr_and_maybe_filter(
// unpack (nested) aliased logical expressions, e.g. "sum(col) as total"
let (name, e) = match e {
Expr::Alias(Alias { expr, name, .. }) => (name.clone(), expr.as_ref()),
+ Expr::AggregateFunction(_) =>
(e.display_name().unwrap_or(physical_name(e)?), e),
Review Comment:
I would be nice if we also use `display_name` in `impl fmt::Display for Expr
`
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -1916,6 +1916,7 @@ pub fn create_aggregate_expr_and_maybe_filter(
// unpack (nested) aliased logical expressions, e.g. "sum(col) as total"
let (name, e) = match e {
Expr::Alias(Alias { expr, name, .. }) => (name.clone(), expr.as_ref()),
+ Expr::AggregateFunction(_) =>
(e.display_name().unwrap_or(physical_name(e)?), e),
Review Comment:
It seems CastExpr is different in `write_name` and `Expr's Display` 🤔
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -1916,6 +1916,7 @@ pub fn create_aggregate_expr_and_maybe_filter(
// unpack (nested) aliased logical expressions, e.g. "sum(col) as total"
let (name, e) = match e {
Expr::Alias(Alias { expr, name, .. }) => (name.clone(), expr.as_ref()),
+ Expr::AggregateFunction(_) =>
(e.display_name().unwrap_or(physical_name(e)?), e),
Review Comment:
It seems CastExpr is different in `write_name` and `Expr's Display` 🤔
And, there is an issue that expect CastExpr be shown in `write name`
https://github.com/apache/datafusion/issues/10274
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -1916,6 +1916,7 @@ pub fn create_aggregate_expr_and_maybe_filter(
// unpack (nested) aliased logical expressions, e.g. "sum(col) as total"
let (name, e) = match e {
Expr::Alias(Alias { expr, name, .. }) => (name.clone(), expr.as_ref()),
+ Expr::AggregateFunction(_) =>
(e.display_name().unwrap_or(physical_name(e)?), e),
Review Comment:
It seems CastExpr is different in `write_name` and `Expr's Display` 🤔
And, there is an issue that expect CastExpr be shown in `write name`
https://github.com/apache/datafusion/issues/10274
CastExpr is removed in #3222, but I didn't find the rationale of doing this
##########
datafusion/core/src/physical_planner.rs:
##########
@@ -1916,6 +1916,7 @@ pub fn create_aggregate_expr_and_maybe_filter(
// unpack (nested) aliased logical expressions, e.g. "sum(col) as total"
let (name, e) = match e {
Expr::Alias(Alias { expr, name, .. }) => (name.clone(), expr.as_ref()),
+ Expr::AggregateFunction(_) =>
(e.display_name().unwrap_or(physical_name(e)?), e),
Review Comment:
It seems CastExpr is different in `write_name` and `Expr's Display` 🤔
And, there is an issue that expect CastExpr be shown in `write name`
https://github.com/apache/datafusion/issues/10274
CastExpr is removed in #3222, but I didn't find the rationale of doing this
If the issue is due to the schema change, then it will be quite tricky
https://github.com/apache/datafusion/pull/10276
We can reuse `display_name` in `physical_name` to fix the regression first
--
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]
