LiaCastaneda commented on code in PR #15438:
URL: https://github.com/apache/datafusion/pull/15438#discussion_r2025709682
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does
not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return
Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
Hi, sorry about this - I'm trying to understand this, in that change what I
did is to ammend the issue by unifying the naming instead of skipping the check.
I checked on how we build the LogicalPlan and input fields have the same
names as the projection column expressions, both have count(Int64(1)):1 so I
don't know where that field is being set to count(Int64(1)) for the physcial
input_schema. On the physical planner I also printed the Logical input schema
[here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2009)
and it appears as count(Int64(1)):1 . I also noticed that same function
[creates a
physical](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2042)
Expr for the projection based on the logical input_schema and not the physical
input_exec schema hence why it fails later on the check.
I think an approach could be to move the check
[here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2028)
and add an option on the runtime config we can get thorugh the session_state
to skip it (and by default set it to false), same as it is done for
[aggregate](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L658)
nodes. I was looking into your PR that adds that check also, but apparently
we don't have `create_physical_name` anymore after #11977
##########
datafusion/physical-expr/src/equivalence/projection.rs:
##########
@@ -66,9 +66,9 @@ impl ProjectionMapping {
let idx = col.index();
let matching_input_field = input_schema.field(idx);
if col.name() != matching_input_field.name() {
- return internal_err!("Input field name {} does
not match with the projection expression {}",
- matching_input_field.name(),col.name())
- }
+ let fixed_col = Column::new(col.name(), idx);
+ return
Ok(Transformed::yes(Arc::new(fixed_col)))
+ }
Review Comment:
Hi, sorry about this - I'm trying to understand this, in that change what I
did is to ammend the issue by unifying the naming instead of skipping the check.
I checked on how we build the `LogicalPlan` and input fields have the same
names as the projection column expressions, both have `count(Int64(1)):1` so I
don't know where that field is being set to `count(Int64(1))` for the physcial
`input_schema`. On the physical planner I also printed the Logical input schema
[here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2009)
and it appears as `count(Int64(1)):1` . I also noticed that same function
[creates a
physical](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2042)
`Expr` for the projection based on the logical `input_schema` and not the
physical `input_exec` schema hence why it fails later on the check.
edit: I think an approach could be to move the check
[here](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L2028)
and add an option on the runtime config we can get thorugh the `session_state`
to skip it (and by default set it to false), same as it is done for
[aggregate](https://github.com/apache/datafusion/blob/28451b5ff1a1363c3495dc328cf8aaf9bc7b925e/datafusion/core/src/physical_planner.rs#L658)
nodes
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
