alamb commented on code in PR #9154: URL: https://github.com/apache/arrow-datafusion/pull/9154#discussion_r1492339330
########## datafusion/sqllogictest/test_files/same_column_name_cross_join.slt: ########## @@ -0,0 +1,61 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at + +# http://www.apache.org/licenses/LICENSE-2.0 + +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + + +# prepare the tables Review Comment: I ran this test without the code in this PR and it fails like this, so I think the test covers the changes in the PR ``` External error: query failed: DataFusion error: Error during planning: Projections require unique expression names but the expression "t.a" at position 0 and "t.a" at position 2 have the same name. Consider aliasing ("AS") one of them. [SQL] select * from (t1 cross join t2) as t cross join t3; ------- at test_files/./join.slt:733 at test_files/join_disable_repartition_joins.slt:26 Error: Execution("1 failures") error: test failed, to rerun pass `-p datafusion-sqllogictest --test sqllogictests` Caused by: process didn't exit successfully: `/Users/andrewlamb/Software/arrow-datafusion/target/debug/deps/sqllogictests-6e501f6e871dacfc joins` (exit status: 1) ``` ########## datafusion/expr/src/logical_plan/builder.rs: ########## @@ -1124,7 +1124,27 @@ impl LogicalPlanBuilder { )?)) } } - +pub fn change_redundant_column(fields: Vec<DFField>) -> Vec<DFField> { + let mut name_map = HashMap::new(); + fields + .into_iter() + .map(|field| { + let counter = name_map.entry(field.name().to_string()).or_insert(0); + *counter += 1; + if *counter > 1 { + let new_name = format!("{}:{}", field.name(), *counter - 1); Review Comment: This effectively renames some of the columns to something else (like `a:1`) I think How do we: 1. know that name is unique? (couldn't there be another table in the query that has a column named `a:1`? 2. know we should resolve the namespace conflict with the first column with the name? ########## datafusion/expr/src/logical_plan/builder.rs: ########## @@ -2076,4 +2098,25 @@ mod tests { Ok(()) } + #[test] + fn test_change_redundant_column() -> Result<()> { + let t1_field_1 = DFField::new_unqualified("a", DataType::Int32, false); + let t2_field_1 = DFField::new_unqualified("a", DataType::Int32, false); + let t1_field_2 = DFField::new_unqualified("b", DataType::Int32, false); + let t2_field_2 = DFField::new_unqualified("b", DataType::Int32, false); + Review Comment: Would it be possible to add one more field named "a" here (to show a counter other than `:1`)? -- 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]
