Merge pull request ClickHouse#75676 from ClickHouse/vidmir/issue_73142

Fix column name clash with `join_using_top_level_identifier`

Author: vdimir

src/Analyzer/JoinNode.cpp

@@ -81,6 +81,10 @@ static ASTPtr tryMakeUsingColumnASTWithAlias(const QueryTreeNodePtr & node)
     if (!lhs_column_node || !rhs_column_node)
         return nullptr;
 
+    /// If USING column resolved from projection, keep its name
+    if (lhs_column_node->hasExpression())
+        return nullptr;
+
     if (lhs_column_node->getColumnName() == rhs_column_node->getColumnName())
         return nullptr;
 

src/Analyzer/Resolve/QueryAnalyzer.cpp

@@ -1743,19 +1743,26 @@ bool hasTableExpressionInJoinTree(const QueryTreeNodePtr & join_tree_node, const
         if (node_to_process == table_expression)
             return true;
 
-        if (node_to_process->getNodeType() == QueryTreeNodeType::JOIN)
+        auto node_type = node_to_process->getNodeType();
+
+        if (node_type == QueryTreeNodeType::JOIN)
         {
             const auto & join_node = node_to_process->as<JoinNode &>();
             nodes_to_process.push_back(join_node.getLeftTableExpression());
             nodes_to_process.push_back(join_node.getRightTableExpression());
         }
-
-        if (node_to_process->getNodeType() == QueryTreeNodeType::CROSS_JOIN)
+        else if (node_type == QueryTreeNodeType::CROSS_JOIN)
         {
             const auto & join_node = node_to_process->as<CrossJoinNode &>();
             for (const auto & expr : join_node.getTableExpressions())
                 nodes_to_process.push_back(expr);
         }
+        else if (node_type == QueryTreeNodeType::ARRAY_JOIN)
+        {
+            const auto & array_join_node = node_to_process->as<ArrayJoinNode &>();
+            nodes_to_process.push_back(array_join_node.getTableExpression());
+        }
+
     }
     return false;
 }
@@ -2203,6 +2210,8 @@ QueryAnalyzer::QueryTreeNodesWithNames QueryAnalyzer::resolveUnqualifiedMatcher(
             table_expression_columns,
             scope);
 
+        updateMatchedColumnsFromJoinUsing(matched_column_nodes_with_names, table_expression, scope);
+
         table_expressions_column_nodes_with_names_stack.push_back(std::move(matched_column_nodes_with_names));
     }
 
@@ -5202,6 +5211,58 @@ void QueryAnalyzer::resolveCrossJoin(QueryTreeNodePtr & cross_join_node, Identif
     }
 }
 
+static NameSet getColumnsFromTableExpression(const QueryTreeNodePtr & table_expression)
+{
+    NameSet existing_columns;
+    switch (table_expression->getNodeType())
+    {
+        case QueryTreeNodeType::TABLE: {
+            const auto * table_node = table_expression->as<TableNode>();
+            chassert(table_node);
+
+            auto get_column_options = GetColumnsOptions(GetColumnsOptions::AllPhysical).withSubcolumns();
+            for (const auto & column : table_node->getStorageSnapshot()->getColumns(get_column_options))
+                existing_columns.insert(column.name);
+
+            return existing_columns;
+        }
+        case QueryTreeNodeType::TABLE_FUNCTION: {
+            const auto * table_function_node = table_expression->as<TableFunctionNode>();
+            chassert(table_function_node);
+
+            auto get_column_options = GetColumnsOptions(GetColumnsOptions::AllPhysical).withSubcolumns();
+            for (const auto & column : table_function_node->getStorageSnapshot()->getColumns(get_column_options))
+                existing_columns.insert(column.name);
+
+            return existing_columns;
+        }
+        case QueryTreeNodeType::QUERY: {
+            const auto * query_node = table_expression->as<QueryNode>();
+            chassert(query_node);
+
+            for (const auto & column : query_node->getProjectionColumns())
+                existing_columns.insert(column.name);
+
+            return existing_columns;
+        }
+        case QueryTreeNodeType::UNION: {
+            const auto * union_node = table_expression->as<UnionNode>();
+            chassert(union_node);
+
+            for (const auto & column : union_node->computeProjectionColumns())
+                existing_columns.insert(column.name);
+
+            return existing_columns;
+        }
+        default:
+            throw Exception(
+                ErrorCodes::LOGICAL_ERROR,
+                "Expected TableNode, TableFunctionNode, QueryNode or UnionNode, got {}: {}",
+                table_expression->getNodeTypeName(),
+                table_expression->formatASTForErrorMessage());
+    }
+}
+
 /// Resolve join node in scope
 void QueryAnalyzer::resolveJoin(QueryTreeNodePtr & join_node, IdentifierResolveScope & scope, QueryExpressionsAliasVisitor & expressions_visitor)
 {
@@ -5286,11 +5347,15 @@ void QueryAnalyzer::resolveJoin(QueryTreeNodePtr & join_node, IdentifierResolveS
                         const auto & resolved_nodes = left_subquery->getProjection().getNodes();
                         if (resolved_nodes.size() == 1)
                         {
+                            /// Added column should not conflict with existing column names
+                            NameSet existing_columns = getColumnsFromTableExpression(left_table_expression);
+
+                            NameAndTypePair column_name_type(identifier_full_name_, resolved_nodes.front()->getResultType());
+                            while (existing_columns.contains(column_name_type.name))
+                                column_name_type.name = "_" + column_name_type.name;
+
                             /// Create ColumnNode with expression from parent projection
-                            return std::make_shared<ColumnNode>(
-                                NameAndTypePair{identifier_full_name_, resolved_nodes.front()->getResultType()},
-                                resolved_nodes.front(),
-                                left_table_expression);
+                            return std::make_shared<ColumnNode>(std::move(column_name_type), resolved_nodes.front(), left_table_expression);
                         }
                     }
                 }
@@ -5324,7 +5389,7 @@ void QueryAnalyzer::resolveJoin(QueryTreeNodePtr & join_node, IdentifierResolveS
             {
                 String extra_message;
                 const QueryNode * query_node = scope.scope_node ? scope.scope_node->as<QueryNode>() : nullptr;
-                if (settings[Setting::analyzer_compatibility_join_using_top_level_identifier] && query_node)
+                if (!settings[Setting::analyzer_compatibility_join_using_top_level_identifier] && query_node)
                 {
                     for (const auto & projection_node : query_node->getProjection().getNodes())
                     {

tests/queries/0_stateless/02989_join_using_parent_scope.reference

@@ -1,4 +1,8 @@
 -- { echoOn }
+SELECT 1 AS c0 FROM (SELECT 1 AS c1) t0 JOIN (SELECT 1 AS c0) t1 USING (c0);
+1
+SELECT 1 AS c0 FROM (SELECT 1 AS c0) t0 JOIN (SELECT 1 AS c0) t1 USING (c0);
+1
 SELECT 1 AS a FROM tb JOIN tabc USING (a) ORDER BY ALL;
 1
 1
@@ -87,6 +91,7 @@ SELECT b + 1 AS a, s FROM tb FULL OUTER JOIN tabc USING (a) PREWHERE a > 2 ORDER
 \N	abc0
 \N	abc1
 \N	abc2
+EXPLAIN PIPELINE SELECT (SELECT 1) AS c0 FROM (SELECT 1 AS c0, 1 AS c1) tx JOIN (SELECT 0 AS c0, 1 AS c1) ty USING (c0, c1) FORMAT Null SETTINGS enable_analyzer = 1;
 -- It's a default behavior for old analyzer and new with analyzer_compatibility_join_using_top_level_identifier
 -- Column `b` actually exists in left table, but `b` from USING is resoled to `a + 2` and `a` is not in left table
 -- so we get UNKNOWN_IDENTIFIER error.

tests/queries/0_stateless/02989_join_using_parent_scope.sql

@@ -15,6 +15,9 @@ SET join_use_nulls = 1;
 SET analyzer_compatibility_join_using_top_level_identifier = 1;
 
 -- { echoOn }
+SELECT 1 AS c0 FROM (SELECT 1 AS c1) t0 JOIN (SELECT 1 AS c0) t1 USING (c0);
+SELECT 1 AS c0 FROM (SELECT 1 AS c0) t0 JOIN (SELECT 1 AS c0) t1 USING (c0);
+
 SELECT 1 AS a FROM tb JOIN tabc USING (a) ORDER BY ALL;
 SELECT a + 2 AS b FROM ta JOIN tabc USING (b) ORDER BY ALL;
 SELECT b + 2 AS a FROM tb JOIN tabc USING (a) ORDER BY ALL;
@@ -40,6 +43,7 @@ SELECT b + 1 AS a, * FROM (SELECT b FROM tb) t1 FULL JOIN (SELECT a, b FROM tabc
 
 SELECT b + 1 AS a, s FROM tb FULL OUTER JOIN tabc USING (a) PREWHERE a > 2 ORDER BY ALL SETTINGS enable_analyzer = 1;
 
+EXPLAIN PIPELINE SELECT (SELECT 1) AS c0 FROM (SELECT 1 AS c0, 1 AS c1) tx JOIN (SELECT 0 AS c0, 1 AS c1) ty USING (c0, c1) FORMAT Null SETTINGS enable_analyzer = 1;
 
 -- It's a default behavior for old analyzer and new with analyzer_compatibility_join_using_top_level_identifier
 -- Column `b` actually exists in left table, but `b` from USING is resoled to `a + 2` and `a` is not in left table

tests/queries/0_stateless/03327_alias_column_constant.reference

@@ -0,0 +1,40 @@
+-- { echoOn }
+
+SELECT b FROM t1;
+1
+1
+1
+SELECT b FROM t1 JOIN t2 USING b;
+1
+1
+1
+1
+1
+1
+1
+1
+1
+SELECT 1 AS b FROM t1 JOIN t2 USING b;
+1
+1
+1
+1
+1
+1
+1
+1
+1
+SELECT 1 AS b FROM t1 JOIN t2 USING b SETTINGS analyzer_compatibility_join_using_top_level_identifier = 1;
+1
+1
+1
+1
+1
+1
+1
+1
+1
+SELECT 2 AS a FROM t1 JOIN t2 USING a SETTINGS analyzer_compatibility_join_using_top_level_identifier = 1;
+2
+2
+2

tests/queries/0_stateless/03327_alias_column_constant.sql

@@ -0,0 +1,18 @@
+
+DROP TABLE IF EXISTS t1;
+CREATE TABLE t1 (a Int32, b Int32 ALIAS 1) ENGINE = MergeTree ORDER BY tuple();
+INSERT INTO t1 VALUES (1), (2), (3);
+
+DROP TABLE IF EXISTS t2;
+CREATE TABLE t2 (a Int32, b Int32 ALIAS 1) ENGINE = MergeTree ORDER BY tuple();
+INSERT INTO t2 VALUES (2), (3), (4);
+
+-- { echoOn }
+
+SELECT b FROM t1;
+SELECT b FROM t1 JOIN t2 USING b;
+SELECT 1 AS b FROM t1 JOIN t2 USING b;
+SELECT 1 AS b FROM t1 JOIN t2 USING b SETTINGS analyzer_compatibility_join_using_top_level_identifier = 1;
+SELECT 2 AS a FROM t1 JOIN t2 USING a SETTINGS analyzer_compatibility_join_using_top_level_identifier = 1;
+
+

tests/queries/0_stateless/03512_join_using_parent_scope_matcher.reference

@@ -0,0 +1,16 @@
+2	3	2
+3	4	3
+4	5	4
+5	6	5
+2	2
+3	3
+4	4
+5	5
+3	2
+4	3
+5	4
+6	5
+(1,2,1)	2	2
+(2,3,1)	3	3
+(3,4,1)	4	4
+(4,5,1)	5	5

tests/queries/0_stateless/03512_join_using_parent_scope_matcher.sql

@@ -0,0 +1,22 @@
+#!/usr/bin/env -S ${HOME}/clickhouse-client --queries-file
+
+
+DROP TABLE IF EXISTS t1;
+DROP TABLE IF EXISTS t2;
+
+CREATE TABLE t1 (`b` Float64) ENGINE = MergeTree ORDER BY tuple();
+INSERT INTO t1 VALUES (1.0), (2.0), (3.0), (4.0), (5.0);
+
+CREATE TABLE t2 (`a` UInt32) ENGINE = MergeTree ORDER BY a;
+INSERT INTO t2 VALUES (1), (2), (3), (4), (5);
+
+SET enable_analyzer = 1;
+
+SET analyzer_compatibility_join_using_top_level_identifier = 1;
+
+SELECT * APPLY ((x) -> x+1), b + 1 AS a FROM t1 INNER JOIN t2 USING (a) ORDER BY ALL;
+SELECT t1.* APPLY ((x) -> x+1), b + 1 AS a FROM t1 INNER JOIN t2 USING (a) ORDER BY ALL;
+SELECT t2.* APPLY ((x) -> x+1), b + 1 AS a FROM t1 INNER JOIN t2 USING (a) ORDER BY ALL;
+
+SELECT (*, 1), b + 1 AS a, b + 1 AS a FROM t1 INNER JOIN t2 USING (a) ORDER BY ALL;
+