From 9331cbb4c748570e5084e3f2ae6aa3ebc07bbfc4 Mon Sep 17 00:00:00 2001 From: robot-clickhouse Date: Wed, 10 Apr 2024 12:04:09 +0000 Subject: [PATCH] Backport #62362 to 24.3: Fix analyzer with positional arguments in distributed query --- src/Analyzer/Passes/QueryAnalysisPass.cpp | 25 +++++++++---------- .../QueryPlan/DistributedCreateLocalPlan.cpp | 4 +++ ..._query_with_positional_arguments.reference | 3 +++ ...ibuted_query_with_positional_arguments.sql | 7 ++++++ 4 files changed, 26 insertions(+), 13 deletions(-) create mode 100644 tests/queries/0_stateless/03018_analyzer_distributed_query_with_positional_arguments.reference create mode 100644 tests/queries/0_stateless/03018_analyzer_distributed_query_with_positional_arguments.sql diff --git a/src/Analyzer/Passes/QueryAnalysisPass.cpp b/src/Analyzer/Passes/QueryAnalysisPass.cpp index 91832f6060d..c9a88fec393 100644 --- a/src/Analyzer/Passes/QueryAnalysisPass.cpp +++ b/src/Analyzer/Passes/QueryAnalysisPass.cpp @@ -2276,6 +2276,10 @@ void QueryAnalyzer::mergeWindowWithParentWindow(const QueryTreeNodePtr & window_ */ void QueryAnalyzer::replaceNodesWithPositionalArguments(QueryTreeNodePtr & node_list, const QueryTreeNodes & projection_nodes, IdentifierResolveScope & scope) { + const auto & settings = scope.context->getSettingsRef(); + if (!settings.enable_positional_arguments || scope.context->getClientInfo().query_kind != ClientInfo::QueryKind::INITIAL_QUERY) + return; + auto & node_list_typed = node_list->as(); for (auto & node : node_list_typed.getNodes()) @@ -2288,7 +2292,8 @@ void QueryAnalyzer::replaceNodesWithPositionalArguments(QueryTreeNodePtr & node_ auto * constant_node = (*node_to_replace)->as(); if (!constant_node - || (constant_node->getValue().getType() != Field::Types::UInt64 && constant_node->getValue().getType() != Field::Types::Int64)) + || (constant_node->getValue().getType() != Field::Types::UInt64 + && constant_node->getValue().getType() != Field::Types::Int64)) continue; UInt64 pos; @@ -6681,15 +6686,12 @@ void expandTuplesInList(QueryTreeNodes & key_list) */ void QueryAnalyzer::resolveGroupByNode(QueryNode & query_node_typed, IdentifierResolveScope & scope) { - const auto & settings = scope.context->getSettingsRef(); - if (query_node_typed.isGroupByWithGroupingSets()) { QueryTreeNodes nullable_group_by_keys; for (auto & grouping_sets_keys_list_node : query_node_typed.getGroupBy().getNodes()) { - if (settings.enable_positional_arguments) - replaceNodesWithPositionalArguments(grouping_sets_keys_list_node, query_node_typed.getProjection().getNodes(), scope); + replaceNodesWithPositionalArguments(grouping_sets_keys_list_node, query_node_typed.getProjection().getNodes(), scope); // Remove redundant calls to `tuple` function. It simplifies checking if expression is an aggregation key. // It's required to support queries like: SELECT number FROM numbers(3) GROUP BY (number, number % 2) @@ -6708,8 +6710,7 @@ void QueryAnalyzer::resolveGroupByNode(QueryNode & query_node_typed, IdentifierR } else { - if (settings.enable_positional_arguments) - replaceNodesWithPositionalArguments(query_node_typed.getGroupByNode(), query_node_typed.getProjection().getNodes(), scope); + replaceNodesWithPositionalArguments(query_node_typed.getGroupByNode(), query_node_typed.getProjection().getNodes(), scope); // Remove redundant calls to `tuple` function. It simplifies checking if expression is an aggregation key. // It's required to support queries like: SELECT number FROM numbers(3) GROUP BY (number, number % 2) @@ -7860,8 +7861,6 @@ void QueryAnalyzer::resolveQuery(const QueryTreeNodePtr & query_node, Identifier if (query_node_typed.isCTE()) cte_in_resolve_process.insert(query_node_typed.getCTEName()); - const auto & settings = scope.context->getSettingsRef(); - bool is_rollup_or_cube = query_node_typed.isGroupByWithRollup() || query_node_typed.isGroupByWithCube(); if (query_node_typed.isGroupByWithGroupingSets() @@ -8045,8 +8044,9 @@ void QueryAnalyzer::resolveQuery(const QueryTreeNodePtr & query_node, Identifier if (query_node_typed.hasOrderBy()) { - if (settings.enable_positional_arguments) - replaceNodesWithPositionalArguments(query_node_typed.getOrderByNode(), query_node_typed.getProjection().getNodes(), scope); + replaceNodesWithPositionalArguments(query_node_typed.getOrderByNode(), query_node_typed.getProjection().getNodes(), scope); + + const auto & settings = scope.context->getSettingsRef(); expandOrderByAll(query_node_typed, settings); resolveSortNodeList(query_node_typed.getOrderByNode(), scope); @@ -8069,8 +8069,7 @@ void QueryAnalyzer::resolveQuery(const QueryTreeNodePtr & query_node, Identifier if (query_node_typed.hasLimitBy()) { - if (settings.enable_positional_arguments) - replaceNodesWithPositionalArguments(query_node_typed.getLimitByNode(), query_node_typed.getProjection().getNodes(), scope); + replaceNodesWithPositionalArguments(query_node_typed.getLimitByNode(), query_node_typed.getProjection().getNodes(), scope); resolveExpressionNodeList(query_node_typed.getLimitByNode(), scope, false /*allow_lambda_expression*/, false /*allow_table_expression*/); } diff --git a/src/Processors/QueryPlan/DistributedCreateLocalPlan.cpp b/src/Processors/QueryPlan/DistributedCreateLocalPlan.cpp index c8d230c87d9..d4545482477 100644 --- a/src/Processors/QueryPlan/DistributedCreateLocalPlan.cpp +++ b/src/Processors/QueryPlan/DistributedCreateLocalPlan.cpp @@ -68,6 +68,10 @@ std::unique_ptr createLocalPlan( if (context->getSettingsRef().allow_experimental_analyzer) { + /// For Analyzer, identifier in GROUP BY/ORDER BY/LIMIT BY lists has been resolved to + /// ConstantNode in QueryTree if it is an alias of a constant, so we should not replace + /// ConstantNode with ProjectionNode again(https://github.com/ClickHouse/ClickHouse/issues/62289). + new_context->setSetting("enable_positional_arguments", Field(false)); auto interpreter = InterpreterSelectQueryAnalyzer(query_ast, new_context, select_query_options); query_plan = std::make_unique(std::move(interpreter).extractQueryPlan()); } diff --git a/tests/queries/0_stateless/03018_analyzer_distributed_query_with_positional_arguments.reference b/tests/queries/0_stateless/03018_analyzer_distributed_query_with_positional_arguments.reference new file mode 100644 index 00000000000..bb0b1cf658d --- /dev/null +++ b/tests/queries/0_stateless/03018_analyzer_distributed_query_with_positional_arguments.reference @@ -0,0 +1,3 @@ +0 +0 +0 diff --git a/tests/queries/0_stateless/03018_analyzer_distributed_query_with_positional_arguments.sql b/tests/queries/0_stateless/03018_analyzer_distributed_query_with_positional_arguments.sql new file mode 100644 index 00000000000..16ba3b15594 --- /dev/null +++ b/tests/queries/0_stateless/03018_analyzer_distributed_query_with_positional_arguments.sql @@ -0,0 +1,7 @@ +select 0 as x +from remote('127.0.0.{1,2}', system.one) +group by x; + +select 0 as x +from remote('127.0.0.{1,2}', system.one) +order by x;