[GitHub] [arrow-datafusion] waynexia commented on a diff in pull request #2915: Fix invalid projection in `CommonSubexprEliminate`

[GitBox <gi...@apache.org>]

waynexia commented on code in PR #2915:
URL: https://github.com/apache/arrow-datafusion/pull/2915#discussion_r921756118


##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -282,15 +282,13 @@ fn build_project_plan(
     }
 
     for field in input.schema().fields() {
-        if !fields_set.contains(field.name()) {
-            fields_set.insert(field.name().to_owned());
+        if fields_set.insert(field.qualified_name()) {

Review Comment:
   Nice simplification 
   
   >Use qualified name instead of unqualified name in the HashSet that tracks which fields have already been added to the projection
   
   This is correct 👍 It used to be inconsistent with L287 which uses qualified name. 
   



##########
datafusion/optimizer/src/common_subexpr_eliminate.rs:
##########
@@ -282,15 +282,13 @@ fn build_project_plan(
     }
 
     for field in input.schema().fields() {
-        if !fields_set.contains(field.name()) {
-            fields_set.insert(field.name().to_owned());
+        if fields_set.insert(field.qualified_name()) {
             fields.push(field.clone());
             project_exprs.push(Expr::Column(field.qualified_column()));
         }
     }
 
-    let mut schema = DFSchema::new_with_metadata(fields, HashMap::new())?;
-    schema.merge(input.schema());

Review Comment:
   `merge` is done in the previous `for` loop so I think this is redundant. Let's remove it.



-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org