[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2334: fix: union schema

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

alamb commented on code in PR #2334:
URL: https://github.com/apache/arrow-datafusion/pull/2334#discussion_r858034480


##########
datafusion/core/src/execution/context.rs:
##########
@@ -3243,7 +3243,7 @@ mod tests {
             "SELECT cat, SUM(i) AS total FROM (
                     SELECT i, 'a' AS cat FROM catalog_a.schema_a.table_a
                     UNION ALL
-                    SELECT i, 'b' AS cat FROM catalog_b.schema_b.table_b

Review Comment:
   I would recommend leaving this test alone and instead adding a new test, perhaps in https://github.com/apache/arrow-datafusion/blob/master/datafusion/core/tests/sql/union.rs using the reproducer from https://github.com/apache/arrow-datafusion/issues/2083
   
   That way it is clear we haven't changed the coverage of this test and there is a more focused tests for just this particular change



##########
datafusion/core/src/logical_plan/builder.rs:
##########
@@ -1055,19 +1055,23 @@ pub fn union_with_alias(
     right_plan: LogicalPlan,
     alias: Option<String>,
 ) -> Result<LogicalPlan> {
-    let inputs = vec![left_plan, right_plan]
+    let union_schema = left_plan.schema().clone();
+    let inputs_iter = vec![left_plan, right_plan]

Review Comment:
   I wonder if there is a reason you needed to make changes below this line (other than to use `union_schema` rather than `schema` when creating the `Projection`
   
   In particular, I wonder if we can avoid the extra call to `clone()` by going back to the previous setup?



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