Fixed code review issues

src/Storages/MergeTree/MergeTreeIndexConditionBloomFilter.cpp

@@ -293,8 +293,6 @@ bool MergeTreeIndexConditionBloomFilter::traverseFunction(const RPNBuilderTreeNo
         auto lhs_argument = function.getArgumentAt(0);
         auto rhs_argument = function.getArgumentAt(1);
 
-        auto lhs_argument_column_name = lhs_argument.getColumnName();
-
         if (functionIsInOrGlobalInOperator(function_name))
         {
             ConstSetPtr prepared_set = rhs_argument.tryGetPreparedSet();
@@ -407,20 +405,21 @@ bool MergeTreeIndexConditionBloomFilter::traverseTreeIn(
             if (set_contain_default_value)
                 return false;
 
-            const auto & col_name = key_node_function.getArgumentAt(0).getColumnName();
+            auto first_argument = key_node_function.getArgumentAt(0);
-            auto map_keys_index_column_name = fmt::format("mapKeys({})", col_name);
+            const auto column_name = first_argument.getColumnName();
-            auto map_values_index_column_name = fmt::format("mapValues({})", col_name);
+            auto map_keys_index_column_name = fmt::format("mapKeys({})", column_name);
+            auto map_values_index_column_name = fmt::format("mapValues({})", column_name);
 
             if (header.has(map_keys_index_column_name))
             {
                 /// For mapKeys we serialize key argument with bloom filter
 
-                auto first_argument = key_node_function.getArgumentAt(1);
+                auto second_argument = key_node_function.getArgumentAt(1);
 
                 Field constant_value;
                 DataTypePtr constant_type;
 
-                if (first_argument.tryGetConstant(constant_value, constant_type))
+                if (second_argument.tryGetConstant(constant_value, constant_type))
                 {
                     size_t position = header.getPositionByName(map_keys_index_column_name);
                     const DataTypePtr & index_type = header.getByPosition(position).type;
@@ -688,10 +687,10 @@ bool MergeTreeIndexConditionBloomFilter::traverseTreeEquals(
                 return false;
 
             auto first_argument = key_node_function.getArgumentAt(0);
-            const auto col_name = first_argument.getColumnName();
+            const auto column_name = first_argument.getColumnName();
 
-            auto map_keys_index_column_name = fmt::format("mapKeys({})", col_name);
+            auto map_keys_index_column_name = fmt::format("mapKeys({})", column_name);
-            auto map_values_index_column_name = fmt::format("mapValues({})", col_name);
+            auto map_values_index_column_name = fmt::format("mapValues({})", column_name);
 
             size_t position = 0;
             Field const_value = value_field;

src/Storages/MergeTree/MergeTreeIndexFullText.cpp

@@ -434,7 +434,7 @@ bool MergeTreeConditionFullText::traverseTreeEquals(
                 return false;
 
             auto first_argument = key_function_node.getArgumentAt(0);
-            const auto & map_column_name = first_argument.getColumnName();
+            const auto map_column_name = first_argument.getColumnName();
 
             size_t map_keys_key_column_num = 0;
             auto map_keys_index_column_name = fmt::format("mapKeys({})", map_column_name);
@@ -588,23 +588,20 @@ bool MergeTreeConditionFullText::tryPrepareSetBloomFilter(
     std::vector<KeyTuplePositionMapping> key_tuple_mapping;
     DataTypes data_types;
 
-    if (left_argument.isFunction())
+    auto left_argument_function_node_optional = left_argument.toFunctionNodeOrNull();
+
+    if (left_argument_function_node_optional && left_argument_function_node_optional->getFunctionName() == "tuple")
     {
-        auto left_argument_function_node = left_argument.toFunctionNode();
+        const auto & left_argument_function_node = *left_argument_function_node_optional;
-        auto left_argument_function_node_name = left_argument_function_node.getFunctionName();
+        size_t left_argument_function_node_arguments_size = left_argument_function_node.getArgumentsSize();
 
-        if (left_argument_function_node_name == "tuple")
+        for (size_t i = 0; i < left_argument_function_node_arguments_size; ++i)
         {
-            size_t left_argument_function_node_arguments_size = left_argument_function_node.getArgumentsSize();
+            size_t key = 0;
-
+            if (getKey(left_argument_function_node.getArgumentAt(i).getColumnName(), key))
-            for (size_t i = 0; i < left_argument_function_node_arguments_size; ++i)
             {
-                size_t key = 0;
+                key_tuple_mapping.emplace_back(i, key);
-                if (getKey(left_argument_function_node.getArgumentAt(i).getColumnName(), key))
+                data_types.push_back(index_data_types[key]);
-                {
-                    key_tuple_mapping.emplace_back(i, key);
-                    data_types.push_back(index_data_types[key]);
-                }
             }
         }
     }

src/Storages/MergeTree/MergeTreeIndexUtils.h

@@ -6,7 +6,7 @@ namespace DB
 {
 
 /** Build AST filter node for index analysis from WHERE and PREWHERE sections of select query and additional filters.
-  * If select query does not have WHERE or PREWHERE and additional filters are empty null is returned.
+  * If select query does not have WHERE and PREWHERE and additional filters are empty null is returned.
   */
 ASTPtr buildFilterNode(const ASTPtr & select_query, ASTs additional_filters = {});
 

src/Storages/MergeTree/RPNBuilder.cpp

@@ -129,7 +129,7 @@ std::string RPNBuilderTreeNode::getColumnNameWithModuloLegacy() const
     }
     else
     {
-        return getColumnNameWithoutAlias(*dag_node, true);
+        return getColumnNameWithoutAlias(*dag_node, true /*legacy*/);
     }
 }
 
@@ -203,7 +203,7 @@ bool RPNBuilderTreeNode::tryGetConstant(Field & output_value, DataTypePtr & outp
         String column_name = ast_node->getColumnName();
         const auto & block_with_constants = tree_context.getBlockWithConstants();
 
-        if (const auto * lit = ast_node->as<ASTLiteral>())
+        if (const auto * literal = ast_node->as<ASTLiteral>())
         {
             /// By default block_with_constants has only one column named "_dummy".
             /// If block contains only constants it's may not be preprocessed by
@@ -212,7 +212,7 @@ bool RPNBuilderTreeNode::tryGetConstant(Field & output_value, DataTypePtr & outp
                 column_name = "_dummy";
 
             /// Simple literal
-            output_value = lit->value;
+            output_value = literal->value;
             output_type = block_with_constants.getByName(column_name).type;
 
             /// If constant is not Null, we can assume it's type is not Nullable as well.
@@ -225,9 +225,9 @@ bool RPNBuilderTreeNode::tryGetConstant(Field & output_value, DataTypePtr & outp
             isColumnConst(*block_with_constants.getByName(column_name).column))
         {
             /// An expression which is dependent on constants only
-            const auto & expr_info = block_with_constants.getByName(column_name);
+            const auto & constant_column = block_with_constants.getByName(column_name);
-            output_value = (*expr_info.column)[0];
+            output_value = (*constant_column.column)[0];
-            output_type = expr_info.type;
+            output_type = constant_column.type;
 
             if (!output_value.isNull())
                 output_type = removeNullable(output_type);
@@ -260,13 +260,13 @@ ConstSetPtr tryGetSetFromDAGNode(const ActionsDAG::Node * dag_node)
     if (!dag_node->column)
         return {};
 
-    const IColumn * col = dag_node->column.get();
+    const IColumn * column = dag_node->column.get();
-    if (const auto * col_const = typeid_cast<const ColumnConst *>(col))
+    if (const auto * column_const = typeid_cast<const ColumnConst *>(column))
-        col = &col_const->getDataColumn();
+        column = &column_const->getDataColumn();
 
-    if (const auto * col_set = typeid_cast<const ColumnSet *>(col))
+    if (const auto * column_set = typeid_cast<const ColumnSet *>(column))
     {
-        auto set = col_set->getData();
+        auto set = column_set->getData();
 
         if (set->isCreated())
             return set;
@@ -369,6 +369,17 @@ RPNBuilderFunctionTreeNode RPNBuilderTreeNode::toFunctionNode() const
         return RPNBuilderFunctionTreeNode(this->dag_node, tree_context);
 }
 
+std::optional<RPNBuilderFunctionTreeNode> RPNBuilderTreeNode::toFunctionNodeOrNull() const
+{
+    if (!isFunction())
+        return {};
+
+    if (this->ast_node)
+        return RPNBuilderFunctionTreeNode(this->ast_node, tree_context);
+    else
+        return RPNBuilderFunctionTreeNode(this->dag_node, tree_context);
+}
+
 std::string RPNBuilderFunctionTreeNode::getFunctionName() const
 {
     if (ast_node)

src/Storages/MergeTree/RPNBuilder.h

@@ -12,7 +12,7 @@ namespace DB
 
 /** Context of RPNBuilderTree.
   *
-  * For AST tree context, precalculated block with constansts and prepared sets are required for index analysis.
+  * For AST tree context, precalculated block with constants and prepared sets are required for index analysis.
   * For DAG tree precalculated block with constants and prepared sets are not required, because constants and sets already
   * calculated inside COLUMN actions dag node.
   */
@@ -112,6 +112,9 @@ public:
       */
     RPNBuilderFunctionTreeNode toFunctionNode() const;
 
+    /// Convert node to function node or null optional
+    std::optional<RPNBuilderFunctionTreeNode> toFunctionNodeOrNull() const;
+
     /// Get tree context
     const RPNBuilderTreeContext & getTreeContext() const
     {
@@ -136,16 +139,16 @@ protected:
 class RPNBuilderFunctionTreeNode : public RPNBuilderTreeNode
 {
 public:
+    using RPNBuilderTreeNode::RPNBuilderTreeNode;
+
     /// Get function name
     std::string getFunctionName() const;
 
     /// Get function arguments size
     size_t getArgumentsSize() const;
 
-    /// Get argument at index
+    /// Get function argument at index
     RPNBuilderTreeNode getArgumentAt(size_t index) const;
-
-    using RPNBuilderTreeNode::RPNBuilderTreeNode;
 };
 
 /** RPN Builder build stack of reverse polish notation elements (RPNElements) required for index analysis.
@@ -240,9 +243,10 @@ private:
 
     bool extractLogicalOperatorFromTree(const RPNBuilderFunctionTreeNode & function_node, RPNElement & out)
     {
-        /// Functions AND, OR, NOT.
+        /** Functions AND, OR, NOT.
-        /// Also a special function `indexHint` - works as if instead of calling a function there are just parentheses
+          * Also a special function `indexHint` - works as if instead of calling a function there are just parentheses
-        /// (or, the same thing - calling the function `and` from one argument).
+          * (or, the same thing - calling the function `and` from one argument).
+          */
 
         auto function_name = function_node.getFunctionName();
         if (function_name == "not")