Merge pull request #16308 from nikitamikhaylov/totals-having

Incorrect behaviour of SELECT WITH TOTALS

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 create_source_for_in_)
     : context(context_)
     , set_size_limit(set_size_limit_)
     , subquery_depth(subquery_depth_)
@@ -395,7 +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_)
+    , 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)
@@ -1045,12 +1045,15 @@ SetPtr ActionsMatcher::makeSet(const ASTFunction & node, Data & data, bool no_su
 
         SetPtr set = std::make_shared<Set>(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.create_source_for_in)
         {
             auto interpreter = interpretSubquery(right_in_operand, data.context, data.subquery_depth, {});
             subquery_for_set.source = std::make_unique<QueryPlan>();

src/Interpreters/ActionsVisitor.h

@@ -117,7 +117,7 @@ public:
         bool no_subqueries;
         bool no_makeset;
         bool only_consts;
-        bool no_storage_or_local;
+        bool create_source_for_in;
         size_t visit_depth;
         ScopeStack actions_stack;
 
@@ -131,7 +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 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;

src/Interpreters/ExpressionAnalyzer.cpp

@@ -412,6 +412,16 @@ void ExpressionAnalyzer::getRootActionsNoMakeSet(const ASTPtr & ast, bool no_sub
     actions = visitor_data.getActions();
 }
 
+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, true);
+    ActionsVisitor(visitor_data, log.stream()).visit(ast);
+    actions = visitor_data.getActions();
+}
+
 
 bool ExpressionAnalyzer::makeAggregateDescriptions(ActionsDAGPtr & actions)
 {
@@ -825,7 +835,7 @@ bool SelectQueryExpressionAnalyzer::appendHaving(ExpressionActionsChain & chain,
 
     ExpressionActionsChain::Step & step = chain.lastStep(aggregated_columns);
 
-    getRootActions(select_query->having(), only_types, step.actions());
+    getRootActionsForHaving(select_query->having(), only_types, step.actions());
     step.required_output.push_back(select_query->having()->getColumnName());
 
     return true;

src/Interpreters/ExpressionAnalyzer.h

@@ -151,6 +151,8 @@ protected:
       */
     void getRootActionsNoMakeSet(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,
       *  or after all the actions that are normally performed before aggregation.

src/Interpreters/InterpreterSelectQuery.cpp

@@ -1071,7 +1071,7 @@ void InterpreterSelectQuery::executeImpl(QueryPlan & query_plan, const BlockInpu
         }
     }
 
-    if (query_analyzer->hasGlobalSubqueries() && !subqueries_for_sets.empty())
+    if (!subqueries_for_sets.empty() && (expressions.hasHaving() || query_analyzer->hasGlobalSubqueries()))
         executeSubqueriesInSetsAndJoins(query_plan, subqueries_for_sets);
 }
 

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

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}', currentDatabase(), 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', currentDatabase(), 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}', currentDatabase(), local_t)
+GROUP BY a
+    WITH TOTALS
+HAVING a IN 
+(
+    SELECT 1
+);
+
+
+drop table if exists local_t;