From 48d21bb03b3cd7c522735bdcfdc381f0185b9c13 Mon Sep 17 00:00:00 2001 From: kssenii Date: Mon, 28 Jun 2021 13:19:13 +0000 Subject: [PATCH 1/8] Fix inconsistency --- src/Interpreters/ActionsDAG.cpp | 3 +++ ...01925_test_const_column_group_by_consistency.reference | 6 ++++++ .../01925_test_const_column_group_by_consistency.sql | 8 ++++++++ 3 files changed, 17 insertions(+) create mode 100644 tests/queries/0_stateless/01925_test_const_column_group_by_consistency.reference create mode 100644 tests/queries/0_stateless/01925_test_const_column_group_by_consistency.sql diff --git a/src/Interpreters/ActionsDAG.cpp b/src/Interpreters/ActionsDAG.cpp index 9fa48f6ceab..55c863a6f8c 100644 --- a/src/Interpreters/ActionsDAG.cpp +++ b/src/Interpreters/ActionsDAG.cpp @@ -219,6 +219,9 @@ const ActionsDAG::Node & ActionsDAG::addFunction( column = node.function_base->getConstantResultForNonConstArguments(arguments, node.result_type); } + if (all_const && column && !isColumnConst(*column) && column->size() <= 1) + column = ColumnConst::create(std::move(column), column->size()); + /// If the result is not a constant, just in case, we will consider the result as unknown. if (column && isColumnConst(*column)) { diff --git a/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.reference b/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.reference new file mode 100644 index 00000000000..f9bcdca26da --- /dev/null +++ b/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.reference @@ -0,0 +1,6 @@ +1 0 +1 0 +1 0 +1 0 +1 1 0 +0 0 0 diff --git a/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.sql b/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.sql new file mode 100644 index 00000000000..d288c7db023 --- /dev/null +++ b/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.sql @@ -0,0 +1,8 @@ +SELECT 1 as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; + +SELECT materialize(1) as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; +SELECT materialize(1) as a, count() FROM numbers(10)　WHERE 0 ORDER BY a; + +SELECT isConstant(1) as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; +SELECT 1 as b, isConstant(b) as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; +SELECT 0 as b, least(isConstant(materialize(1)), b) as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; From 54a7e2158c2f976bcc660c4ed53debd008ec3220 Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Mon, 28 Jun 2021 17:09:26 +0300 Subject: [PATCH 2/8] Update ActionsDAG.cpp --- src/Interpreters/ActionsDAG.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Interpreters/ActionsDAG.cpp b/src/Interpreters/ActionsDAG.cpp index 55c863a6f8c..f0c6e49b891 100644 --- a/src/Interpreters/ActionsDAG.cpp +++ b/src/Interpreters/ActionsDAG.cpp @@ -219,8 +219,8 @@ const ActionsDAG::Node & ActionsDAG::addFunction( column = node.function_base->getConstantResultForNonConstArguments(arguments, node.result_type); } - if (all_const && column && !isColumnConst(*column) && column->size() <= 1) - column = ColumnConst::create(std::move(column), column->size()); + if (all_const && column && !isColumnConst(*column) && column->size() == 1) + column = ColumnConst::create(std::move(column), 1); /// If the result is not a constant, just in case, we will consider the result as unknown. if (column && isColumnConst(*column)) From 6816e8c5aa570638bad56baf1342e72b939b474d Mon Sep 17 00:00:00 2001 From: kssenii Date: Tue, 29 Jun 2021 14:54:32 +0000 Subject: [PATCH 3/8] Revert change, fix injectiveness --- src/Functions/materialize.h | 5 +++++ src/Interpreters/ActionsDAG.cpp | 3 --- .../01925_test_const_column_group_by_consistency.reference | 4 ---- .../01925_test_const_column_group_by_consistency.sql | 6 ------ 4 files changed, 5 insertions(+), 13 deletions(-) diff --git a/src/Functions/materialize.h b/src/Functions/materialize.h index 5d36a4b2340..376fb34a672 100644 --- a/src/Functions/materialize.h +++ b/src/Functions/materialize.h @@ -29,6 +29,11 @@ public: return name; } + bool isInjective(const ColumnsWithTypeAndName & /*sample_columns*/) const override + { + return true; + } + bool useDefaultImplementationForLowCardinalityColumns() const override { return false; } size_t getNumberOfArguments() const override diff --git a/src/Interpreters/ActionsDAG.cpp b/src/Interpreters/ActionsDAG.cpp index f0c6e49b891..9fa48f6ceab 100644 --- a/src/Interpreters/ActionsDAG.cpp +++ b/src/Interpreters/ActionsDAG.cpp @@ -219,9 +219,6 @@ const ActionsDAG::Node & ActionsDAG::addFunction( column = node.function_base->getConstantResultForNonConstArguments(arguments, node.result_type); } - if (all_const && column && !isColumnConst(*column) && column->size() == 1) - column = ColumnConst::create(std::move(column), 1); - /// If the result is not a constant, just in case, we will consider the result as unknown. if (column && isColumnConst(*column)) { diff --git a/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.reference b/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.reference index f9bcdca26da..2e50651146b 100644 --- a/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.reference +++ b/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.reference @@ -1,6 +1,2 @@ 1 0 1 0 -1 0 -1 0 -1 1 0 -0 0 0 diff --git a/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.sql b/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.sql index d288c7db023..ed7a64d0877 100644 --- a/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.sql +++ b/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.sql @@ -1,8 +1,2 @@ SELECT 1 as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; - SELECT materialize(1) as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; -SELECT materialize(1) as a, count() FROM numbers(10)　WHERE 0 ORDER BY a; - -SELECT isConstant(1) as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; -SELECT 1 as b, isConstant(b) as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; -SELECT 0 as b, least(isConstant(materialize(1)), b) as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; From 40c51889a7e75a34472854758f97d375c08205d8 Mon Sep 17 00:00:00 2001 From: kssenii Date: Tue, 29 Jun 2021 16:05:53 +0000 Subject: [PATCH 4/8] Update test --- .../01925_test_const_column_group_by_consistency.reference | 2 -- .../01925_test_const_column_group_by_consistency.sql | 2 -- .../01925_test_group_by_const_consistency.reference | 5 +++++ .../0_stateless/01925_test_group_by_const_consistency.sql | 7 +++++++ 4 files changed, 12 insertions(+), 4 deletions(-) delete mode 100644 tests/queries/0_stateless/01925_test_const_column_group_by_consistency.reference delete mode 100644 tests/queries/0_stateless/01925_test_const_column_group_by_consistency.sql create mode 100644 tests/queries/0_stateless/01925_test_group_by_const_consistency.reference create mode 100644 tests/queries/0_stateless/01925_test_group_by_const_consistency.sql diff --git a/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.reference b/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.reference deleted file mode 100644 index 2e50651146b..00000000000 --- a/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.reference +++ /dev/null @@ -1,2 +0,0 @@ -1 0 -1 0 diff --git a/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.sql b/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.sql deleted file mode 100644 index ed7a64d0877..00000000000 --- a/tests/queries/0_stateless/01925_test_const_column_group_by_consistency.sql +++ /dev/null @@ -1,2 +0,0 @@ -SELECT 1 as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; -SELECT materialize(1) as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; diff --git a/tests/queries/0_stateless/01925_test_group_by_const_consistency.reference b/tests/queries/0_stateless/01925_test_group_by_const_consistency.reference new file mode 100644 index 00000000000..f2342900cb9 --- /dev/null +++ b/tests/queries/0_stateless/01925_test_group_by_const_consistency.reference @@ -0,0 +1,5 @@ +1 0 +1 0 +1 0 +2 1 0 +D 0 diff --git a/tests/queries/0_stateless/01925_test_group_by_const_consistency.sql b/tests/queries/0_stateless/01925_test_group_by_const_consistency.sql new file mode 100644 index 00000000000..f31d22a88ac --- /dev/null +++ b/tests/queries/0_stateless/01925_test_group_by_const_consistency.sql @@ -0,0 +1,7 @@ +SELECT 1 as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; + +SELECT materialize(1) as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; +SELECT materialize(1) as a, count() FROM numbers(10)　WHERE 0 ORDER BY a; + +SELECT 2 as b, less(1, b) as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; +SELECT upper('d') as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; From 94a4c7ff85c2cf784f8e9ef9a4da744e7226b5e2 Mon Sep 17 00:00:00 2001 From: kssenii Date: Tue, 29 Jun 2021 19:56:05 +0000 Subject: [PATCH 5/8] Update ANTLR skip list --- tests/queries/skip_list.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/queries/skip_list.json b/tests/queries/skip_list.json index 7c1f998e91d..76c0d0a4817 100644 --- a/tests/queries/skip_list.json +++ b/tests/queries/skip_list.json @@ -520,7 +520,8 @@ "01914_exchange_dictionaries", "01915_create_or_replace_dictionary", "01913_names_of_tuple_literal", - "01925_merge_prewhere_table" + "01925_merge_prewhere_table", + "01925_test_group_by_const_consistency" ], "parallel": [ From 02681019f86743bc8d5d063623e9877588a6f6c2 Mon Sep 17 00:00:00 2001 From: kssenii Date: Sat, 3 Jul 2021 07:45:37 +0000 Subject: [PATCH 6/8] Fix --- src/Interpreters/ExpressionAnalyzer.cpp | 7 ++++++- src/Interpreters/ExpressionAnalyzer.h | 2 ++ src/Interpreters/InterpreterSelectQuery.cpp | 2 +- src/Parsers/ASTSelectQuery.h | 1 + .../01925_test_group_by_const_consistency.reference | 6 +----- .../0_stateless/01925_test_group_by_const_consistency.sql | 7 +------ 6 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/Interpreters/ExpressionAnalyzer.cpp b/src/Interpreters/ExpressionAnalyzer.cpp index e693d4ba988..d4d0c0d0a9b 100644 --- a/src/Interpreters/ExpressionAnalyzer.cpp +++ b/src/Interpreters/ExpressionAnalyzer.cpp @@ -231,7 +231,6 @@ void ExpressionAnalyzer::analyzeAggregation() if (has_aggregation) { - /// Find out aggregation keys. if (select_query) { @@ -252,6 +251,8 @@ void ExpressionAnalyzer::analyzeAggregation() /// Constant expressions have non-null column pointer at this stage. if (node->column && isColumnConst(*node->column)) { + select_query->group_by_with_constant_keys = true; + /// But don't remove last key column if no aggregate functions, otherwise aggregation will not work. if (!aggregate_descriptions.empty() || size > 1) { @@ -288,6 +289,10 @@ void ExpressionAnalyzer::analyzeAggregation() else aggregated_columns = temp_actions->getNamesAndTypesList(); + /// Constant expressions are already removed during first 'analyze' run. + /// So for second `analyze` information is taken from select_query. + has_const_aggregation_keys = select_query->group_by_with_constant_keys; + for (const auto & desc : aggregate_descriptions) aggregated_columns.emplace_back(desc.column_name, desc.function->getReturnType()); } diff --git a/src/Interpreters/ExpressionAnalyzer.h b/src/Interpreters/ExpressionAnalyzer.h index fe00b3e9f88..ac5d281f337 100644 --- a/src/Interpreters/ExpressionAnalyzer.h +++ b/src/Interpreters/ExpressionAnalyzer.h @@ -65,6 +65,7 @@ struct ExpressionAnalyzerData bool has_aggregation = false; NamesAndTypesList aggregation_keys; + bool has_const_aggregation_keys = false; AggregateDescriptions aggregate_descriptions; WindowDescriptions window_descriptions; @@ -309,6 +310,7 @@ public: bool hasTableJoin() const { return syntax->ast_join; } const NamesAndTypesList & aggregationKeys() const { return aggregation_keys; } + bool hasConstAggregationKeys() const { return has_const_aggregation_keys; } const AggregateDescriptions & aggregates() const { return aggregate_descriptions; } const PreparedSets & getPreparedSets() const { return prepared_sets; } diff --git a/src/Interpreters/InterpreterSelectQuery.cpp b/src/Interpreters/InterpreterSelectQuery.cpp index 181b60b7bf3..324340e4635 100644 --- a/src/Interpreters/InterpreterSelectQuery.cpp +++ b/src/Interpreters/InterpreterSelectQuery.cpp @@ -2035,7 +2035,7 @@ void InterpreterSelectQuery::executeAggregation(QueryPlan & query_plan, const Ac settings.group_by_two_level_threshold, settings.group_by_two_level_threshold_bytes, settings.max_bytes_before_external_group_by, - settings.empty_result_for_aggregation_by_empty_set, + settings.empty_result_for_aggregation_by_empty_set || (keys.empty() && query_analyzer->hasConstAggregationKeys()), context->getTemporaryVolume(), settings.max_threads, settings.min_free_disk_space_for_temporary_data); diff --git a/src/Parsers/ASTSelectQuery.h b/src/Parsers/ASTSelectQuery.h index e9aaa4ab83b..3fc8efb5311 100644 --- a/src/Parsers/ASTSelectQuery.h +++ b/src/Parsers/ASTSelectQuery.h @@ -44,6 +44,7 @@ public: bool group_by_with_totals = false; bool group_by_with_rollup = false; bool group_by_with_cube = false; + bool group_by_with_constant_keys = false; bool limit_with_ties = false; ASTPtr & refSelect() { return getExpression(Expression::SELECT); } diff --git a/tests/queries/0_stateless/01925_test_group_by_const_consistency.reference b/tests/queries/0_stateless/01925_test_group_by_const_consistency.reference index f2342900cb9..573541ac970 100644 --- a/tests/queries/0_stateless/01925_test_group_by_const_consistency.reference +++ b/tests/queries/0_stateless/01925_test_group_by_const_consistency.reference @@ -1,5 +1 @@ -1 0 -1 0 -1 0 -2 1 0 -D 0 +0 diff --git a/tests/queries/0_stateless/01925_test_group_by_const_consistency.sql b/tests/queries/0_stateless/01925_test_group_by_const_consistency.sql index f31d22a88ac..8a5de0e7c4f 100644 --- a/tests/queries/0_stateless/01925_test_group_by_const_consistency.sql +++ b/tests/queries/0_stateless/01925_test_group_by_const_consistency.sql @@ -1,7 +1,2 @@ SELECT 1 as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; - -SELECT materialize(1) as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; -SELECT materialize(1) as a, count() FROM numbers(10)　WHERE 0 ORDER BY a; - -SELECT 2 as b, less(1, b) as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; -SELECT upper('d') as a, count() FROM numbers(10)　WHERE 0 GROUP BY a; +SELECT count() FROM numbers(10) WHERE 0 From 8a8e72b77f0fc3683477c9e43eb4d611a6e2555a Mon Sep 17 00:00:00 2001 From: kssenii Date: Sat, 3 Jul 2021 18:44:17 +0000 Subject: [PATCH 7/8] Update .reference --- tests/queries/0_stateless/01414_optimize_any_bug.reference | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/queries/0_stateless/01414_optimize_any_bug.reference b/tests/queries/0_stateless/01414_optimize_any_bug.reference index 573541ac970..e69de29bb2d 100644 --- a/tests/queries/0_stateless/01414_optimize_any_bug.reference +++ b/tests/queries/0_stateless/01414_optimize_any_bug.reference @@ -1 +0,0 @@ -0 From e866faa975013f70eb9c455a0497d66c4c73c56b Mon Sep 17 00:00:00 2001 From: kssenii Date: Sun, 4 Jul 2021 02:20:36 +0300 Subject: [PATCH 8/8] Fix --- src/Interpreters/ExpressionAnalyzer.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Interpreters/ExpressionAnalyzer.cpp b/src/Interpreters/ExpressionAnalyzer.cpp index d4d0c0d0a9b..0897efe08fb 100644 --- a/src/Interpreters/ExpressionAnalyzer.cpp +++ b/src/Interpreters/ExpressionAnalyzer.cpp @@ -291,7 +291,8 @@ void ExpressionAnalyzer::analyzeAggregation() /// Constant expressions are already removed during first 'analyze' run. /// So for second `analyze` information is taken from select_query. - has_const_aggregation_keys = select_query->group_by_with_constant_keys; + if (select_query) + has_const_aggregation_keys = select_query->group_by_with_constant_keys; for (const auto & desc : aggregate_descriptions) aggregated_columns.emplace_back(desc.column_name, desc.function->getReturnType());