From 726e22ea1dd2b841e7535dfb40294a1c2acfb034 Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Thu, 3 Jun 2021 16:26:04 +0300 Subject: [PATCH 1/7] Always return false for canConstantBeWrappedByMonotonicFunctions. --- src/Storages/MergeTree/KeyCondition.cpp | 124 ++++++++++++------------ 1 file changed, 63 insertions(+), 61 deletions(-) diff --git a/src/Storages/MergeTree/KeyCondition.cpp b/src/Storages/MergeTree/KeyCondition.cpp index 268c45c305f..d71176e9ad9 100644 --- a/src/Storages/MergeTree/KeyCondition.cpp +++ b/src/Storages/MergeTree/KeyCondition.cpp @@ -618,76 +618,78 @@ bool KeyCondition::canConstantBeWrapped(const ASTPtr & node, const String & expr } bool KeyCondition::canConstantBeWrappedByMonotonicFunctions( - const ASTPtr & node, - size_t & out_key_column_num, - DataTypePtr & out_key_column_type, - Field & out_value, - DataTypePtr & out_type) + const ASTPtr & node [[maybe_unused]], + size_t & out_key_column_num [[maybe_unused]], + DataTypePtr & out_key_column_type [[maybe_unused]], + Field & out_value [[maybe_unused]], + DataTypePtr & out_type [[maybe_unused]]) { - // Constant expr should use alias names if any - String passed_expr_name = node->getColumnName(); - String expr_name; - if (!canConstantBeWrapped(node, passed_expr_name, expr_name)) - return false; + return false; - const auto & sample_block = key_expr->getSampleBlock(); + // // Constant expr should use alias names if any + // String passed_expr_name = node->getColumnName(); + // String expr_name; + // if (!canConstantBeWrapped(node, passed_expr_name, expr_name)) + // return false; - /// TODO Nullable index is not yet landed. - if (out_value.isNull()) - return false; + // const auto & sample_block = key_expr->getSampleBlock(); - bool found_transformation = false; - auto input_column = sample_block.getByName(expr_name); - auto const_column = out_type->createColumnConst(1, out_value); - auto const_value = (*castColumn({const_column, out_type, "c"}, input_column.type))[0]; - auto const_type = input_column.type; - for (const auto & action : key_expr->getActions()) - { - /** The key functional expression constraint may be inferred from a plain column in the expression. - * For example, if the key contains `toStartOfHour(Timestamp)` and query contains `WHERE Timestamp >= now()`, - * it can be assumed that if `toStartOfHour()` is monotonic on [now(), inf), the `toStartOfHour(Timestamp) >= toStartOfHour(now())` - * condition also holds, so the index may be used to select only parts satisfying this condition. - * - * To check the assumption, we'd need to assert that the inverse function to this transformation is also monotonic, however the - * inversion isn't exported (or even viable for not strictly monotonic functions such as `toStartOfHour()`). - * Instead, we can qualify only functions that do not transform the range (for example rounding), - * which while not strictly monotonic, are monotonic everywhere on the input range. - */ - const auto & children = action.node->children; - if (action.node->type == ActionsDAG::ActionType::FUNCTION - && children.size() == 1 - && children[0]->result_name == expr_name) - { - if (!action.node->function_base->hasInformationAboutMonotonicity()) - return false; + // /// TODO Nullable index is not yet landed. + // if (out_value.isNull()) + // return false; - /// Range is irrelevant in this case. - IFunction::Monotonicity monotonicity = action.node->function_base->getMonotonicityForRange(*const_type, Field(), Field()); - if (!monotonicity.is_always_monotonic) - return false; + // bool found_transformation = false; + // auto input_column = sample_block.getByName(expr_name); + // auto const_column = out_type->createColumnConst(1, out_value); + // auto const_value = (*castColumn({const_column, out_type, "c"}, input_column.type))[0]; + // auto const_type = input_column.type; + // for (const auto & action : key_expr->getActions()) + // { + // /** The key functional expression constraint may be inferred from a plain column in the expression. + // * For example, if the key contains `toStartOfHour(Timestamp)` and query contains `WHERE Timestamp >= now()`, + // * it can be assumed that if `toStartOfHour()` is monotonic on [now(), inf), the `toStartOfHour(Timestamp) >= toStartOfHour(now())` + // * condition also holds, so the index may be used to select only parts satisfying this condition. + // * + // * To check the assumption, we'd need to assert that the inverse function to this transformation is also monotonic, however the + // * inversion isn't exported (or even viable for not strictly monotonic functions such as `toStartOfHour()`). + // * Instead, we can qualify only functions that do not transform the range (for example rounding), + // * which while not strictly monotonic, are monotonic everywhere on the input range. + // */ + // const auto & children = action.node->children; + // if (action.node->type == ActionsDAG::ActionType::FUNCTION + // && children.size() == 1 + // && children[0]->result_name == expr_name) + // { + // if (!action.node->function_base->hasInformationAboutMonotonicity()) + // return false; - /// Apply the next transformation step. - std::tie(const_value, const_type) = applyFunctionForFieldOfUnknownType( - action.node->function_builder, - const_type, const_value); + // /// Range is irrelevant in this case. + // IFunction::Monotonicity monotonicity = action.node->function_base->getMonotonicityForRange(*const_type, Field(), Field()); + // if (!monotonicity.is_always_monotonic) + // return false; - expr_name = action.node->result_name; + // /// Apply the next transformation step. + // std::tie(const_value, const_type) = applyFunctionForFieldOfUnknownType( + // action.node->function_builder, + // const_type, const_value); - /// Transformation results in a key expression, accept. - auto it = key_columns.find(expr_name); - if (key_columns.end() != it) - { - out_key_column_num = it->second; - out_key_column_type = sample_block.getByName(it->first).type; - out_value = const_value; - out_type = const_type; - found_transformation = true; - break; - } - } - } + // expr_name = action.node->result_name; - return found_transformation; + // /// Transformation results in a key expression, accept. + // auto it = key_columns.find(expr_name); + // if (key_columns.end() != it) + // { + // out_key_column_num = it->second; + // out_key_column_type = sample_block.getByName(it->first).type; + // out_value = const_value; + // out_type = const_type; + // found_transformation = true; + // break; + // } + // } + // } + + // return found_transformation; } /// Looking for possible transformation of `column = constant` into `partition_expr = function(constant)` From 3e5a1cda6009be10a6e3d5d0f784ffefed7e5512 Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Thu, 3 Jun 2021 17:44:59 +0300 Subject: [PATCH 2/7] Revert --- src/Storages/MergeTree/KeyCondition.cpp | 128 +++++++++++++----------- 1 file changed, 68 insertions(+), 60 deletions(-) diff --git a/src/Storages/MergeTree/KeyCondition.cpp b/src/Storages/MergeTree/KeyCondition.cpp index d71176e9ad9..46d7eaac00b 100644 --- a/src/Storages/MergeTree/KeyCondition.cpp +++ b/src/Storages/MergeTree/KeyCondition.cpp @@ -451,6 +451,8 @@ bool KeyCondition::getConstant(const ASTPtr & expr, Block & block_with_constants // Constant expr should use alias names if any String column_name = expr->getColumnName(); + std::cerr << "========= get const for : " << column_name << "\n" << block_with_constants.dumpStructure() << std::endl; + if (const auto * lit = expr->as()) { /// By default block_with_constants has only one column named "_dummy". @@ -594,11 +596,13 @@ void KeyCondition::traverseAST(const ASTPtr & node, ContextPtr context, Block & bool KeyCondition::canConstantBeWrapped(const ASTPtr & node, const String & expr_name, String & result_expr_name) { - const auto & sample_block = key_expr->getSampleBlock(); + NameSet names; + for (const auto & action : key_expr->getActions()) + names.insert(action.node->result_name); /// sample_block from key_expr cannot contain modulo and moduloLegacy at the same time. /// For partition key it is always moduloLegacy. - if (sample_block.has(expr_name)) + if (names.count(expr_name)) { result_expr_name = expr_name; } @@ -608,7 +612,7 @@ bool KeyCondition::canConstantBeWrapped(const ASTPtr & node, const String & expr KeyDescription::moduloToModuloLegacyRecursive(adjusted_ast); String adjusted_expr_name = adjusted_ast->getColumnName(); - if (!sample_block.has(adjusted_expr_name)) + if (!names.count(adjusted_expr_name)) return false; result_expr_name = adjusted_expr_name; @@ -624,72 +628,76 @@ bool KeyCondition::canConstantBeWrappedByMonotonicFunctions( Field & out_value [[maybe_unused]], DataTypePtr & out_type [[maybe_unused]]) { - return false; + // Constant expr should use alias names if any + String passed_expr_name = node->getColumnName(); + String expr_name; + if (!canConstantBeWrapped(node, passed_expr_name, expr_name)) + return false; - // // Constant expr should use alias names if any - // String passed_expr_name = node->getColumnName(); - // String expr_name; - // if (!canConstantBeWrapped(node, passed_expr_name, expr_name)) - // return false; + //const auto & sample_block = key_expr->getSampleBlock(); - // const auto & sample_block = key_expr->getSampleBlock(); + /// TODO Nullable index is not yet landed. + if (out_value.isNull()) + return false; - // /// TODO Nullable index is not yet landed. - // if (out_value.isNull()) - // return false; + bool found_transformation = false; - // bool found_transformation = false; - // auto input_column = sample_block.getByName(expr_name); - // auto const_column = out_type->createColumnConst(1, out_value); - // auto const_value = (*castColumn({const_column, out_type, "c"}, input_column.type))[0]; - // auto const_type = input_column.type; - // for (const auto & action : key_expr->getActions()) - // { - // /** The key functional expression constraint may be inferred from a plain column in the expression. - // * For example, if the key contains `toStartOfHour(Timestamp)` and query contains `WHERE Timestamp >= now()`, - // * it can be assumed that if `toStartOfHour()` is monotonic on [now(), inf), the `toStartOfHour(Timestamp) >= toStartOfHour(now())` - // * condition also holds, so the index may be used to select only parts satisfying this condition. - // * - // * To check the assumption, we'd need to assert that the inverse function to this transformation is also monotonic, however the - // * inversion isn't exported (or even viable for not strictly monotonic functions such as `toStartOfHour()`). - // * Instead, we can qualify only functions that do not transform the range (for example rounding), - // * which while not strictly monotonic, are monotonic everywhere on the input range. - // */ - // const auto & children = action.node->children; - // if (action.node->type == ActionsDAG::ActionType::FUNCTION - // && children.size() == 1 - // && children[0]->result_name == expr_name) - // { - // if (!action.node->function_base->hasInformationAboutMonotonicity()) - // return false; + DataTypePtr input_type; + for (const auto & action : key_expr->getActions()) + if (action.node->result_name == expr_name) + input_type = action.node->result_type; - // /// Range is irrelevant in this case. - // IFunction::Monotonicity monotonicity = action.node->function_base->getMonotonicityForRange(*const_type, Field(), Field()); - // if (!monotonicity.is_always_monotonic) - // return false; + //auto input_column = sample_block.getByName(expr_name); + auto const_column = out_type->createColumnConst(1, out_value); + auto const_value = (*castColumn({const_column, out_type, "c"}, input_type))[0]; + auto const_type = input_type; + for (const auto & action : key_expr->getActions()) + { + /** The key functional expression constraint may be inferred from a plain column in the expression. + * For example, if the key contains `toStartOfHour(Timestamp)` and query contains `WHERE Timestamp >= now()`, + * it can be assumed that if `toStartOfHour()` is monotonic on [now(), inf), the `toStartOfHour(Timestamp) >= toStartOfHour(now())` + * condition also holds, so the index may be used to select only parts satisfying this condition. + * + * To check the assumption, we'd need to assert that the inverse function to this transformation is also monotonic, however the + * inversion isn't exported (or even viable for not strictly monotonic functions such as `toStartOfHour()`). + * Instead, we can qualify only functions that do not transform the range (for example rounding), + * which while not strictly monotonic, are monotonic everywhere on the input range. + */ + const auto & children = action.node->children; + if (action.node->type == ActionsDAG::ActionType::FUNCTION + && children.size() == 1 + && children[0]->result_name == expr_name) + { + if (!action.node->function_base->hasInformationAboutMonotonicity()) + return false; - // /// Apply the next transformation step. - // std::tie(const_value, const_type) = applyFunctionForFieldOfUnknownType( - // action.node->function_builder, - // const_type, const_value); + /// Range is irrelevant in this case. + IFunction::Monotonicity monotonicity = action.node->function_base->getMonotonicityForRange(*const_type, Field(), Field()); + if (!monotonicity.is_always_monotonic) + return false; - // expr_name = action.node->result_name; + /// Apply the next transformation step. + std::tie(const_value, const_type) = applyFunctionForFieldOfUnknownType( + action.node->function_builder, + const_type, const_value); - // /// Transformation results in a key expression, accept. - // auto it = key_columns.find(expr_name); - // if (key_columns.end() != it) - // { - // out_key_column_num = it->second; - // out_key_column_type = sample_block.getByName(it->first).type; - // out_value = const_value; - // out_type = const_type; - // found_transformation = true; - // break; - // } - // } - // } + expr_name = action.node->result_name; - // return found_transformation; + /// Transformation results in a key expression, accept. + auto it = key_columns.find(expr_name); + if (key_columns.end() != it) + { + out_key_column_num = it->second; + out_key_column_type = action.node->result_type; //sample_block.getByName(it->first).type; + out_value = const_value; + out_type = const_type; + found_transformation = true; + break; + } + } + } + + return found_transformation; } /// Looking for possible transformation of `column = constant` into `partition_expr = function(constant)` From 397f6133e0fd5eb04ff3619090acdcc302a26e8d Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Fri, 4 Jun 2021 20:56:56 +0300 Subject: [PATCH 3/7] Refactor canConstantBeWrappedByMonotonicFunctions function. --- src/Storages/MergeTree/KeyCondition.cpp | 144 ++++++++++++++---------- 1 file changed, 87 insertions(+), 57 deletions(-) diff --git a/src/Storages/MergeTree/KeyCondition.cpp b/src/Storages/MergeTree/KeyCondition.cpp index 46d7eaac00b..178b54e9362 100644 --- a/src/Storages/MergeTree/KeyCondition.cpp +++ b/src/Storages/MergeTree/KeyCondition.cpp @@ -451,7 +451,7 @@ bool KeyCondition::getConstant(const ASTPtr & expr, Block & block_with_constants // Constant expr should use alias names if any String column_name = expr->getColumnName(); - std::cerr << "========= get const for : " << column_name << "\n" << block_with_constants.dumpStructure() << std::endl; + // std::cerr << "========= get const for : " << column_name << "\n" << block_with_constants.dumpStructure() << std::endl; if (const auto * lit = expr->as()) { @@ -495,17 +495,21 @@ static Field applyFunctionForField( /// The case when arguments may have types different than in the primary key. static std::pair applyFunctionForFieldOfUnknownType( - const FunctionOverloadResolverPtr & func, + const FunctionBasePtr & func, const DataTypePtr & arg_type, const Field & arg_value) { ColumnsWithTypeAndName arguments{{ arg_type->createColumnConst(1, arg_value), arg_type, "x" }}; - FunctionBasePtr func_base = func->build(arguments); + // std::cerr << ">>>>> applaying func " << func->getName() << " to column " << arguments[0].dumpStructure() << std::endl; - DataTypePtr return_type = func_base->getResultType(); + //FunctionBasePtr func_base = func->build(arguments); - auto col = func_base->execute(arguments, return_type, 1); + DataTypePtr return_type = func->getResultType(); + + auto col = func->execute(arguments, return_type, 1); + + // std::cerr << ">>>>> got " << ColumnWithTypeAndName(col, return_type, "").dumpStructure() << std::endl; Field result = (*col)[0]; @@ -628,76 +632,102 @@ bool KeyCondition::canConstantBeWrappedByMonotonicFunctions( Field & out_value [[maybe_unused]], DataTypePtr & out_type [[maybe_unused]]) { + // std::cerr << "=========== canConstantBeWrappedByMonotonicFunctions for " << node->getColumnName() << std::endl; + // std::cerr << key_expr->dumpActions() << std::endl; + // Constant expr should use alias names if any - String passed_expr_name = node->getColumnName(); + String passed_expr_name = node->getColumnNameWithoutAlias(); String expr_name; if (!canConstantBeWrapped(node, passed_expr_name, expr_name)) return false; - //const auto & sample_block = key_expr->getSampleBlock(); - /// TODO Nullable index is not yet landed. if (out_value.isNull()) return false; - bool found_transformation = false; + const auto & sample_block = key_expr->getSampleBlock(); - DataTypePtr input_type; - for (const auto & action : key_expr->getActions()) - if (action.node->result_name == expr_name) - input_type = action.node->result_type; - - //auto input_column = sample_block.getByName(expr_name); - auto const_column = out_type->createColumnConst(1, out_value); - auto const_value = (*castColumn({const_column, out_type, "c"}, input_type))[0]; - auto const_type = input_type; - for (const auto & action : key_expr->getActions()) + /** The key functional expression constraint may be inferred from a plain column in the expression. + * For example, if the key contains `toStartOfHour(Timestamp)` and query contains `WHERE Timestamp >= now()`, + * it can be assumed that if `toStartOfHour()` is monotonic on [now(), inf), the `toStartOfHour(Timestamp) >= toStartOfHour(now())` + * condition also holds, so the index may be used to select only parts satisfying this condition. + * + * To check the assumption, we'd need to assert that the inverse function to this transformation is also monotonic, however the + * inversion isn't exported (or even viable for not strictly monotonic functions such as `toStartOfHour()`). + * Instead, we can qualify only functions that do not transform the range (for example rounding), + * which while not strictly monotonic, are monotonic everywhere on the input range. + */ + for (const auto & dag_node : key_expr->getNodes()) { - /** The key functional expression constraint may be inferred from a plain column in the expression. - * For example, if the key contains `toStartOfHour(Timestamp)` and query contains `WHERE Timestamp >= now()`, - * it can be assumed that if `toStartOfHour()` is monotonic on [now(), inf), the `toStartOfHour(Timestamp) >= toStartOfHour(now())` - * condition also holds, so the index may be used to select only parts satisfying this condition. - * - * To check the assumption, we'd need to assert that the inverse function to this transformation is also monotonic, however the - * inversion isn't exported (or even viable for not strictly monotonic functions such as `toStartOfHour()`). - * Instead, we can qualify only functions that do not transform the range (for example rounding), - * which while not strictly monotonic, are monotonic everywhere on the input range. - */ - const auto & children = action.node->children; - if (action.node->type == ActionsDAG::ActionType::FUNCTION - && children.size() == 1 - && children[0]->result_name == expr_name) + auto it = key_columns.find(dag_node.result_name); + if (it != key_columns.end()) { - if (!action.node->function_base->hasInformationAboutMonotonicity()) - return false; + std::stack chain; - /// Range is irrelevant in this case. - IFunction::Monotonicity monotonicity = action.node->function_base->getMonotonicityForRange(*const_type, Field(), Field()); - if (!monotonicity.is_always_monotonic) - return false; + const auto * cur_node = &dag_node; + bool is_valid_chain = true; - /// Apply the next transformation step. - std::tie(const_value, const_type) = applyFunctionForFieldOfUnknownType( - action.node->function_builder, - const_type, const_value); - - expr_name = action.node->result_name; - - /// Transformation results in a key expression, accept. - auto it = key_columns.find(expr_name); - if (key_columns.end() != it) + while (is_valid_chain) { + if (cur_node->result_name == expr_name) + break; + + chain.push(cur_node); + + if (cur_node->type == ActionsDAG::ActionType::FUNCTION && cur_node->children.size() == 1) + { + + if (!cur_node->function_base->hasInformationAboutMonotonicity()) + { + is_valid_chain = false; + break; + } + + cur_node = cur_node->children.front(); + } + else if (cur_node->type == ActionsDAG::ActionType::ALIAS) + cur_node = cur_node->children.front(); + else + is_valid_chain = false; + } + + if (is_valid_chain) + { + /// Here we cast constant to the input type. + /// It is not clear, why this works in general. + /// I can imagine the case when expression like `column < const` is legal, + /// but `type(column)` and `type(const)` are of different types, + /// and const cannot be casted to column type. + /// (There could be `superType(type(column), type(const))` which is used for comparison). + /// + /// However, looks like this case newer happenes (I could not find such). + /// Let's assume that any two comparable types are castable to each other. + auto const_type = cur_node->result_type; + auto const_column = out_type->createColumnConst(1, out_value); + auto const_value = (*castColumn({const_column, out_type, ""}, const_type))[0]; + + while (!chain.empty()) + { + const auto * func = chain.top(); + chain.pop(); + + if (func->type != ActionsDAG::ActionType::FUNCTION) + continue; + + std::tie(const_value, const_type) = + applyFunctionForFieldOfUnknownType(func->function_base, const_type, const_value); + } + out_key_column_num = it->second; - out_key_column_type = action.node->result_type; //sample_block.getByName(it->first).type; + out_key_column_type = sample_block.getByName(it->first).type; out_value = const_value; out_type = const_type; - found_transformation = true; - break; + return true; } } } - return found_transformation; + return false; } /// Looking for possible transformation of `column = constant` into `partition_expr = function(constant)` @@ -763,10 +793,10 @@ bool KeyCondition::canConstantBeWrappedByFunctions( if (is_valid_chain) { - auto input_column = sample_block.getByName(expr_name); + /// This CAST is the same as in canConstantBeWrappedByMonotonicFunctions (see comment). + auto const_type = cur_node->result_type; auto const_column = out_type->createColumnConst(1, out_value); - auto const_value = (*castColumn({const_column, out_type, "c"}, input_column.type))[0]; - auto const_type = input_column.type; + auto const_value = (*castColumn({const_column, out_type, ""}, const_type))[0]; while (!chain.empty()) { @@ -778,7 +808,7 @@ bool KeyCondition::canConstantBeWrappedByFunctions( if (func->children.size() == 1) { - std::tie(const_value, const_type) = applyFunctionForFieldOfUnknownType(func->function_builder, const_type, const_value); + std::tie(const_value, const_type) = applyFunctionForFieldOfUnknownType(func->function_base, const_type, const_value); } else if (func->children.size() == 2) { From 0d2a839ca4a3ab4b81c1cfccf406bb7643ba2c81 Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Mon, 7 Jun 2021 16:41:40 +0300 Subject: [PATCH 4/7] Fix tests. --- src/Storages/MergeTree/KeyCondition.cpp | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/src/Storages/MergeTree/KeyCondition.cpp b/src/Storages/MergeTree/KeyCondition.cpp index 178b54e9362..afbaab25687 100644 --- a/src/Storages/MergeTree/KeyCondition.cpp +++ b/src/Storages/MergeTree/KeyCondition.cpp @@ -602,7 +602,12 @@ bool KeyCondition::canConstantBeWrapped(const ASTPtr & node, const String & expr { NameSet names; for (const auto & action : key_expr->getActions()) + { names.insert(action.node->result_name); + // std::cerr << "-- added " << action.node->result_name << std::endl; + } + + // std::cerr << key_expr->getSampleBlock().dumpStructure() << std::endl; /// sample_block from key_expr cannot contain modulo and moduloLegacy at the same time. /// For partition key it is always moduloLegacy. @@ -676,14 +681,20 @@ bool KeyCondition::canConstantBeWrappedByMonotonicFunctions( if (cur_node->type == ActionsDAG::ActionType::FUNCTION && cur_node->children.size() == 1) { + const auto * next_node = cur_node->children.front(); if (!cur_node->function_base->hasInformationAboutMonotonicity()) - { is_valid_chain = false; - break; + else + { + /// Range is irrelevant in this case. + auto monotonicity = cur_node->function_base->getMonotonicityForRange( + *next_node->result_type, Field(), Field()); + if (!monotonicity.is_always_monotonic) + is_valid_chain = false; } - cur_node = cur_node->children.front(); + cur_node = next_node; } else if (cur_node->type == ActionsDAG::ActionType::ALIAS) cur_node = cur_node->children.front(); @@ -691,7 +702,7 @@ bool KeyCondition::canConstantBeWrappedByMonotonicFunctions( is_valid_chain = false; } - if (is_valid_chain) + if (is_valid_chain && !chain.empty()) { /// Here we cast constant to the input type. /// It is not clear, why this works in general. @@ -703,6 +714,7 @@ bool KeyCondition::canConstantBeWrappedByMonotonicFunctions( /// However, looks like this case newer happenes (I could not find such). /// Let's assume that any two comparable types are castable to each other. auto const_type = cur_node->result_type; + // std::cerr << "==== Using type (mon) for expr " << expr_name << " " << const_type->getName() << std::endl; auto const_column = out_type->createColumnConst(1, out_value); auto const_value = (*castColumn({const_column, out_type, ""}, const_type))[0]; @@ -734,6 +746,8 @@ bool KeyCondition::canConstantBeWrappedByMonotonicFunctions( bool KeyCondition::canConstantBeWrappedByFunctions( const ASTPtr & ast, size_t & out_key_column_num, DataTypePtr & out_key_column_type, Field & out_value, DataTypePtr & out_type) { + + // std::cerr << "=========== canConstantBeWrappedByMonotonicFunctions for " << ast->getColumnName() << std::endl; // Constant expr should use alias names if any String passed_expr_name = ast->getColumnName(); String expr_name; @@ -795,6 +809,7 @@ bool KeyCondition::canConstantBeWrappedByFunctions( { /// This CAST is the same as in canConstantBeWrappedByMonotonicFunctions (see comment). auto const_type = cur_node->result_type; + // std::cerr << "==== Using type for expr " << expr_name << " " << const_type->getName() << std::endl; auto const_column = out_type->createColumnConst(1, out_value); auto const_value = (*castColumn({const_column, out_type, ""}, const_type))[0]; @@ -806,6 +821,8 @@ bool KeyCondition::canConstantBeWrappedByFunctions( if (func->type != ActionsDAG::ActionType::FUNCTION) continue; + // std::cerr << ".. chain func " << func->function_base->getName() << ' ' << func->function_builder->getName() << std::endl; + if (func->children.size() == 1) { std::tie(const_value, const_type) = applyFunctionForFieldOfUnknownType(func->function_base, const_type, const_value); From c4832fd3c02bafde0baf0863cbcd7f0acbab9a21 Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Mon, 7 Jun 2021 21:24:32 +0300 Subject: [PATCH 5/7] Added test. --- src/Storages/MergeTree/KeyCondition.cpp | 22 +------------ ...89_key_condition_function_chains.reference | 7 ++++ .../01889_key_condition_function_chains.sql | 33 +++++++++++++++++++ 3 files changed, 41 insertions(+), 21 deletions(-) create mode 100644 tests/queries/0_stateless/01889_key_condition_function_chains.reference create mode 100644 tests/queries/0_stateless/01889_key_condition_function_chains.sql diff --git a/src/Storages/MergeTree/KeyCondition.cpp b/src/Storages/MergeTree/KeyCondition.cpp index afbaab25687..04eae5c04f5 100644 --- a/src/Storages/MergeTree/KeyCondition.cpp +++ b/src/Storages/MergeTree/KeyCondition.cpp @@ -500,17 +500,10 @@ static std::pair applyFunctionForFieldOfUnknownType( const Field & arg_value) { ColumnsWithTypeAndName arguments{{ arg_type->createColumnConst(1, arg_value), arg_type, "x" }}; - - // std::cerr << ">>>>> applaying func " << func->getName() << " to column " << arguments[0].dumpStructure() << std::endl; - - //FunctionBasePtr func_base = func->build(arguments); - DataTypePtr return_type = func->getResultType(); auto col = func->execute(arguments, return_type, 1); - // std::cerr << ">>>>> got " << ColumnWithTypeAndName(col, return_type, "").dumpStructure() << std::endl; - Field result = (*col)[0]; return {std::move(result), std::move(return_type)}; @@ -602,12 +595,7 @@ bool KeyCondition::canConstantBeWrapped(const ASTPtr & node, const String & expr { NameSet names; for (const auto & action : key_expr->getActions()) - { names.insert(action.node->result_name); - // std::cerr << "-- added " << action.node->result_name << std::endl; - } - - // std::cerr << key_expr->getSampleBlock().dumpStructure() << std::endl; /// sample_block from key_expr cannot contain modulo and moduloLegacy at the same time. /// For partition key it is always moduloLegacy. @@ -637,8 +625,6 @@ bool KeyCondition::canConstantBeWrappedByMonotonicFunctions( Field & out_value [[maybe_unused]], DataTypePtr & out_type [[maybe_unused]]) { - // std::cerr << "=========== canConstantBeWrappedByMonotonicFunctions for " << node->getColumnName() << std::endl; - // std::cerr << key_expr->dumpActions() << std::endl; // Constant expr should use alias names if any String passed_expr_name = node->getColumnNameWithoutAlias(); @@ -714,7 +700,6 @@ bool KeyCondition::canConstantBeWrappedByMonotonicFunctions( /// However, looks like this case newer happenes (I could not find such). /// Let's assume that any two comparable types are castable to each other. auto const_type = cur_node->result_type; - // std::cerr << "==== Using type (mon) for expr " << expr_name << " " << const_type->getName() << std::endl; auto const_column = out_type->createColumnConst(1, out_value); auto const_value = (*castColumn({const_column, out_type, ""}, const_type))[0]; @@ -746,10 +731,8 @@ bool KeyCondition::canConstantBeWrappedByMonotonicFunctions( bool KeyCondition::canConstantBeWrappedByFunctions( const ASTPtr & ast, size_t & out_key_column_num, DataTypePtr & out_key_column_type, Field & out_value, DataTypePtr & out_type) { - - // std::cerr << "=========== canConstantBeWrappedByMonotonicFunctions for " << ast->getColumnName() << std::endl; // Constant expr should use alias names if any - String passed_expr_name = ast->getColumnName(); + String passed_expr_name = ast->getColumnNameWithoutAlias(); String expr_name; if (!canConstantBeWrapped(ast, passed_expr_name, expr_name)) return false; @@ -809,7 +792,6 @@ bool KeyCondition::canConstantBeWrappedByFunctions( { /// This CAST is the same as in canConstantBeWrappedByMonotonicFunctions (see comment). auto const_type = cur_node->result_type; - // std::cerr << "==== Using type for expr " << expr_name << " " << const_type->getName() << std::endl; auto const_column = out_type->createColumnConst(1, out_value); auto const_value = (*castColumn({const_column, out_type, ""}, const_type))[0]; @@ -821,8 +803,6 @@ bool KeyCondition::canConstantBeWrappedByFunctions( if (func->type != ActionsDAG::ActionType::FUNCTION) continue; - // std::cerr << ".. chain func " << func->function_base->getName() << ' ' << func->function_builder->getName() << std::endl; - if (func->children.size() == 1) { std::tie(const_value, const_type) = applyFunctionForFieldOfUnknownType(func->function_base, const_type, const_value); diff --git a/tests/queries/0_stateless/01889_key_condition_function_chains.reference b/tests/queries/0_stateless/01889_key_condition_function_chains.reference new file mode 100644 index 00000000000..79185ae7f93 --- /dev/null +++ b/tests/queries/0_stateless/01889_key_condition_function_chains.reference @@ -0,0 +1,7 @@ +2020-02-02 01:01:01 +2020-02-02 01:01:01 +2020-02-02 01:01:01 +2020-02-02 01:01:01 +1 1 +1 1 +1 1 diff --git a/tests/queries/0_stateless/01889_key_condition_function_chains.sql b/tests/queries/0_stateless/01889_key_condition_function_chains.sql new file mode 100644 index 00000000000..5afececf1af --- /dev/null +++ b/tests/queries/0_stateless/01889_key_condition_function_chains.sql @@ -0,0 +1,33 @@ +set force_primary_key=1; + +drop table if exists tab; +create table tab (t DateTime) engine = MergeTree order by toStartOfDay(t); +insert into tab values ('2020-02-02 01:01:01'); +select t from tab where t > '2020-01-01 01:01:01'; +with t as s select t from tab where s > '2020-01-01 01:01:01'; + +drop table if exists tab; +create table tab (t DateTime) engine = MergeTree order by toStartOfDay(t + 1); +insert into tab values ('2020-02-02 01:01:01'); +select t from tab where t + 1 > '2020-01-01 01:01:01'; +with t + 1 as s select t from tab where s > '2020-01-01 01:01:01'; + + +set force_primary_key = 0; +set force_index_by_date=1; + +drop table if exists tab; +create table tab (x Int32, y Int32) engine = MergeTree partition by x + y order by tuple(); +insert into tab values (1, 1), (2, 2); +select x, y from tab where (x + y) = 2; +with x + y as s select x, y from tab where s = 2; +-- with x as s select x, y from tab where s + y = 2; + +drop table if exists tab; +create table tab (x Int32, y Int32) engine = MergeTree partition by ((x + y) + 1) * 2 order by tuple(); +insert into tab values (1, 1), (2, 2); +select x, y from tab where (x + y) + 1 = 3; +-- with x + y as s select x, y from tab where s + 1 = 3; + +set force_index_by_date=0; +drop table if exists tab; From b8b7f7b33d771e8217ce5435d2c14817e9a56acb Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Tue, 8 Jun 2021 15:02:39 +0300 Subject: [PATCH 6/7] Fix test. --- ...sion_versioned_collapsing_merge_tree_zookeeper.reference | 6 +++--- ...er_version_versioned_collapsing_merge_tree_zookeeper.sql | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/queries/0_stateless/01511_alter_version_versioned_collapsing_merge_tree_zookeeper.reference b/tests/queries/0_stateless/01511_alter_version_versioned_collapsing_merge_tree_zookeeper.reference index c6cd81a4aca..83a8162e5f4 100644 --- a/tests/queries/0_stateless/01511_alter_version_versioned_collapsing_merge_tree_zookeeper.reference +++ b/tests/queries/0_stateless/01511_alter_version_versioned_collapsing_merge_tree_zookeeper.reference @@ -1,9 +1,9 @@ 1 1 1 -1 2 2 2 -1 -CREATE TABLE default.table_with_version_replicated_1\n(\n `key` UInt64,\n `value` String,\n `version` UInt8,\n `sign` Int8\n)\nENGINE = ReplicatedVersionedCollapsingMergeTree(\'/clickhouse/test_01511/t\', \'1\', sign, version)\nORDER BY key\nSETTINGS index_granularity = 8192 +CREATE TABLE default.table_with_version_replicated_1\n(\n `key` UInt64,\n `value` String,\n `version` UInt8,\n `sign` Int8\n)\nENGINE = ReplicatedVersionedCollapsingMergeTree(\'/clickhouse/default/test_01511/t\', \'1\', sign, version)\nORDER BY key\nSETTINGS index_granularity = 8192 1 1 1 -1 2 2 2 -1 -CREATE TABLE default.table_with_version_replicated_1\n(\n `key` UInt64,\n `value` String,\n `version` UInt32,\n `sign` Int8\n)\nENGINE = ReplicatedVersionedCollapsingMergeTree(\'/clickhouse/test_01511/t\', \'1\', sign, version)\nORDER BY key\nSETTINGS index_granularity = 8192 +CREATE TABLE default.table_with_version_replicated_1\n(\n `key` UInt64,\n `value` String,\n `version` UInt32,\n `sign` Int8\n)\nENGINE = ReplicatedVersionedCollapsingMergeTree(\'/clickhouse/default/test_01511/t\', \'1\', sign, version)\nORDER BY key\nSETTINGS index_granularity = 8192 1 1 2 1 2 2 2 -1 1 1 2 1 @@ -11,6 +11,6 @@ CREATE TABLE default.table_with_version_replicated_1\n(\n `key` UInt64,\n 3 3 65555 1 1 1 2 1 2 2 2 -1 -CREATE TABLE default.table_with_version_replicated_2\n(\n `key` UInt64,\n `value` String,\n `version` UInt32,\n `sign` Int8\n)\nENGINE = ReplicatedVersionedCollapsingMergeTree(\'/clickhouse/test_01511/t\', \'2\', sign, version)\nORDER BY key\nSETTINGS index_granularity = 8192 +CREATE TABLE default.table_with_version_replicated_2\n(\n `key` UInt64,\n `value` String,\n `version` UInt32,\n `sign` Int8\n)\nENGINE = ReplicatedVersionedCollapsingMergeTree(\'/clickhouse/default/test_01511/t\', \'2\', sign, version)\nORDER BY key\nSETTINGS index_granularity = 8192 1 1 2 1 2 2 2 -1 diff --git a/tests/queries/0_stateless/01511_alter_version_versioned_collapsing_merge_tree_zookeeper.sql b/tests/queries/0_stateless/01511_alter_version_versioned_collapsing_merge_tree_zookeeper.sql index 1307f055e5c..fe3476afbdb 100644 --- a/tests/queries/0_stateless/01511_alter_version_versioned_collapsing_merge_tree_zookeeper.sql +++ b/tests/queries/0_stateless/01511_alter_version_versioned_collapsing_merge_tree_zookeeper.sql @@ -8,7 +8,7 @@ CREATE TABLE table_with_version_replicated_1 version UInt8, sign Int8 ) -ENGINE ReplicatedVersionedCollapsingMergeTree('/clickhouse/test_01511/t', '1', sign, version) +ENGINE ReplicatedVersionedCollapsingMergeTree('/clickhouse/' || currentDatabase() || '/test_01511/t', '1', sign, version) ORDER BY key; CREATE TABLE table_with_version_replicated_2 @@ -18,7 +18,7 @@ CREATE TABLE table_with_version_replicated_2 version UInt8, sign Int8 ) -ENGINE ReplicatedVersionedCollapsingMergeTree('/clickhouse/test_01511/t', '2', sign, version) +ENGINE ReplicatedVersionedCollapsingMergeTree('/clickhouse/' || currentDatabase() || '/test_01511/t', '2', sign, version) ORDER BY key; INSERT INTO table_with_version_replicated_1 VALUES (1, '1', 1, -1); From 6197d20c1849412c6fde691aec7c1df3232320e8 Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Tue, 8 Jun 2021 15:48:14 +0300 Subject: [PATCH 7/7] Update KeyCondition.cpp --- src/Storages/MergeTree/KeyCondition.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Storages/MergeTree/KeyCondition.cpp b/src/Storages/MergeTree/KeyCondition.cpp index 04eae5c04f5..d624550d233 100644 --- a/src/Storages/MergeTree/KeyCondition.cpp +++ b/src/Storages/MergeTree/KeyCondition.cpp @@ -451,8 +451,6 @@ bool KeyCondition::getConstant(const ASTPtr & expr, Block & block_with_constants // Constant expr should use alias names if any String column_name = expr->getColumnName(); - // std::cerr << "========= get const for : " << column_name << "\n" << block_with_constants.dumpStructure() << std::endl; - if (const auto * lit = expr->as()) { /// By default block_with_constants has only one column named "_dummy".