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) so errors can still be caught while 
developing (iiuc this doesn't cause errors during execution), something similar 
was 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, but apparently we 
don't have `create_physical_name` anymore after #11977 
   
   edit: I have a solution in mind based on what I mentioned above, can this 
wait a couple of days instead of reverting the PR commit, since it will also 
revert the duplicate schema names fix. I will try opening another PR this week 
which will include the check back.



-- 
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

Reply via email to