Fix converting join on keys, move actions into TableJoin

src/Interpreters/ExpressionAnalyzer.cpp

@@ -95,27 +95,6 @@ bool allowEarlyConstantFolding(const ActionsDAG & actions, const Settings & sett
     return true;
 }
 
-
-/// Returns converting actions for tables that need to be performed before join
-ActionsDAGPtr createJoinConvertingActions(const ColumnsWithTypeAndName & cols_src,
-                                          const TableJoin::NameToTypeMap & mapping,
-                                          bool has_using,
-                                          NameToNameMap & renames)
-{
-    ColumnsWithTypeAndName cols_dst = cols_src;
-    for (auto & col : cols_dst)
-    {
-        if (auto it = mapping.find(col.name); it != mapping.end())
-        {
-            col.type = it->second;
-            col.column = nullptr;
-        }
-    }
-    return ActionsDAG::makeConvertingActions(
-        cols_src, cols_dst, ActionsDAG::MatchColumnsMode::Name, true, !has_using, &renames);
-};
-
-
 }
 
 bool sanitizeBlock(Block & block, bool throw_if_cannot_create_column)
@@ -741,15 +720,14 @@ bool SelectQueryExpressionAnalyzer::appendJoinLeftKeys(ExpressionActionsChain &
     return true;
 }
 
-JoinPtr SelectQueryExpressionAnalyzer::appendJoin(ExpressionActionsChain & chain, ActionsDAGPtr & left_actions)
+JoinPtr SelectQueryExpressionAnalyzer::appendJoin(ExpressionActionsChain & chain)
 {
     const ColumnsWithTypeAndName & left_sample_columns = chain.getLastStep().getResultColumns();
-    JoinPtr table_join = makeTableJoin(*syntax->ast_join, left_sample_columns, left_actions);
+    JoinPtr table_join = makeTableJoin(*syntax->ast_join, left_sample_columns);
 
     if (syntax->analyzed_join->needConvert())
     {
-        assert(left_actions);
+        chain.steps.push_back(std::make_unique<ExpressionActionsChain::ExpressionActionsStep>(syntax->analyzed_join->leftConvertingActions()));
-        chain.steps.push_back(std::make_unique<ExpressionActionsChain::ExpressionActionsStep>(left_actions));
         chain.addStep();
     }
 
@@ -820,7 +798,7 @@ static std::shared_ptr<IJoin> makeJoin(std::shared_ptr<TableJoin> analyzed_join,
 }
 
 JoinPtr SelectQueryExpressionAnalyzer::makeTableJoin(
-    const ASTTablesInSelectQueryElement & join_element, const ColumnsWithTypeAndName & left_sample_columns, ActionsDAGPtr & left_actions)
+    const ASTTablesInSelectQueryElement & join_element, const ColumnsWithTypeAndName & left_sample_columns)
 {
     /// Two JOINs are not supported with the same subquery, but different USINGs.
     auto join_hash = join_element.getTreeHash();
@@ -859,27 +837,9 @@ JoinPtr SelectQueryExpressionAnalyzer::makeTableJoin(
         subquery_for_join.addJoinActions(joined_block_actions); /// changes subquery_for_join.sample_block inside
 
         const ColumnsWithTypeAndName & right_sample_columns = subquery_for_join.sample_block.getColumnsWithTypeAndName();
-        /// For `USING` we already inferred common type an syntax analyzer stage
+        bool need_convert = syntax->analyzed_join->applyJoinKeyConvert(left_sample_columns, right_sample_columns);
-        if (!syntax->analyzed_join->hasUsing())
+        if (need_convert)
-            syntax->analyzed_join->inferJoinKeyCommonType(left_sample_columns, right_sample_columns);
+            subquery_for_join.addJoinActions(std::make_shared<ExpressionActions>(syntax->analyzed_join->rightConvertingActions()));
-        if (syntax->analyzed_join->needConvert())
-        {
-            NameToNameMap left_column_rename;
-            left_actions = createJoinConvertingActions(left_sample_columns,
-                                                    syntax->analyzed_join->getLeftMapping(),
-                                                    syntax->analyzed_join->hasUsing(),
-                                                    left_column_rename);
-            syntax->analyzed_join->applyKeyColumnRename(left_column_rename, TableJoin::TableSide::Left);
-
-            NameToNameMap right_renames;
-            auto right_actions = createJoinConvertingActions(right_sample_columns,
-                                                             syntax->analyzed_join->getRightMapping(),
-                                                             syntax->analyzed_join->hasUsing(),
-                                                             right_renames);
-            syntax->analyzed_join->applyKeyColumnRename(right_renames, TableJoin::TableSide::Right);
-
-            subquery_for_join.addJoinActions(std::make_shared<ExpressionActions>(right_actions));
-        }
 
         subquery_for_join.join = makeJoin(syntax->analyzed_join, subquery_for_join.sample_block, context);
 
@@ -1476,7 +1436,8 @@ ExpressionAnalysisResult::ExpressionAnalysisResult(
         {
             query_analyzer.appendJoinLeftKeys(chain, only_types || !first_stage);
             before_join = chain.getLastActions();
-            join = query_analyzer.appendJoin(chain, converting_join_columns);
+            join = query_analyzer.appendJoin(chain);
+            converting_join_columns = query_analyzer.analyzedJoin().leftConvertingActions();
             chain.addStep();
         }
 

src/Interpreters/ExpressionAnalyzer.h

@@ -317,8 +317,7 @@ private:
 
     JoinPtr makeTableJoin(
         const ASTTablesInSelectQueryElement & join_element,
-        const ColumnsWithTypeAndName & left_sample_columns,
+        const ColumnsWithTypeAndName & left_sample_columns);
-        ActionsDAGPtr & left_actions);
 
     const ASTSelectQuery * getAggregatingQuery() const;
 
@@ -339,7 +338,7 @@ private:
     /// Before aggregation:
     ArrayJoinActionPtr appendArrayJoin(ExpressionActionsChain & chain, ActionsDAGPtr & before_array_join, bool only_types);
     bool appendJoinLeftKeys(ExpressionActionsChain & chain, bool only_types);
-    JoinPtr appendJoin(ExpressionActionsChain & chain, ActionsDAGPtr & left_actions);
+    JoinPtr appendJoin(ExpressionActionsChain & chain);
     /// Add preliminary rows filtration. Actions are created in other expression analyzer to prevent any possible alias injection.
     void appendPreliminaryFilter(ExpressionActionsChain & chain, ActionsDAGPtr actions_dag, String column_name);
     /// remove_filter is set in ExpressionActionsChain::finalize();

src/Interpreters/TableJoin.cpp

@@ -338,18 +338,29 @@ bool TableJoin::allowDictJoin(const String & dict_key, const Block & sample_bloc
     return true;
 }
 
-bool TableJoin::inferJoinKeyCommonType(const ColumnsWithTypeAndName & left, const ColumnsWithTypeAndName & right)
+bool TableJoin::applyJoinKeyConvert(const ColumnsWithTypeAndName & left_sample_columns, const ColumnsWithTypeAndName & right_sample_columns)
 {
+    bool need_convert = needConvert();
+    if (!need_convert && !hasUsing())
+    {
+        /// For `USING` we already inferred common type an syntax analyzer stage
         NamesAndTypesList left_list;
         NamesAndTypesList right_list;
-
+        for (const auto & col : left_sample_columns)
-    for (const auto & col : left)
             left_list.emplace_back(col.name, col.type);
-
+        for (const auto & col : right_sample_columns)
-    for (const auto & col : right)
             right_list.emplace_back(col.name, col.type);
 
-    return inferJoinKeyCommonType(left_list, right_list);
+        need_convert = inferJoinKeyCommonType(left_list, right_list);
+    }
+
+    if (need_convert)
+    {
+        left_converting_actions = applyKeyConvertToTable(left_sample_columns, left_type_map, key_names_left);
+        right_converting_actions = applyKeyConvertToTable(right_sample_columns, right_type_map, key_names_right);
+    }
+
+    return need_convert;
 }
 
 bool TableJoin::inferJoinKeyCommonType(const NamesAndTypesList & left, const NamesAndTypesList & right)
@@ -403,17 +414,33 @@ bool TableJoin::inferJoinKeyCommonType(const NamesAndTypesList & left, const Nam
     return !left_type_map.empty();
 }
 
-void TableJoin::applyKeyColumnRename(const NameToNameMap & name_map, TableJoin::TableSide side)
+ActionsDAGPtr
+TableJoin::applyKeyConvertToTable(const ColumnsWithTypeAndName & cols_src, const NameToTypeMap & type_mapping, Names & names_to_rename)
 {
-    assert(!hasUsing() || name_map.empty());
+    ColumnsWithTypeAndName cols_dst = cols_src;
-
+    for (auto & col : cols_dst)
-    Names & names = side == TableSide::Left ? key_names_left : key_names_right;
-    for (auto & name : names)
     {
-        const auto it = name_map.find(name);
+        if (auto it = type_mapping.find(col.name); it != type_mapping.end())
-        if (it != name_map.end())
+        {
+            col.type = it->second;
+            col.column = nullptr;
+        }
+    }
+
+    NameToNameMap key_column_rename;
+    /// Returns converting actions for tables that need to be performed before join
+    auto dag = ActionsDAG::makeConvertingActions(
+        cols_src, cols_dst, ActionsDAG::MatchColumnsMode::Name, true, !hasUsing(), &key_column_rename);
+
+    assert(!hasUsing() || key_column_rename.empty());
+
+    for (auto & name : names_to_rename)
+    {
+        const auto it = key_column_rename.find(name);
+        if (it != key_column_rename.end())
             name = it->second;
     }
+    return dag;
 }
 
 }

src/Interpreters/TableJoin.h

@@ -82,6 +82,9 @@ private:
     NameToTypeMap left_type_map;
     NameToTypeMap right_type_map;
 
+    ActionsDAGPtr left_converting_actions;
+    ActionsDAGPtr right_converting_actions;
+
     /// Name -> original name. Names are the same as in columns_from_joined_table list.
     std::unordered_map<String, String> original_names;
     /// Original name -> name. Only renamed columns.
@@ -91,13 +94,12 @@ private:
 
     Names requiredJoinedNames() const;
 
-public:
+    /// Create converting actions and change key column names if required
-    enum class TableSide
+    ActionsDAGPtr applyKeyConvertToTable(const ColumnsWithTypeAndName & cols_src,
-    {
+                                         const NameToTypeMap & type_mapping,
-        Left,
+                                         Names & names_to_rename);
-        Right
-    };
 
+public:
     TableJoin() = default;
     TableJoin(const Settings &, VolumePtr tmp_volume);
 
@@ -157,19 +159,19 @@ public:
     bool rightBecomeNullable(const DataTypePtr & column_type) const;
     void addJoinedColumn(const NameAndTypePair & joined_column);
 
-    void applyKeyColumnRename(const NameToNameMap & name_map, TableSide side);
-
     void addJoinedColumnsAndCorrectTypes(NamesAndTypesList & names_and_types, bool correct_nullability = true) const;
     void addJoinedColumnsAndCorrectTypes(ColumnsWithTypeAndName & columns, bool correct_nullability = true) const;
 
     /// Calculates common supertypes for corresponding join key columns.
     bool inferJoinKeyCommonType(const NamesAndTypesList & left, const NamesAndTypesList & right);
-    bool inferJoinKeyCommonType(const ColumnsWithTypeAndName & left, const ColumnsWithTypeAndName & right);
+    ///
+    bool applyJoinKeyConvert(const ColumnsWithTypeAndName & left_sample_columns, const ColumnsWithTypeAndName & right_sample_columns);
 
     bool needConvert() const { return !left_type_map.empty(); }
-    /// Key columns should be converted according to this mapping before join.
+
-    const NameToTypeMap & getLeftMapping() const { return left_type_map; }
+    /// Key columns should be converted before join.
-    const NameToTypeMap & getRightMapping() const { return right_type_map; }
+    ActionsDAGPtr leftConvertingActions() const { return left_converting_actions; }
+    ActionsDAGPtr rightConvertingActions() const { return right_converting_actions; }
 
     void setAsofInequality(ASOF::Inequality inequality) { asof_inequality = inequality; }
     ASOF::Inequality getAsofInequality() { return asof_inequality; }