From d908cddb494498f4fcf27929892d999f2402792c Mon Sep 17 00:00:00 2001 From: nikitamikhaylov Date: Fri, 23 Oct 2020 22:08:38 +0300 Subject: [PATCH 1/6] done --- src/Interpreters/ActionsVisitor.cpp | 10 +++++++--- src/Interpreters/ActionsVisitor.h | 4 +++- src/Interpreters/ExpressionAnalyzer.cpp | 16 +++++++++++++--- src/Interpreters/ExpressionAnalyzer.h | 2 ++ src/Interpreters/InterpreterSelectQuery.cpp | 4 ++-- 5 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/Interpreters/ActionsVisitor.cpp b/src/Interpreters/ActionsVisitor.cpp index e0e921b003b..2f183d7dd93 100644 --- a/src/Interpreters/ActionsVisitor.cpp +++ b/src/Interpreters/ActionsVisitor.cpp @@ -385,7 +385,7 @@ ActionsMatcher::Data::Data( const Context & context_, SizeLimits set_size_limit_, size_t subquery_depth_, const NamesAndTypesList & source_columns_, ActionsDAGPtr actions, PreparedSets & prepared_sets_, SubqueriesForSets & subqueries_for_sets_, - bool no_subqueries_, bool no_makeset_, bool only_consts_, bool no_storage_or_local_) + bool no_subqueries_, bool no_makeset_, bool only_consts_, bool no_storage_or_local_, bool has_having_) : context(context_) , set_size_limit(set_size_limit_) , subquery_depth(subquery_depth_) @@ -396,6 +396,7 @@ ActionsMatcher::Data::Data( , no_makeset(no_makeset_) , only_consts(only_consts_) , no_storage_or_local(no_storage_or_local_) + , has_having(has_having_) , visit_depth(0) , actions_stack(std::move(actions), context) , next_unique_suffix(actions_stack.getLastActions().getIndex().size() + 1) @@ -944,12 +945,15 @@ SetPtr ActionsMatcher::makeSet(const ASTFunction & node, Data & data, bool no_su SetPtr set = std::make_shared(data.set_size_limit, false, data.context.getSettingsRef().transform_null_in); - /** The following happens for GLOBAL INs: + /** The following happens for GLOBAL INs or INs: * - in the addExternalStorage function, the IN (SELECT ...) subquery is replaced with IN _data1, * in the subquery_for_set object, this subquery is set as source and the temporary table _data1 as the table. * - this function shows the expression IN_data1. + * + * In case that we have HAVING with IN subquery, we have to force creating set for it. + * Also it doesn't make sence if it is GLOBAL IN or ordinary IN. */ - if (!subquery_for_set.source && data.no_storage_or_local) + if ((!subquery_for_set.source && data.no_storage_or_local) || data.has_having) { auto interpreter = interpretSubquery(right_in_operand, data.context, data.subquery_depth, {}); subquery_for_set.source = std::make_unique(); diff --git a/src/Interpreters/ActionsVisitor.h b/src/Interpreters/ActionsVisitor.h index f4da9932163..0179e1fd09e 100644 --- a/src/Interpreters/ActionsVisitor.h +++ b/src/Interpreters/ActionsVisitor.h @@ -118,6 +118,7 @@ public: bool no_makeset; bool only_consts; bool no_storage_or_local; + bool has_having; size_t visit_depth; ScopeStack actions_stack; @@ -131,7 +132,8 @@ public: Data(const Context & context_, SizeLimits set_size_limit_, size_t subquery_depth_, const NamesAndTypesList & source_columns_, ActionsDAGPtr actions, PreparedSets & prepared_sets_, SubqueriesForSets & subqueries_for_sets_, - bool no_subqueries_, bool no_makeset_, bool only_consts_, bool no_storage_or_local_); + bool no_subqueries_, bool no_makeset_, bool only_consts_, bool no_storage_or_local_, + bool has_having); /// Does result of the calculation already exists in the block. bool hasColumn(const String & column_name) const; diff --git a/src/Interpreters/ExpressionAnalyzer.cpp b/src/Interpreters/ExpressionAnalyzer.cpp index f79bb36ec46..b852ab75e1f 100644 --- a/src/Interpreters/ExpressionAnalyzer.cpp +++ b/src/Interpreters/ExpressionAnalyzer.cpp @@ -396,7 +396,7 @@ void ExpressionAnalyzer::getRootActions(const ASTPtr & ast, bool no_subqueries, LogAST log; ActionsVisitor::Data visitor_data(context, settings.size_limits_for_set, subquery_depth, sourceColumns(), std::move(actions), prepared_sets, subqueries_for_sets, - no_subqueries, false, only_consts, !isRemoteStorage()); + no_subqueries, false, only_consts, !isRemoteStorage(), false); ActionsVisitor(visitor_data, log.stream()).visit(ast); actions = visitor_data.getActions(); } @@ -407,7 +407,17 @@ void ExpressionAnalyzer::getRootActionsNoMakeSet(const ASTPtr & ast, bool no_sub LogAST log; ActionsVisitor::Data visitor_data(context, settings.size_limits_for_set, subquery_depth, sourceColumns(), std::move(actions), prepared_sets, subqueries_for_sets, - no_subqueries, true, only_consts, !isRemoteStorage()); + no_subqueries, true, only_consts, !isRemoteStorage(), false); + ActionsVisitor(visitor_data, log.stream()).visit(ast); + actions = visitor_data.getActions(); +} + +void ExpressionAnalyzer::getRootActionsHasHaving(const ASTPtr & ast, bool no_subqueries, ActionsDAGPtr & actions, bool only_consts) +{ + LogAST log; + ActionsVisitor::Data visitor_data(context, settings.size_limits_for_set, subquery_depth, + sourceColumns(), std::move(actions), prepared_sets, subqueries_for_sets, + no_subqueries, false, only_consts, !isRemoteStorage(), true); ActionsVisitor(visitor_data, log.stream()).visit(ast); actions = visitor_data.getActions(); } @@ -825,7 +835,7 @@ bool SelectQueryExpressionAnalyzer::appendHaving(ExpressionActionsChain & chain, ExpressionActionsChain::Step & step = chain.lastStep(aggregated_columns); step.required_output.push_back(select_query->having()->getColumnName()); - getRootActions(select_query->having(), only_types, step.actions()); + getRootActionsHasHaving(select_query->having(), only_types, step.actions()); return true; } diff --git a/src/Interpreters/ExpressionAnalyzer.h b/src/Interpreters/ExpressionAnalyzer.h index 6389d8a142c..622c5204257 100644 --- a/src/Interpreters/ExpressionAnalyzer.h +++ b/src/Interpreters/ExpressionAnalyzer.h @@ -151,6 +151,8 @@ protected: */ void getRootActionsNoMakeSet(const ASTPtr & ast, bool no_subqueries, ActionsDAGPtr & actions, bool only_consts = false); + void getRootActionsHasHaving(const ASTPtr & ast, bool no_subqueries, ActionsDAGPtr & actions, bool only_consts = false); + /** Add aggregation keys to aggregation_keys, aggregate functions to aggregate_descriptions, * Create a set of columns aggregated_columns resulting after the aggregation, if any, * or after all the actions that are normally performed before aggregation. diff --git a/src/Interpreters/InterpreterSelectQuery.cpp b/src/Interpreters/InterpreterSelectQuery.cpp index d9821be4e4e..07c1942c08d 100644 --- a/src/Interpreters/InterpreterSelectQuery.cpp +++ b/src/Interpreters/InterpreterSelectQuery.cpp @@ -958,7 +958,7 @@ void InterpreterSelectQuery::executeImpl(QueryPlan & query_plan, const BlockInpu preliminary_sort(); // If there is no global subqueries, we can run subqueries only when receive them on server. - if (!query_analyzer->hasGlobalSubqueries() && !subqueries_for_sets.empty()) + if (expressions.hasHaving() || (!query_analyzer->hasGlobalSubqueries() && !subqueries_for_sets.empty())) executeSubqueriesInSetsAndJoins(query_plan, subqueries_for_sets); } @@ -1071,7 +1071,7 @@ void InterpreterSelectQuery::executeImpl(QueryPlan & query_plan, const BlockInpu } } - if (query_analyzer->hasGlobalSubqueries() && !subqueries_for_sets.empty()) + if (expressions.hasHaving() || (query_analyzer->hasGlobalSubqueries() && !subqueries_for_sets.empty())) executeSubqueriesInSetsAndJoins(query_plan, subqueries_for_sets); } From 7822dafcae3b19f3a4500736776c82b5a9e05dba Mon Sep 17 00:00:00 2001 From: nikitamikhaylov Date: Fri, 23 Oct 2020 22:37:54 +0300 Subject: [PATCH 2/6] test added --- .../01532_having_with_totals.reference | 12 +++++ .../0_stateless/01532_having_with_totals.sql | 45 +++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 tests/queries/0_stateless/01532_having_with_totals.reference create mode 100644 tests/queries/0_stateless/01532_having_with_totals.sql diff --git a/tests/queries/0_stateless/01532_having_with_totals.reference b/tests/queries/0_stateless/01532_having_with_totals.reference new file mode 100644 index 00000000000..2087369cae7 --- /dev/null +++ b/tests/queries/0_stateless/01532_having_with_totals.reference @@ -0,0 +1,12 @@ +127.0.0.{1,2} +1 + +0 +127.0.0.1 +1 + +0 +with explicit having +1 2 + +0 2 diff --git a/tests/queries/0_stateless/01532_having_with_totals.sql b/tests/queries/0_stateless/01532_having_with_totals.sql new file mode 100644 index 00000000000..00b8987fd83 --- /dev/null +++ b/tests/queries/0_stateless/01532_having_with_totals.sql @@ -0,0 +1,45 @@ +drop table if exists local_t; +create table local_t engine Log as select 1 a; + +SELECT '127.0.0.{1,2}'; +SELECT * +FROM +( + SELECT a + FROM remote('127.0.0.{1,2}', default, local_t) + GROUP BY a + WITH TOTALS +) +WHERE a IN +( + SELECT 1 +); + +SELECT '127.0.0.1'; +SELECT * +FROM +( + SELECT a + FROM remote('127.0.0.1', default, local_t) + GROUP BY a + WITH TOTALS +) +WHERE a IN +( + SELECT 1 +); + +SELECT 'with explicit having'; +SELECT + a, + count() +FROM remote('127.0.0.{1,2}', default, t) +GROUP BY a + WITH TOTALS +HAVING a IN +( + SELECT 1 +); + + +drop table if exists local_t; \ No newline at end of file From fe9440689a8bf9d2ef173e8e6aac1e787867ca7d Mon Sep 17 00:00:00 2001 From: nikitamikhaylov Date: Sat, 24 Oct 2020 00:05:24 +0300 Subject: [PATCH 3/6] better --- src/Interpreters/ActionsVisitor.h | 2 +- tests/queries/0_stateless/01532_having_with_totals.sql | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Interpreters/ActionsVisitor.h b/src/Interpreters/ActionsVisitor.h index 0179e1fd09e..8c0b56f0c3c 100644 --- a/src/Interpreters/ActionsVisitor.h +++ b/src/Interpreters/ActionsVisitor.h @@ -132,7 +132,7 @@ public: Data(const Context & context_, SizeLimits set_size_limit_, size_t subquery_depth_, const NamesAndTypesList & source_columns_, ActionsDAGPtr actions, PreparedSets & prepared_sets_, SubqueriesForSets & subqueries_for_sets_, - bool no_subqueries_, bool no_makeset_, bool only_consts_, bool no_storage_or_local_, + bool no_subqueries_, bool no_makeset_, bool only_consts_, bool no_storage_or_local_, bool has_having); /// Does result of the calculation already exists in the block. diff --git a/tests/queries/0_stateless/01532_having_with_totals.sql b/tests/queries/0_stateless/01532_having_with_totals.sql index 00b8987fd83..10f55c8c135 100644 --- a/tests/queries/0_stateless/01532_having_with_totals.sql +++ b/tests/queries/0_stateless/01532_having_with_totals.sql @@ -6,7 +6,7 @@ SELECT * FROM ( SELECT a - FROM remote('127.0.0.{1,2}', default, local_t) + FROM remote('127.0.0.{1,2}', currentDatabase(), local_t) GROUP BY a WITH TOTALS ) @@ -20,7 +20,7 @@ SELECT * FROM ( SELECT a - FROM remote('127.0.0.1', default, local_t) + FROM remote('127.0.0.1', currentDatabase(), local_t) GROUP BY a WITH TOTALS ) @@ -33,7 +33,7 @@ SELECT 'with explicit having'; SELECT a, count() -FROM remote('127.0.0.{1,2}', default, t) +FROM remote('127.0.0.{1,2}', currentDatabase(), local_t) GROUP BY a WITH TOTALS HAVING a IN From ceb83602b8fdf28fae45feac73e61cb0de53c383 Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Mon, 2 Nov 2020 15:07:01 +0300 Subject: [PATCH 4/6] Refactor --- src/Interpreters/ActionsVisitor.cpp | 7 +++---- src/Interpreters/ActionsVisitor.h | 6 ++---- src/Interpreters/ExpressionAnalyzer.cpp | 10 +++++----- src/Interpreters/ExpressionAnalyzer.h | 2 +- src/Interpreters/InterpreterSelectQuery.cpp | 4 ++-- 5 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/Interpreters/ActionsVisitor.cpp b/src/Interpreters/ActionsVisitor.cpp index 202cda2d467..3054f4781d0 100644 --- a/src/Interpreters/ActionsVisitor.cpp +++ b/src/Interpreters/ActionsVisitor.cpp @@ -385,7 +385,7 @@ ActionsMatcher::Data::Data( const Context & context_, SizeLimits set_size_limit_, size_t subquery_depth_, const NamesAndTypesList & source_columns_, ActionsDAGPtr actions, PreparedSets & prepared_sets_, SubqueriesForSets & subqueries_for_sets_, - bool no_subqueries_, bool no_makeset_, bool only_consts_, bool no_storage_or_local_, bool has_having_) + bool no_subqueries_, bool no_makeset_, bool only_consts_, bool create_source_for_in_) : context(context_) , set_size_limit(set_size_limit_) , subquery_depth(subquery_depth_) @@ -395,8 +395,7 @@ ActionsMatcher::Data::Data( , no_subqueries(no_subqueries_) , no_makeset(no_makeset_) , only_consts(only_consts_) - , no_storage_or_local(no_storage_or_local_) - , has_having(has_having_) + , create_source_for_in(create_source_for_in_) , visit_depth(0) , actions_stack(std::move(actions), context) , next_unique_suffix(actions_stack.getLastActions().getIndex().size() + 1) @@ -1054,7 +1053,7 @@ SetPtr ActionsMatcher::makeSet(const ASTFunction & node, Data & data, bool no_su * In case that we have HAVING with IN subquery, we have to force creating set for it. * Also it doesn't make sence if it is GLOBAL IN or ordinary IN. */ - if ((!subquery_for_set.source && data.no_storage_or_local) || data.has_having) + if (!subquery_for_set.source && data.create_source_for_in) { auto interpreter = interpretSubquery(right_in_operand, data.context, data.subquery_depth, {}); subquery_for_set.source = std::make_unique(); diff --git a/src/Interpreters/ActionsVisitor.h b/src/Interpreters/ActionsVisitor.h index 10583efa5b0..c2dd9c9b033 100644 --- a/src/Interpreters/ActionsVisitor.h +++ b/src/Interpreters/ActionsVisitor.h @@ -117,8 +117,7 @@ public: bool no_subqueries; bool no_makeset; bool only_consts; - bool no_storage_or_local; - bool has_having; + bool create_source_for_in; size_t visit_depth; ScopeStack actions_stack; @@ -132,8 +131,7 @@ public: Data(const Context & context_, SizeLimits set_size_limit_, size_t subquery_depth_, const NamesAndTypesList & source_columns_, ActionsDAGPtr actions, PreparedSets & prepared_sets_, SubqueriesForSets & subqueries_for_sets_, - bool no_subqueries_, bool no_makeset_, bool only_consts_, bool no_storage_or_local_, - bool has_having); + bool no_subqueries_, bool no_makeset_, bool only_consts_, bool create_source_for_in_); /// Does result of the calculation already exists in the block. bool hasColumn(const String & column_name) const; diff --git a/src/Interpreters/ExpressionAnalyzer.cpp b/src/Interpreters/ExpressionAnalyzer.cpp index 6f062548653..516c12b2be7 100644 --- a/src/Interpreters/ExpressionAnalyzer.cpp +++ b/src/Interpreters/ExpressionAnalyzer.cpp @@ -396,7 +396,7 @@ void ExpressionAnalyzer::getRootActions(const ASTPtr & ast, bool no_subqueries, LogAST log; ActionsVisitor::Data visitor_data(context, settings.size_limits_for_set, subquery_depth, sourceColumns(), std::move(actions), prepared_sets, subqueries_for_sets, - no_subqueries, false, only_consts, !isRemoteStorage(), false); + no_subqueries, false, only_consts, !isRemoteStorage()); ActionsVisitor(visitor_data, log.stream()).visit(ast); actions = visitor_data.getActions(); } @@ -407,17 +407,17 @@ void ExpressionAnalyzer::getRootActionsNoMakeSet(const ASTPtr & ast, bool no_sub LogAST log; ActionsVisitor::Data visitor_data(context, settings.size_limits_for_set, subquery_depth, sourceColumns(), std::move(actions), prepared_sets, subqueries_for_sets, - no_subqueries, true, only_consts, !isRemoteStorage(), false); + no_subqueries, true, only_consts, !isRemoteStorage()); ActionsVisitor(visitor_data, log.stream()).visit(ast); actions = visitor_data.getActions(); } -void ExpressionAnalyzer::getRootActionsHasHaving(const ASTPtr & ast, bool no_subqueries, ActionsDAGPtr & actions, bool only_consts) +void ExpressionAnalyzer::getRootActionsForHaving(const ASTPtr & ast, bool no_subqueries, ActionsDAGPtr & actions, bool only_consts) { LogAST log; ActionsVisitor::Data visitor_data(context, settings.size_limits_for_set, subquery_depth, sourceColumns(), std::move(actions), prepared_sets, subqueries_for_sets, - no_subqueries, false, only_consts, !isRemoteStorage(), true); + no_subqueries, false, only_consts, true); ActionsVisitor(visitor_data, log.stream()).visit(ast); actions = visitor_data.getActions(); } @@ -836,7 +836,7 @@ bool SelectQueryExpressionAnalyzer::appendHaving(ExpressionActionsChain & chain, ExpressionActionsChain::Step & step = chain.lastStep(aggregated_columns); step.required_output.push_back(select_query->having()->getColumnName()); - getRootActions(select_query->having(), only_types, step.actions()); + getRootActionsForHaving(select_query->having(), only_types, step.actions()); return true; } diff --git a/src/Interpreters/ExpressionAnalyzer.h b/src/Interpreters/ExpressionAnalyzer.h index 622c5204257..bd027e5a613 100644 --- a/src/Interpreters/ExpressionAnalyzer.h +++ b/src/Interpreters/ExpressionAnalyzer.h @@ -151,7 +151,7 @@ protected: */ void getRootActionsNoMakeSet(const ASTPtr & ast, bool no_subqueries, ActionsDAGPtr & actions, bool only_consts = false); - void getRootActionsHasHaving(const ASTPtr & ast, bool no_subqueries, ActionsDAGPtr & actions, bool only_consts = false); + void getRootActionsForHaving(const ASTPtr & ast, bool no_subqueries, ActionsDAGPtr & actions, bool only_consts = false); /** Add aggregation keys to aggregation_keys, aggregate functions to aggregate_descriptions, * Create a set of columns aggregated_columns resulting after the aggregation, if any, diff --git a/src/Interpreters/InterpreterSelectQuery.cpp b/src/Interpreters/InterpreterSelectQuery.cpp index 07c1942c08d..814350d2267 100644 --- a/src/Interpreters/InterpreterSelectQuery.cpp +++ b/src/Interpreters/InterpreterSelectQuery.cpp @@ -958,7 +958,7 @@ void InterpreterSelectQuery::executeImpl(QueryPlan & query_plan, const BlockInpu preliminary_sort(); // If there is no global subqueries, we can run subqueries only when receive them on server. - if (expressions.hasHaving() || (!query_analyzer->hasGlobalSubqueries() && !subqueries_for_sets.empty())) + if (!query_analyzer->hasGlobalSubqueries() && !subqueries_for_sets.empty()) executeSubqueriesInSetsAndJoins(query_plan, subqueries_for_sets); } @@ -1071,7 +1071,7 @@ void InterpreterSelectQuery::executeImpl(QueryPlan & query_plan, const BlockInpu } } - if (expressions.hasHaving() || (query_analyzer->hasGlobalSubqueries() && !subqueries_for_sets.empty())) + if (!subqueries_for_sets.empty() && (expressions.hasHaving() || query_analyzer->hasGlobalSubqueries())) executeSubqueriesInSetsAndJoins(query_plan, subqueries_for_sets); } From 726be794a4546101c078976427e77c7ce657876e Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Tue, 3 Nov 2020 17:14:34 +0300 Subject: [PATCH 5/6] Update ExpressionAnalyzer.cpp --- src/Interpreters/ExpressionAnalyzer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Interpreters/ExpressionAnalyzer.cpp b/src/Interpreters/ExpressionAnalyzer.cpp index 516c12b2be7..45230c53e81 100644 --- a/src/Interpreters/ExpressionAnalyzer.cpp +++ b/src/Interpreters/ExpressionAnalyzer.cpp @@ -835,8 +835,8 @@ bool SelectQueryExpressionAnalyzer::appendHaving(ExpressionActionsChain & chain, ExpressionActionsChain::Step & step = chain.lastStep(aggregated_columns); - step.required_output.push_back(select_query->having()->getColumnName()); getRootActionsForHaving(select_query->having(), only_types, step.actions()); + step.required_output.push_back(select_query->having()->getColumnName()); return true; } From d7de4509a3b2e26ca8f5f417ad4abb09818797fe Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Tue, 3 Nov 2020 19:07:27 +0300 Subject: [PATCH 6/6] Update ActionsVisitor.cpp --- src/Interpreters/ActionsVisitor.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Interpreters/ActionsVisitor.cpp b/src/Interpreters/ActionsVisitor.cpp index 3054f4781d0..67ef37ba319 100644 --- a/src/Interpreters/ActionsVisitor.cpp +++ b/src/Interpreters/ActionsVisitor.cpp @@ -1049,7 +1049,7 @@ SetPtr ActionsMatcher::makeSet(const ASTFunction & node, Data & data, bool no_su * - in the addExternalStorage function, the IN (SELECT ...) subquery is replaced with IN _data1, * in the subquery_for_set object, this subquery is set as source and the temporary table _data1 as the table. * - this function shows the expression IN_data1. - * + * * In case that we have HAVING with IN subquery, we have to force creating set for it. * Also it doesn't make sence if it is GLOBAL IN or ordinary IN. */