From 70f779b81dc04110ebc7240d154b285aaa0f205b Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Thu, 18 Aug 2022 20:40:20 +0000 Subject: [PATCH 01/54] Just to save --- .../QueryPlan/Optimizations/Optimizations.h | 9 +++++-- .../QueryPlanOptimizationSettings.cpp | 1 + .../QueryPlanOptimizationSettings.h | 3 +++ .../Optimizations/distinctReadInOrder.cpp | 24 +++++++++++++++++++ 4 files changed, 35 insertions(+), 2 deletions(-) create mode 100644 src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp diff --git a/src/Processors/QueryPlan/Optimizations/Optimizations.h b/src/Processors/QueryPlan/Optimizations/Optimizations.h index 904f30e84b0..c19a39fdba4 100644 --- a/src/Processors/QueryPlan/Optimizations/Optimizations.h +++ b/src/Processors/QueryPlan/Optimizations/Optimizations.h @@ -54,16 +54,21 @@ size_t tryExecuteFunctionsAfterSorting(QueryPlan::Node * parent_node, QueryPlan: /// Update information about prefix sort description in SortingStep. size_t tryReuseStorageOrderingForWindowFunctions(QueryPlan::Node * parent_node, QueryPlan::Nodes & nodes); +/// TODO: description +size_t tryDistinctReadInOrder(QueryPlan::Node * node, QueryPlan::Nodes & nodes); + inline const auto & getOptimizations() { - static const std::array optimizations = {{ + static const std::array optimizations = {{ {tryLiftUpArrayJoin, "liftUpArrayJoin", &QueryPlanOptimizationSettings::optimize_plan}, {tryPushDownLimit, "pushDownLimit", &QueryPlanOptimizationSettings::optimize_plan}, {trySplitFilter, "splitFilter", &QueryPlanOptimizationSettings::optimize_plan}, {tryMergeExpressions, "mergeExpressions", &QueryPlanOptimizationSettings::optimize_plan}, {tryPushDownFilter, "pushDownFilter", &QueryPlanOptimizationSettings::filter_push_down}, {tryExecuteFunctionsAfterSorting, "liftUpFunctions", &QueryPlanOptimizationSettings::optimize_plan}, - {tryReuseStorageOrderingForWindowFunctions, "reuseStorageOrderingForWindowFunctions", &QueryPlanOptimizationSettings::optimize_plan} + {tryReuseStorageOrderingForWindowFunctions, "reuseStorageOrderingForWindowFunctions", &QueryPlanOptimizationSettings::optimize_plan}, + /// todo: should we have just a common optimization regarding reading in order? + {tryDistinctReadInOrder, "distinctReadInOrder", &QueryPlanOptimizationSettings::optimize_plan} }}; return optimizations; diff --git a/src/Processors/QueryPlan/Optimizations/QueryPlanOptimizationSettings.cpp b/src/Processors/QueryPlan/Optimizations/QueryPlanOptimizationSettings.cpp index 1472fb87a89..2342f961751 100644 --- a/src/Processors/QueryPlan/Optimizations/QueryPlanOptimizationSettings.cpp +++ b/src/Processors/QueryPlan/Optimizations/QueryPlanOptimizationSettings.cpp @@ -11,6 +11,7 @@ QueryPlanOptimizationSettings QueryPlanOptimizationSettings::fromSettings(const settings.optimize_plan = from.query_plan_enable_optimizations; settings.max_optimizations_to_apply = from.query_plan_max_optimizations_to_apply; settings.filter_push_down = from.query_plan_filter_push_down; + settings.distinct_in_order = from.optimize_distinct_in_order; return settings; } diff --git a/src/Processors/QueryPlan/Optimizations/QueryPlanOptimizationSettings.h b/src/Processors/QueryPlan/Optimizations/QueryPlanOptimizationSettings.h index b5a37bf69d6..0da89f28aad 100644 --- a/src/Processors/QueryPlan/Optimizations/QueryPlanOptimizationSettings.h +++ b/src/Processors/QueryPlan/Optimizations/QueryPlanOptimizationSettings.h @@ -21,6 +21,9 @@ struct QueryPlanOptimizationSettings /// If filter push down optimization is enabled. bool filter_push_down = true; + /// if distinct in order optimization is enabled + bool distinct_in_order = false; + static QueryPlanOptimizationSettings fromSettings(const Settings & from); static QueryPlanOptimizationSettings fromContext(ContextPtr from); }; diff --git a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp new file mode 100644 index 00000000000..3bc51a0bffe --- /dev/null +++ b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp @@ -0,0 +1,24 @@ +#include +#include +#include +#include + +namespace DB::QueryPlanOptimizations +{ + +/// +size_t tryDistinctReadInOrder(QueryPlan::Node * , QueryPlan::Nodes & ) +{ + /// check if storage already read in order + + /// find DISTINCT + + /// check if DISTINCT has the same columns as we read from storage + + /// check if sorting is preserved for the DISTINCt columns throught plan + /// if so, set storage to read in order + + return 0; +} + +} From 30860290de1213f5ef63acea449e7ab021975d65 Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Mon, 5 Sep 2022 20:12:00 +0000 Subject: [PATCH 02/54] Continue --- src/Interpreters/TreeOptimizer.cpp | 6 +- src/Processors/QueryPlan/DistinctStep.h | 2 + .../QueryPlan/Optimizations/Optimizations.h | 2 +- .../Optimizations/distinctReadInOrder.cpp | 62 ++++++++++++++++--- src/Storages/SelectQueryInfo.h | 6 +- 5 files changed, 62 insertions(+), 16 deletions(-) diff --git a/src/Interpreters/TreeOptimizer.cpp b/src/Interpreters/TreeOptimizer.cpp index eaf59731967..71d0e6f76f1 100644 --- a/src/Interpreters/TreeOptimizer.cpp +++ b/src/Interpreters/TreeOptimizer.cpp @@ -418,7 +418,7 @@ void optimizeDuplicateDistinct(ASTSelectQuery & select) return; std::unordered_set distinct_names = getDistinctNames(*subselect); - std::unordered_set selected_names; + std::unordered_set selected_names; /// Check source column names from select list (ignore aliases and table names) for (const auto & id : select.select()->children) @@ -427,11 +427,11 @@ void optimizeDuplicateDistinct(ASTSelectQuery & select) if (!identifier) return; - String name = identifier->shortName(); + const String & name = identifier->shortName(); if (!distinct_names.contains(name)) return; /// Not a distinct column, keep DISTINCT for it. - selected_names.insert(name); + selected_names.emplace(name); } /// select columns list != distinct columns list diff --git a/src/Processors/QueryPlan/DistinctStep.h b/src/Processors/QueryPlan/DistinctStep.h index dc734a58704..ecd7ca3e1d0 100644 --- a/src/Processors/QueryPlan/DistinctStep.h +++ b/src/Processors/QueryPlan/DistinctStep.h @@ -24,6 +24,8 @@ public: void describeActions(JSONBuilder::JSONMap & map) const override; void describeActions(FormatSettings & settings) const override; + bool isPreliminary() const { return pre_distinct; } + private: void updateOutputStream() override; diff --git a/src/Processors/QueryPlan/Optimizations/Optimizations.h b/src/Processors/QueryPlan/Optimizations/Optimizations.h index c19a39fdba4..34338243f97 100644 --- a/src/Processors/QueryPlan/Optimizations/Optimizations.h +++ b/src/Processors/QueryPlan/Optimizations/Optimizations.h @@ -68,7 +68,7 @@ inline const auto & getOptimizations() {tryExecuteFunctionsAfterSorting, "liftUpFunctions", &QueryPlanOptimizationSettings::optimize_plan}, {tryReuseStorageOrderingForWindowFunctions, "reuseStorageOrderingForWindowFunctions", &QueryPlanOptimizationSettings::optimize_plan}, /// todo: should we have just a common optimization regarding reading in order? - {tryDistinctReadInOrder, "distinctReadInOrder", &QueryPlanOptimizationSettings::optimize_plan} + {tryDistinctReadInOrder, "distinctReadInOrder", &QueryPlanOptimizationSettings::distinct_in_order} }}; return optimizations; diff --git a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp index 3bc51a0bffe..f1efbb5da6f 100644 --- a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp +++ b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp @@ -1,23 +1,67 @@ +#include +#include #include -#include -#include -#include +#include +#include namespace DB::QueryPlanOptimizations { -/// -size_t tryDistinctReadInOrder(QueryPlan::Node * , QueryPlan::Nodes & ) +/// +size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) { /// check if storage already read in order - - /// find DISTINCT + QueryPlan::Node * node = parent_node; + DistinctStep const * pre_distinct = nullptr; + while (!node->children.empty()) + { + if (pre_distinct) + { + /// check if nodes below DISTINCT preserve sorting + const auto * step = typeid_cast(node->step.get()); + if (step) + { + const ITransformingStep::DataStreamTraits & traits = step->getDataStreamTraits(); + if (!traits.preserves_sorting) + return 0; + } + } + if (auto const * tmp = typeid_cast(node->step.get()); tmp) + { + if (tmp->isPreliminary()) + pre_distinct = tmp; + } + node = node->children.front(); + } + if (!pre_distinct) + return 0; - /// check if DISTINCT has the same columns as we read from storage + auto const * storage = typeid_cast(node->step.get()); + if (!storage) + return 0; + + const DataStream & output = storage->getOutputStream(); + if (output.sort_mode != DataStream::SortMode::Chunk) + return 0; + + /// check if DISTINCT has the same columns as sorting key + const SortDescription & sort_desc = output.sort_description; + const DataStream & distinct_output = pre_distinct->getOutputStream(); + const auto & distinct_columns = distinct_output.distinct_columns; + auto it = sort_desc.begin(); + for (; it != sort_desc.end(); ++it) + { + if (distinct_columns.end() == std::find(begin(distinct_columns), end(distinct_columns), it->column_name)) + break; + } + /// apply optimization only when distinct columns match or form prefix of sorting key + /// todo: check if reading in order optimization would be benefitial sorting key is prefix of columns in DISTINCT + if (it != sort_desc.end()) + return 0; - /// check if sorting is preserved for the DISTINCt columns throught plan /// if so, set storage to read in order + return 0; } diff --git a/src/Storages/SelectQueryInfo.h b/src/Storages/SelectQueryInfo.h index c41b422199d..f2835ab4dbf 100644 --- a/src/Storages/SelectQueryInfo.h +++ b/src/Storages/SelectQueryInfo.h @@ -117,10 +117,10 @@ struct InputOrderInfo * sort_description_for_merging will be equal to (c, d) and * used_prefix_of_sorting_key_size will be equal to 4. */ - size_t used_prefix_of_sorting_key_size; + const size_t used_prefix_of_sorting_key_size; - int direction; - UInt64 limit; + const int direction; + const UInt64 limit; InputOrderInfo( const SortDescription & sort_description_for_merging_, From c23412ae77c7f79a367d581f13a5650a5b8ad2c2 Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Wed, 7 Sep 2022 22:02:50 +0000 Subject: [PATCH 03/54] slight changes --- .../Optimizations/distinctReadInOrder.cpp | 40 ++++++++++++------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp index f1efbb5da6f..3aa953fa460 100644 --- a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp +++ b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp @@ -1,7 +1,10 @@ +#include +#include #include #include #include #include +#include #include namespace DB::QueryPlanOptimizations @@ -10,7 +13,9 @@ namespace DB::QueryPlanOptimizations /// size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) { - /// check if storage already read in order + /// walk through the plan + /// (1) check if there is preliminary distinct node + /// (2) check if nodes below preliminary distinct preserve sorting QueryPlan::Node * node = parent_node; DistinctStep const * pre_distinct = nullptr; while (!node->children.empty()) @@ -36,31 +41,36 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) if (!pre_distinct) return 0; - auto const * storage = typeid_cast(node->step.get()); - if (!storage) + auto * read_from_merge_tree = typeid_cast(node->step.get()); + if (!read_from_merge_tree) return 0; - const DataStream & output = storage->getOutputStream(); + /// check if reading in order is already there + const DataStream & output = read_from_merge_tree->getOutputStream(); if (output.sort_mode != DataStream::SortMode::Chunk) return 0; - /// check if DISTINCT has the same columns as sorting key const SortDescription & sort_desc = output.sort_description; - const DataStream & distinct_output = pre_distinct->getOutputStream(); - const auto & distinct_columns = distinct_output.distinct_columns; - auto it = sort_desc.begin(); - for (; it != sort_desc.end(); ++it) - { - if (distinct_columns.end() == std::find(begin(distinct_columns), end(distinct_columns), it->column_name)) - break; - } + const auto & distinct_columns = pre_distinct->getOutputStream().distinct_columns; /// apply optimization only when distinct columns match or form prefix of sorting key /// todo: check if reading in order optimization would be benefitial sorting key is prefix of columns in DISTINCT - if (it != sort_desc.end()) + if (sort_desc.size() < distinct_columns.size()) return 0; - /// if so, set storage to read in order + /// check if DISTINCT has the same columns as sorting key + SortDescription distinct_sort_desc; + distinct_sort_desc.reserve(sort_desc.size()); + for (const auto & column_desc : sort_desc) + { + if (distinct_columns.end() == std::find(begin(distinct_columns), end(distinct_columns), column_desc.column_name)) + break; + distinct_sort_desc.push_back(column_desc); + } + if (!distinct_sort_desc.empty()) + return 0; + + // TODO: provide input order info to read_from_merge_tree return 0; } From 9fc0aaf477d0c631c78ec14d9651361601c1bf7a Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Mon, 12 Sep 2022 17:01:26 +0000 Subject: [PATCH 04/54] Something working with tests --- src/Core/SortDescription.cpp | 6 ++-- src/Core/SortDescription.h | 3 +- src/Processors/QueryPlan/DistinctStep.h | 2 ++ .../Optimizations/distinctReadInOrder.cpp | 16 ++++++--- .../QueryPlan/ReadFromMergeTree.cpp | 10 +++++- ...ct_in_order_optimization_explain.reference | 18 ++++++---- ..._distinct_in_order_optimization_explain.sh | 33 ++++++++++++------- 7 files changed, 60 insertions(+), 28 deletions(-) diff --git a/src/Core/SortDescription.cpp b/src/Core/SortDescription.cpp index 59018fb13b4..047b14cb8a4 100644 --- a/src/Core/SortDescription.cpp +++ b/src/Core/SortDescription.cpp @@ -42,7 +42,7 @@ void SortColumnDescription::explain(JSONBuilder::JSONMap & map) const map.add("With Fill", with_fill); } -bool SortDescription::hasPrefix(const SortDescription & prefix) const +size_t SortDescription::hasPrefix(const SortDescription & prefix) const { if (prefix.empty()) return true; @@ -53,9 +53,9 @@ bool SortDescription::hasPrefix(const SortDescription & prefix) const for (size_t i = 0; i < prefix.size(); ++i) { if ((*this)[i] != prefix[i]) - return false; + return i; } - return true; + return size(); } #if USE_EMBEDDED_COMPILER diff --git a/src/Core/SortDescription.h b/src/Core/SortDescription.h index 0025e44b489..9eaf4ce1da3 100644 --- a/src/Core/SortDescription.h +++ b/src/Core/SortDescription.h @@ -48,6 +48,7 @@ struct SortColumnDescription bool with_fill; FillColumnDescription fill_description; + SortColumnDescription() = default; explicit SortColumnDescription( const std::string & column_name_, int direction_ = 1, @@ -120,7 +121,7 @@ public: size_t min_count_to_compile_sort_description = 3; bool compile_sort_description = false; - bool hasPrefix(const SortDescription & prefix) const; + size_t hasPrefix(const SortDescription & prefix) const; }; /** Compile sort description for header_types. diff --git a/src/Processors/QueryPlan/DistinctStep.h b/src/Processors/QueryPlan/DistinctStep.h index ecd7ca3e1d0..57205b72030 100644 --- a/src/Processors/QueryPlan/DistinctStep.h +++ b/src/Processors/QueryPlan/DistinctStep.h @@ -26,6 +26,8 @@ public: bool isPreliminary() const { return pre_distinct; } + UInt64 getLimitHint() const { return limit_hint; } + private: void updateOutputStream() override; diff --git a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp index 3aa953fa460..66a8a3d8feb 100644 --- a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp +++ b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -6,6 +7,8 @@ #include #include #include +#include "Processors/QueryPlan/IQueryPlanStep.h" +#include "Storages/SelectQueryInfo.h" namespace DB::QueryPlanOptimizations { @@ -17,13 +20,13 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) /// (1) check if there is preliminary distinct node /// (2) check if nodes below preliminary distinct preserve sorting QueryPlan::Node * node = parent_node; - DistinctStep const * pre_distinct = nullptr; + DistinctStep * pre_distinct = nullptr; while (!node->children.empty()) { if (pre_distinct) { /// check if nodes below DISTINCT preserve sorting - const auto * step = typeid_cast(node->step.get()); + auto * step = typeid_cast(node->step.get()); if (step) { const ITransformingStep::DataStreamTraits & traits = step->getDataStreamTraits(); @@ -31,7 +34,7 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) return 0; } } - if (auto const * tmp = typeid_cast(node->step.get()); tmp) + if (auto * tmp = typeid_cast(node->step.get()); tmp) { if (tmp->isPreliminary()) pre_distinct = tmp; @@ -70,7 +73,12 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) if (!distinct_sort_desc.empty()) return 0; - // TODO: provide input order info to read_from_merge_tree + InputOrderInfoPtr order_info + = std::make_shared(distinct_sort_desc, distinct_sort_desc.size(), 1, pre_distinct->getLimitHint()); + read_from_merge_tree->setQueryInfoInputOrderInfo(order_info); + + /// update data stream's sorting properties + pre_distinct->updateInputStream(read_from_merge_tree->getOutputStream()); return 0; } diff --git a/src/Processors/QueryPlan/ReadFromMergeTree.cpp b/src/Processors/QueryPlan/ReadFromMergeTree.cpp index 1f6c6ee2a3f..baed67cfc43 100644 --- a/src/Processors/QueryPlan/ReadFromMergeTree.cpp +++ b/src/Processors/QueryPlan/ReadFromMergeTree.cpp @@ -149,7 +149,6 @@ ReadFromMergeTree::ReadFromMergeTree( } output_stream->sort_description = std::move(sort_description); - } } @@ -1038,6 +1037,15 @@ void ReadFromMergeTree::setQueryInfoInputOrderInfo(InputOrderInfoPtr order_info) { query_info.input_order_info = order_info; } + + /// update sort info for output stream + SortDescription & current_sort_desc = output_stream->sort_description; + size_t prefix_size = current_sort_desc.hasPrefix(order_info->sort_description_for_merging); + if (!prefix_size) + return; + + current_sort_desc.resize(prefix_size); + output_stream->sort_mode = DataStream::SortMode::Port; } ReadFromMergeTree::AnalysisResult ReadFromMergeTree::getAnalysisResult() const diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference index 01e0c397dc7..51816015c56 100644 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference @@ -5,22 +5,22 @@ DistinctTransform -- enable optimize_distinct_in_order -- distinct with all primary key columns -> pre-distinct optimization only DistinctTransform -DistinctSortedChunkTransform +DistinctSortedChunkTransform × 2 -- distinct with primary key prefix -> pre-distinct optimization only DistinctTransform -DistinctSortedChunkTransform +DistinctSortedChunkTransform × 2 -- distinct with primary key prefix and order by column in distinct -> pre-distinct and final distinct optimization DistinctSortedTransform -DistinctSortedChunkTransform +DistinctSortedChunkTransform × 2 -- distinct with primary key prefix and order by the same columns -> pre-distinct and final distinct optimization DistinctSortedStreamTransform -DistinctSortedChunkTransform +DistinctSortedChunkTransform × 2 -- distinct with primary key prefix and order by column in distinct but non-primary key prefix -> pre-distinct and final distinct optimization DistinctSortedTransform -DistinctSortedChunkTransform +DistinctSortedChunkTransform × 2 -- distinct with primary key prefix and order by column _not_ in distinct -> pre-distinct optimization only DistinctTransform -DistinctSortedChunkTransform +DistinctSortedChunkTransform × 2 -- distinct with non-primary key prefix -> ordinary distinct DistinctTransform DistinctTransform @@ -29,7 +29,11 @@ DistinctSortedTransform DistinctTransform -- distinct with non-primary key prefix and order by column _not_ in distinct -> ordinary distinct DistinctTransform -DistinctTransform +DistinctTransform × 2 -- distinct with non-primary key prefix and order by _const_ column in distinct -> ordinary distinct DistinctTransform DistinctTransform +-- Check reading in order for distinct +MergeTreeInOrder × 2 0 → 1 +MergeTreeThread × 2 0 → 1 +MergeTreeThread × 2 0 → 1 diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh index fcffb974433..3ed80d17a43 100755 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh @@ -11,44 +11,53 @@ ENABLE_OPTIMIZATION="set optimize_distinct_in_order=1" GREP_DISTINCT="grep 'DistinctSortedChunkTransform\|DistinctSortedStreamTransform\|DistinctSortedTransform\|DistinctTransform'" TRIM_LEADING_SPACES="sed -e 's/^[ \t]*//'" FIND_DISTINCT="$GREP_DISTINCT | $TRIM_LEADING_SPACES" +FIND_READING_IN_ORDER="grep 'MergeTreeInOrder' | $TRIM_LEADING_SPACES" +FIND_READING_DEFAULT="grep 'MergeTreeThread' | $TRIM_LEADING_SPACES" +MAKE_OUTPUT_STABLE="set max_threads=1" $CLICKHOUSE_CLIENT -q "drop table if exists distinct_in_order_explain sync" $CLICKHOUSE_CLIENT -q "create table distinct_in_order_explain (a int, b int, c int) engine=MergeTree() order by (a, b)" $CLICKHOUSE_CLIENT -q "insert into distinct_in_order_explain select number % number, number % 5, number % 10 from numbers(1,10)" +$CLICKHOUSE_CLIENT -q "insert into distinct_in_order_explain select number % number, number % 5, number % 10 from numbers(1,10)" $CLICKHOUSE_CLIENT -q "select '-- disable optimize_distinct_in_order'" $CLICKHOUSE_CLIENT -q "select '-- distinct all primary key columns -> ordinary distinct'" -$CLICKHOUSE_CLIENT -nq "$DISABLE_OPTIMIZATION;explain pipeline select distinct * from distinct_in_order_explain" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$DISABLE_OPTIMIZATION;explain pipeline select distinct * from distinct_in_order_explain" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- enable optimize_distinct_in_order'" $CLICKHOUSE_CLIENT -q "select '-- distinct with all primary key columns -> pre-distinct optimization only'" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct * from distinct_in_order_explain" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct * from distinct_in_order_explain" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with primary key prefix -> pre-distinct optimization only'" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, c from distinct_in_order_explain" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct a, c from distinct_in_order_explain" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with primary key prefix and order by column in distinct -> pre-distinct and final distinct optimization'" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, c from distinct_in_order_explain order by c" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct a, c from distinct_in_order_explain order by c" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with primary key prefix and order by the same columns -> pre-distinct and final distinct optimization'" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain order by a, b" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain order by a, b" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with primary key prefix and order by column in distinct but non-primary key prefix -> pre-distinct and final distinct optimization'" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b, c from distinct_in_order_explain order by c" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b, c from distinct_in_order_explain order by c" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with primary key prefix and order by column _not_ in distinct -> pre-distinct optimization only'" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, c from distinct_in_order_explain order by b" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct a, c from distinct_in_order_explain order by b" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with non-primary key prefix -> ordinary distinct'" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct b, c from distinct_in_order_explain" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct b, c from distinct_in_order_explain" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with non-primary key prefix and order by column in distinct -> final distinct optimization only'" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct b, c from distinct_in_order_explain order by b" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct b, c from distinct_in_order_explain order by b" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with non-primary key prefix and order by column _not_ in distinct -> ordinary distinct'" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct b, c from distinct_in_order_explain order by a" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct b, c from distinct_in_order_explain order by a" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with non-primary key prefix and order by _const_ column in distinct -> ordinary distinct'" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct b, 1 as x from distinct_in_order_explain order by x" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct b, 1 as x from distinct_in_order_explain order by x" | eval $FIND_DISTINCT -$CLICKHOUSE_CLIENT -q "drop table if exists distinct_in_order_explain sync" +echo "-- Check reading in order for distinct" +$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER +$CLICKHOUSE_CLIENT -nq "$DISABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_DEFAULT +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct b from distinct_in_order_explain" | eval $FIND_READING_DEFAULT + +# $CLICKHOUSE_CLIENT -q "drop table if exists distinct_in_order_explain sync" From 7502be3b7534d5625a9eefe1fb9a332419120ca0 Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Mon, 12 Sep 2022 20:11:24 +0000 Subject: [PATCH 05/54] Fixes --- src/Core/Block.cpp | 1 + .../QueryPlan/Optimizations/distinctReadInOrder.cpp | 10 +++++----- src/Processors/QueryPlan/ReadFromMergeTree.cpp | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Core/Block.cpp b/src/Core/Block.cpp index 3b7595eb886..33691e83d27 100644 --- a/src/Core/Block.cpp +++ b/src/Core/Block.cpp @@ -623,6 +623,7 @@ NamesAndTypesList Block::getNamesAndTypesList() const NamesAndTypes Block::getNamesAndTypes() const { NamesAndTypes res; + res.reserve(columns()); for (const auto & elem : data) res.emplace_back(elem.name, elem.type); diff --git a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp index 66a8a3d8feb..d9ed690a6ff 100644 --- a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp +++ b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp @@ -7,8 +7,6 @@ #include #include #include -#include "Processors/QueryPlan/IQueryPlanStep.h" -#include "Storages/SelectQueryInfo.h" namespace DB::QueryPlanOptimizations { @@ -50,11 +48,11 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) /// check if reading in order is already there const DataStream & output = read_from_merge_tree->getOutputStream(); - if (output.sort_mode != DataStream::SortMode::Chunk) + if (output.sort_scope != DataStream::SortScope::Chunk) return 0; const SortDescription & sort_desc = output.sort_description; - const auto & distinct_columns = pre_distinct->getOutputStream().distinct_columns; + const auto & distinct_columns = pre_distinct->getOutputStream().header.getNames(); /// apply optimization only when distinct columns match or form prefix of sorting key /// todo: check if reading in order optimization would be benefitial sorting key is prefix of columns in DISTINCT if (sort_desc.size() < distinct_columns.size()) @@ -70,9 +68,11 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) distinct_sort_desc.push_back(column_desc); } - if (!distinct_sort_desc.empty()) + if (distinct_sort_desc.empty()) return 0; + LOG_ERROR(&Poco::Logger::get("DistinctOptimization"), "DISTINCT sort description: {}", dumpSortDescription(distinct_sort_desc)); + InputOrderInfoPtr order_info = std::make_shared(distinct_sort_desc, distinct_sort_desc.size(), 1, pre_distinct->getLimitHint()); read_from_merge_tree->setQueryInfoInputOrderInfo(order_info); diff --git a/src/Processors/QueryPlan/ReadFromMergeTree.cpp b/src/Processors/QueryPlan/ReadFromMergeTree.cpp index 43698973950..984d92e56ef 100644 --- a/src/Processors/QueryPlan/ReadFromMergeTree.cpp +++ b/src/Processors/QueryPlan/ReadFromMergeTree.cpp @@ -1044,7 +1044,7 @@ void ReadFromMergeTree::setQueryInfoInputOrderInfo(InputOrderInfoPtr order_info) return; current_sort_desc.resize(prefix_size); - output_stream->sort_mode = DataStream::SortMode::Port; + output_stream->sort_scope = DataStream::SortScope::Stream; } ReadFromMergeTree::AnalysisResult ReadFromMergeTree::getAnalysisResult() const From 1c9c303cce0fa3851101c8895e4543ed7c910c88 Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Mon, 12 Sep 2022 21:11:05 +0000 Subject: [PATCH 06/54] Fix typos --- .../QueryPlan/Optimizations/distinctReadInOrder.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp index d9ed690a6ff..ea7252fcd96 100644 --- a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp +++ b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp @@ -54,7 +54,7 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) const SortDescription & sort_desc = output.sort_description; const auto & distinct_columns = pre_distinct->getOutputStream().header.getNames(); /// apply optimization only when distinct columns match or form prefix of sorting key - /// todo: check if reading in order optimization would be benefitial sorting key is prefix of columns in DISTINCT + /// todo: check if reading in order optimization would be beneficial if sorting key is prefix of columns in DISTINCT if (sort_desc.size() < distinct_columns.size()) return 0; @@ -71,8 +71,6 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) if (distinct_sort_desc.empty()) return 0; - LOG_ERROR(&Poco::Logger::get("DistinctOptimization"), "DISTINCT sort description: {}", dumpSortDescription(distinct_sort_desc)); - InputOrderInfoPtr order_info = std::make_shared(distinct_sort_desc, distinct_sort_desc.size(), 1, pre_distinct->getLimitHint()); read_from_merge_tree->setQueryInfoInputOrderInfo(order_info); From c129b31d7ba14f3b6461b97315922f1971a9ab32 Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Tue, 13 Sep 2022 09:40:23 +0000 Subject: [PATCH 07/54] Test fix --- .../QueryPlan/Optimizations/Optimizations.h | 1 - ...ct_in_order_optimization_explain.reference | 20 +++++------ ..._distinct_in_order_optimization_explain.sh | 34 +++++++++---------- 3 files changed, 27 insertions(+), 28 deletions(-) diff --git a/src/Processors/QueryPlan/Optimizations/Optimizations.h b/src/Processors/QueryPlan/Optimizations/Optimizations.h index 34338243f97..99c9afbd083 100644 --- a/src/Processors/QueryPlan/Optimizations/Optimizations.h +++ b/src/Processors/QueryPlan/Optimizations/Optimizations.h @@ -67,7 +67,6 @@ inline const auto & getOptimizations() {tryPushDownFilter, "pushDownFilter", &QueryPlanOptimizationSettings::filter_push_down}, {tryExecuteFunctionsAfterSorting, "liftUpFunctions", &QueryPlanOptimizationSettings::optimize_plan}, {tryReuseStorageOrderingForWindowFunctions, "reuseStorageOrderingForWindowFunctions", &QueryPlanOptimizationSettings::optimize_plan}, - /// todo: should we have just a common optimization regarding reading in order? {tryDistinctReadInOrder, "distinctReadInOrder", &QueryPlanOptimizationSettings::distinct_in_order} }}; diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference index 51816015c56..a1f26aaf5e7 100644 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference @@ -5,22 +5,22 @@ DistinctTransform -- enable optimize_distinct_in_order -- distinct with all primary key columns -> pre-distinct optimization only DistinctTransform -DistinctSortedChunkTransform × 2 +DistinctSortedChunkTransform -- distinct with primary key prefix -> pre-distinct optimization only DistinctTransform -DistinctSortedChunkTransform × 2 +DistinctSortedChunkTransform -- distinct with primary key prefix and order by column in distinct -> pre-distinct and final distinct optimization DistinctSortedTransform -DistinctSortedChunkTransform × 2 +DistinctSortedChunkTransform -- distinct with primary key prefix and order by the same columns -> pre-distinct and final distinct optimization DistinctSortedStreamTransform -DistinctSortedChunkTransform × 2 +DistinctSortedChunkTransform -- distinct with primary key prefix and order by column in distinct but non-primary key prefix -> pre-distinct and final distinct optimization DistinctSortedTransform -DistinctSortedChunkTransform × 2 +DistinctSortedChunkTransform -- distinct with primary key prefix and order by column _not_ in distinct -> pre-distinct optimization only DistinctTransform -DistinctSortedChunkTransform × 2 +DistinctSortedChunkTransform -- distinct with non-primary key prefix -> ordinary distinct DistinctTransform DistinctTransform @@ -29,11 +29,11 @@ DistinctSortedTransform DistinctTransform -- distinct with non-primary key prefix and order by column _not_ in distinct -> ordinary distinct DistinctTransform -DistinctTransform × 2 +DistinctTransform -- distinct with non-primary key prefix and order by _const_ column in distinct -> ordinary distinct DistinctTransform DistinctTransform -- Check reading in order for distinct -MergeTreeInOrder × 2 0 → 1 -MergeTreeThread × 2 0 → 1 -MergeTreeThread × 2 0 → 1 +MergeTreeInOrder +MergeTreeThread +MergeTreeThread diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh index 3ed80d17a43..94f1c0c6d4e 100755 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh @@ -10,10 +10,10 @@ DISABLE_OPTIMIZATION="set optimize_distinct_in_order=0" ENABLE_OPTIMIZATION="set optimize_distinct_in_order=1" GREP_DISTINCT="grep 'DistinctSortedChunkTransform\|DistinctSortedStreamTransform\|DistinctSortedTransform\|DistinctTransform'" TRIM_LEADING_SPACES="sed -e 's/^[ \t]*//'" -FIND_DISTINCT="$GREP_DISTINCT | $TRIM_LEADING_SPACES" -FIND_READING_IN_ORDER="grep 'MergeTreeInOrder' | $TRIM_LEADING_SPACES" -FIND_READING_DEFAULT="grep 'MergeTreeThread' | $TRIM_LEADING_SPACES" -MAKE_OUTPUT_STABLE="set max_threads=1" +REMOVE_NON_LETTERS="sed 's/[^a-zA-Z]//g'" +FIND_DISTINCT="$GREP_DISTINCT | $TRIM_LEADING_SPACES | $REMOVE_NON_LETTERS" +FIND_READING_IN_ORDER="grep 'MergeTreeInOrder' | $TRIM_LEADING_SPACES | $REMOVE_NON_LETTERS" +FIND_READING_DEFAULT="grep 'MergeTreeThread' | $TRIM_LEADING_SPACES | $REMOVE_NON_LETTERS" $CLICKHOUSE_CLIENT -q "drop table if exists distinct_in_order_explain sync" $CLICKHOUSE_CLIENT -q "create table distinct_in_order_explain (a int, b int, c int) engine=MergeTree() order by (a, b)" @@ -22,42 +22,42 @@ $CLICKHOUSE_CLIENT -q "insert into distinct_in_order_explain select number % num $CLICKHOUSE_CLIENT -q "select '-- disable optimize_distinct_in_order'" $CLICKHOUSE_CLIENT -q "select '-- distinct all primary key columns -> ordinary distinct'" -$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$DISABLE_OPTIMIZATION;explain pipeline select distinct * from distinct_in_order_explain" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$DISABLE_OPTIMIZATION;explain pipeline select distinct * from distinct_in_order_explain" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- enable optimize_distinct_in_order'" $CLICKHOUSE_CLIENT -q "select '-- distinct with all primary key columns -> pre-distinct optimization only'" -$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct * from distinct_in_order_explain" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct * from distinct_in_order_explain" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with primary key prefix -> pre-distinct optimization only'" -$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct a, c from distinct_in_order_explain" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, c from distinct_in_order_explain" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with primary key prefix and order by column in distinct -> pre-distinct and final distinct optimization'" -$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct a, c from distinct_in_order_explain order by c" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, c from distinct_in_order_explain order by c" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with primary key prefix and order by the same columns -> pre-distinct and final distinct optimization'" -$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain order by a, b" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain order by a, b" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with primary key prefix and order by column in distinct but non-primary key prefix -> pre-distinct and final distinct optimization'" -$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b, c from distinct_in_order_explain order by c" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b, c from distinct_in_order_explain order by c" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with primary key prefix and order by column _not_ in distinct -> pre-distinct optimization only'" -$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct a, c from distinct_in_order_explain order by b" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, c from distinct_in_order_explain order by b" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with non-primary key prefix -> ordinary distinct'" -$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct b, c from distinct_in_order_explain" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct b, c from distinct_in_order_explain" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with non-primary key prefix and order by column in distinct -> final distinct optimization only'" -$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct b, c from distinct_in_order_explain order by b" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct b, c from distinct_in_order_explain order by b" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with non-primary key prefix and order by column _not_ in distinct -> ordinary distinct'" -$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct b, c from distinct_in_order_explain order by a" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct b, c from distinct_in_order_explain order by a" | eval $FIND_DISTINCT $CLICKHOUSE_CLIENT -q "select '-- distinct with non-primary key prefix and order by _const_ column in distinct -> ordinary distinct'" -$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct b, 1 as x from distinct_in_order_explain order by x" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct b, 1 as x from distinct_in_order_explain order by x" | eval $FIND_DISTINCT echo "-- Check reading in order for distinct" -$CLICKHOUSE_CLIENT -nq "$MAKE_OUTPUT_STABLE;$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER $CLICKHOUSE_CLIENT -nq "$DISABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_DEFAULT $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct b from distinct_in_order_explain" | eval $FIND_READING_DEFAULT -# $CLICKHOUSE_CLIENT -q "drop table if exists distinct_in_order_explain sync" +$CLICKHOUSE_CLIENT -q "drop table if exists distinct_in_order_explain sync" From f99f57aed8046dce968fcb8ff2277fea190b791d Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Tue, 13 Sep 2022 12:23:01 +0000 Subject: [PATCH 08/54] Remove unrelated changes --- src/Core/Block.cpp | 1 - src/Interpreters/TreeOptimizer.cpp | 6 +++--- src/Storages/SelectQueryInfo.h | 6 +++--- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/Core/Block.cpp b/src/Core/Block.cpp index 33691e83d27..3b7595eb886 100644 --- a/src/Core/Block.cpp +++ b/src/Core/Block.cpp @@ -623,7 +623,6 @@ NamesAndTypesList Block::getNamesAndTypesList() const NamesAndTypes Block::getNamesAndTypes() const { NamesAndTypes res; - res.reserve(columns()); for (const auto & elem : data) res.emplace_back(elem.name, elem.type); diff --git a/src/Interpreters/TreeOptimizer.cpp b/src/Interpreters/TreeOptimizer.cpp index 74f084df40b..3f7e141db3e 100644 --- a/src/Interpreters/TreeOptimizer.cpp +++ b/src/Interpreters/TreeOptimizer.cpp @@ -418,7 +418,7 @@ void optimizeDuplicateDistinct(ASTSelectQuery & select) return; std::unordered_set distinct_names = getDistinctNames(*subselect); - std::unordered_set selected_names; + std::unordered_set selected_names; /// Check source column names from select list (ignore aliases and table names) for (const auto & id : select.select()->children) @@ -427,11 +427,11 @@ void optimizeDuplicateDistinct(ASTSelectQuery & select) if (!identifier) return; - const String & name = identifier->shortName(); + String name = identifier->shortName(); if (!distinct_names.contains(name)) return; /// Not a distinct column, keep DISTINCT for it. - selected_names.emplace(name); + selected_names.insert(name); } /// select columns list != distinct columns list diff --git a/src/Storages/SelectQueryInfo.h b/src/Storages/SelectQueryInfo.h index f2835ab4dbf..c41b422199d 100644 --- a/src/Storages/SelectQueryInfo.h +++ b/src/Storages/SelectQueryInfo.h @@ -117,10 +117,10 @@ struct InputOrderInfo * sort_description_for_merging will be equal to (c, d) and * used_prefix_of_sorting_key_size will be equal to 4. */ - const size_t used_prefix_of_sorting_key_size; + size_t used_prefix_of_sorting_key_size; - const int direction; - const UInt64 limit; + int direction; + UInt64 limit; InputOrderInfo( const SortDescription & sort_description_for_merging_, From c04d46bc96ab213a19de3de8dc3d6af31fd32c97 Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Tue, 13 Sep 2022 17:50:43 +0000 Subject: [PATCH 09/54] Update data stream properties after reading in order applied + tests --- .../Optimizations/distinctReadInOrder.cpp | 33 ++++++++++++++++--- ...7_distinct_in_order_optimization.reference | 4 +++ .../02317_distinct_in_order_optimization.sql | 15 ++++++++- 3 files changed, 47 insertions(+), 5 deletions(-) diff --git a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp index ea7252fcd96..5b12322560c 100644 --- a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp +++ b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp @@ -19,12 +19,13 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) /// (2) check if nodes below preliminary distinct preserve sorting QueryPlan::Node * node = parent_node; DistinctStep * pre_distinct = nullptr; + QueryPlan::Node * pre_distinct_node = nullptr; while (!node->children.empty()) { if (pre_distinct) { /// check if nodes below DISTINCT preserve sorting - auto * step = typeid_cast(node->step.get()); + const auto * step = dynamic_cast(node->step.get()); if (step) { const ITransformingStep::DataStreamTraits & traits = step->getDataStreamTraits(); @@ -35,7 +36,10 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) if (auto * tmp = typeid_cast(node->step.get()); tmp) { if (tmp->isPreliminary()) + { + pre_distinct_node = node; pre_distinct = tmp; + } } node = node->children.front(); } @@ -54,7 +58,7 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) const SortDescription & sort_desc = output.sort_description; const auto & distinct_columns = pre_distinct->getOutputStream().header.getNames(); /// apply optimization only when distinct columns match or form prefix of sorting key - /// todo: check if reading in order optimization would be beneficial if sorting key is prefix of columns in DISTINCT + /// todo: check if reading in order optimization would be beneficial when sorting key is prefix of columns in DISTINCT if (sort_desc.size() < distinct_columns.size()) return 0; @@ -71,12 +75,33 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) if (distinct_sort_desc.empty()) return 0; + const int direction = 1; /// default direction, ASC InputOrderInfoPtr order_info - = std::make_shared(distinct_sort_desc, distinct_sort_desc.size(), 1, pre_distinct->getLimitHint()); + = std::make_shared(distinct_sort_desc, distinct_sort_desc.size(), direction, pre_distinct->getLimitHint()); read_from_merge_tree->setQueryInfoInputOrderInfo(order_info); /// update data stream's sorting properties - pre_distinct->updateInputStream(read_from_merge_tree->getOutputStream()); + std::vector steps2update; + node = pre_distinct_node; + while (node && node->step.get() != read_from_merge_tree) + { + auto * transform = dynamic_cast(node->step.get()); + if (transform) + steps2update.push_back(transform); + + if (!node->children.empty()) + node = node->children.front(); + else + node = nullptr; + } + + const DataStream * input_stream = &read_from_merge_tree->getOutputStream(); + while (!steps2update.empty()) + { + steps2update.back()->updateInputStream(*input_stream); + input_stream = &steps2update.back()->getOutputStream(); + steps2update.pop_back(); + } return 0; } diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization.reference b/tests/queries/0_stateless/02317_distinct_in_order_optimization.reference index d827dc33838..22ecaafe989 100644 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization.reference +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization.reference @@ -104,3 +104,7 @@ select distinct a, b, x, y from (select a, b, 1 as x, 2 as y from distinct_in_or 0 3 1 2 0 4 1 2 -- check that distinct in order returns the same result as ordinary distinct +-- check that distinct in order WITH order by returns the same result as ordinary distinct +0 +-- check that distinct in order WITHOUT order by returns the same result as ordinary distinct +0 diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization.sql b/tests/queries/0_stateless/02317_distinct_in_order_optimization.sql index 0fb3766edb4..f0c7a11f418 100644 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization.sql +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization.sql @@ -65,12 +65,25 @@ INSERT INTO distinct_cardinality_low SELECT number % 1e1, number % 1e2, number % drop table if exists distinct_in_order sync; drop table if exists ordinary_distinct sync; +select '-- check that distinct in order WITH order by returns the same result as ordinary distinct'; create table distinct_in_order (low UInt64, medium UInt64, high UInt64) engine=MergeTree() order by (low, medium); insert into distinct_in_order select distinct * from distinct_cardinality_low order by high settings optimize_distinct_in_order=1; create table ordinary_distinct (low UInt64, medium UInt64, high UInt64) engine=MergeTree() order by (low, medium); insert into ordinary_distinct select distinct * from distinct_cardinality_low order by high settings optimize_distinct_in_order=0; -select distinct * from distinct_in_order except select * from ordinary_distinct; +select count() as diff from (select distinct * from distinct_in_order except select * from ordinary_distinct); + +drop table if exists distinct_in_order sync; +drop table if exists ordinary_distinct sync; + +select '-- check that distinct in order WITHOUT order by returns the same result as ordinary distinct'; +create table distinct_in_order (low UInt64, medium UInt64, high UInt64) engine=MergeTree() order by (low, medium); +insert into distinct_in_order select distinct * from distinct_cardinality_low settings optimize_distinct_in_order=1; +create table ordinary_distinct (low UInt64, medium UInt64, high UInt64) engine=MergeTree() order by (low, medium); +insert into ordinary_distinct select distinct * from distinct_cardinality_low settings optimize_distinct_in_order=0; +select count() as diff from (select distinct * from distinct_in_order except select * from ordinary_distinct); drop table if exists distinct_in_order; drop table if exists ordinary_distinct; + drop table if exists distinct_cardinality_low; + From 1be5a79e9f0e1bc26599e13307b67ed5cef6127d Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Tue, 13 Sep 2022 18:23:55 +0000 Subject: [PATCH 10/54] Exclude const columns in distinct when matching with sorting key + tests --- .../Optimizations/distinctReadInOrder.cpp | 14 +++++++++++--- ...istinct_in_order_optimization_explain.reference | 11 ++++++++++- ...02317_distinct_in_order_optimization_explain.sh | 11 ++++++++++- 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp index 5b12322560c..f48340b0417 100644 --- a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp +++ b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp @@ -56,10 +56,18 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) return 0; const SortDescription & sort_desc = output.sort_description; - const auto & distinct_columns = pre_distinct->getOutputStream().header.getNames(); + const auto & distinct_columns = pre_distinct->getOutputStream().header.getColumnsWithTypeAndName(); + std::vector non_const_columns; + non_const_columns.reserve(distinct_columns.size()); + for (const auto & column : distinct_columns) + { + if (!isColumnConst(*column.column)) + non_const_columns.emplace_back(column.name); + } + /// apply optimization only when distinct columns match or form prefix of sorting key /// todo: check if reading in order optimization would be beneficial when sorting key is prefix of columns in DISTINCT - if (sort_desc.size() < distinct_columns.size()) + if (sort_desc.size() < non_const_columns.size()) return 0; /// check if DISTINCT has the same columns as sorting key @@ -67,7 +75,7 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) distinct_sort_desc.reserve(sort_desc.size()); for (const auto & column_desc : sort_desc) { - if (distinct_columns.end() == std::find(begin(distinct_columns), end(distinct_columns), column_desc.column_name)) + if (non_const_columns.end() == std::find(begin(non_const_columns), end(non_const_columns), column_desc.column_name)) break; distinct_sort_desc.push_back(column_desc); diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference index a1f26aaf5e7..9b43eb81747 100644 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference @@ -34,6 +34,15 @@ DistinctTransform DistinctTransform DistinctTransform -- Check reading in order for distinct +-- disabled, distinct columns match sorting key +MergeTreeThread +-- enabled, distinct columns match sorting key MergeTreeInOrder +-- enabled, distinct columns form prefix of sorting key +MergeTreeInOrder +-- enabled, distinct columns DON't form prefix of sorting key MergeTreeThread -MergeTreeThread +-- enabled, distinct columns contains constant columns, non-const columns form prefix of sorting key +MergeTreeInOrder +-- enabled, distinct columns contains constant columns, non-const columns match prefix of sorting key +MergeTreeInOrder diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh index 94f1c0c6d4e..bfbf3b6de0c 100755 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh @@ -56,8 +56,17 @@ $CLICKHOUSE_CLIENT -q "select '-- distinct with non-primary key prefix and order $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct b, 1 as x from distinct_in_order_explain order by x" | eval $FIND_DISTINCT echo "-- Check reading in order for distinct" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER +echo "-- disabled, distinct columns match sorting key" $CLICKHOUSE_CLIENT -nq "$DISABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_DEFAULT +echo "-- enabled, distinct columns match sorting key" +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER +echo "-- enabled, distinct columns form prefix of sorting key" +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER +echo "-- enabled, distinct columns DON't form prefix of sorting key" $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct b from distinct_in_order_explain" | eval $FIND_READING_DEFAULT +echo "-- enabled, distinct columns contains constant columns, non-const columns form prefix of sorting key" +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct 1, a from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER +echo "-- enabled, distinct columns contains constant columns, non-const columns match prefix of sorting key" +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct 1, b, a from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER $CLICKHOUSE_CLIENT -q "drop table if exists distinct_in_order_explain sync" From d91ea0c5ab8d9147f8513947cac63a33f36b5d6e Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Wed, 14 Sep 2022 17:54:05 +0000 Subject: [PATCH 11/54] Fix + tests --- src/Core/SortDescription.cpp | 4 ++-- .../QueryPlan/Optimizations/Optimizations.h | 2 +- src/Processors/QueryPlan/ReadFromMergeTree.cpp | 11 +++++------ .../02317_distinct_in_order_optimization.reference | 2 ++ .../02317_distinct_in_order_optimization.sql | 10 +++++++++- ...7_distinct_in_order_optimization_explain.reference | 5 +++++ .../02317_distinct_in_order_optimization_explain.sh | 4 ++++ 7 files changed, 28 insertions(+), 10 deletions(-) diff --git a/src/Core/SortDescription.cpp b/src/Core/SortDescription.cpp index 047b14cb8a4..66f5b76721b 100644 --- a/src/Core/SortDescription.cpp +++ b/src/Core/SortDescription.cpp @@ -53,9 +53,9 @@ size_t SortDescription::hasPrefix(const SortDescription & prefix) const for (size_t i = 0; i < prefix.size(); ++i) { if ((*this)[i] != prefix[i]) - return i; + return false; } - return size(); + return true; } #if USE_EMBEDDED_COMPILER diff --git a/src/Processors/QueryPlan/Optimizations/Optimizations.h b/src/Processors/QueryPlan/Optimizations/Optimizations.h index 99c9afbd083..4f2e177aaf6 100644 --- a/src/Processors/QueryPlan/Optimizations/Optimizations.h +++ b/src/Processors/QueryPlan/Optimizations/Optimizations.h @@ -67,7 +67,7 @@ inline const auto & getOptimizations() {tryPushDownFilter, "pushDownFilter", &QueryPlanOptimizationSettings::filter_push_down}, {tryExecuteFunctionsAfterSorting, "liftUpFunctions", &QueryPlanOptimizationSettings::optimize_plan}, {tryReuseStorageOrderingForWindowFunctions, "reuseStorageOrderingForWindowFunctions", &QueryPlanOptimizationSettings::optimize_plan}, - {tryDistinctReadInOrder, "distinctReadInOrder", &QueryPlanOptimizationSettings::distinct_in_order} + {tryDistinctReadInOrder, "distinctReadInOrder", &QueryPlanOptimizationSettings::distinct_in_order}, }}; return optimizations; diff --git a/src/Processors/QueryPlan/ReadFromMergeTree.cpp b/src/Processors/QueryPlan/ReadFromMergeTree.cpp index 984d92e56ef..45a731878a8 100644 --- a/src/Processors/QueryPlan/ReadFromMergeTree.cpp +++ b/src/Processors/QueryPlan/ReadFromMergeTree.cpp @@ -1039,12 +1039,11 @@ void ReadFromMergeTree::setQueryInfoInputOrderInfo(InputOrderInfoPtr order_info) /// update sort info for output stream SortDescription & current_sort_desc = output_stream->sort_description; - size_t prefix_size = current_sort_desc.hasPrefix(order_info->sort_description_for_merging); - if (!prefix_size) - return; - - current_sort_desc.resize(prefix_size); - output_stream->sort_scope = DataStream::SortScope::Stream; + if (current_sort_desc.hasPrefix(order_info->sort_description_for_merging)) + { + current_sort_desc.resize(order_info->sort_description_for_merging.size()); + output_stream->sort_scope = DataStream::SortScope::Stream; + } } ReadFromMergeTree::AnalysisResult ReadFromMergeTree::getAnalysisResult() const diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization.reference b/tests/queries/0_stateless/02317_distinct_in_order_optimization.reference index 22ecaafe989..628c2fc0714 100644 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization.reference +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization.reference @@ -108,3 +108,5 @@ select distinct a, b, x, y from (select a, b, 1 as x, 2 as y from distinct_in_or 0 -- check that distinct in order WITHOUT order by returns the same result as ordinary distinct 0 +-- check that distinct in order WITHOUT order by and WITH filter returns the same result as ordinary distinct +0 diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization.sql b/tests/queries/0_stateless/02317_distinct_in_order_optimization.sql index f0c7a11f418..a1e7d7340a3 100644 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization.sql +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization.sql @@ -85,5 +85,13 @@ select count() as diff from (select distinct * from distinct_in_order except sel drop table if exists distinct_in_order; drop table if exists ordinary_distinct; -drop table if exists distinct_cardinality_low; +select '-- check that distinct in order WITHOUT order by and WITH filter returns the same result as ordinary distinct'; +create table distinct_in_order (low UInt64, medium UInt64, high UInt64) engine=MergeTree() order by (low, medium); +insert into distinct_in_order select distinct * from distinct_cardinality_low where low > 0 settings optimize_distinct_in_order=1; +create table ordinary_distinct (low UInt64, medium UInt64, high UInt64) engine=MergeTree() order by (low, medium); +insert into ordinary_distinct select distinct * from distinct_cardinality_low where low > 0 settings optimize_distinct_in_order=0; +select count() as diff from (select distinct * from distinct_in_order except select * from ordinary_distinct); +drop table if exists distinct_in_order; +drop table if exists ordinary_distinct; +drop table if exists distinct_cardinality_low; diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference index 9b43eb81747..de429e4869d 100644 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference @@ -46,3 +46,8 @@ MergeTreeThread MergeTreeInOrder -- enabled, distinct columns contains constant columns, non-const columns match prefix of sorting key MergeTreeInOrder +-- check that sorting properties are propagated from ReadFromMergeTree till preliminary distinct +Sorting (Stream): a ASC, b ASC +Sorting (Stream): a ASC, b ASC +Sorting (Stream): a ASC, b ASC +Sorting (Stream): a ASC, b ASC diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh index bfbf3b6de0c..6ce6b9f63b9 100755 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh @@ -14,6 +14,7 @@ REMOVE_NON_LETTERS="sed 's/[^a-zA-Z]//g'" FIND_DISTINCT="$GREP_DISTINCT | $TRIM_LEADING_SPACES | $REMOVE_NON_LETTERS" FIND_READING_IN_ORDER="grep 'MergeTreeInOrder' | $TRIM_LEADING_SPACES | $REMOVE_NON_LETTERS" FIND_READING_DEFAULT="grep 'MergeTreeThread' | $TRIM_LEADING_SPACES | $REMOVE_NON_LETTERS" +FIND_SORTING_PROPERTIES="grep 'Sorting (Stream)' | $TRIM_LEADING_SPACES" $CLICKHOUSE_CLIENT -q "drop table if exists distinct_in_order_explain sync" $CLICKHOUSE_CLIENT -q "create table distinct_in_order_explain (a int, b int, c int) engine=MergeTree() order by (a, b)" @@ -69,4 +70,7 @@ $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct 1, echo "-- enabled, distinct columns contains constant columns, non-const columns match prefix of sorting key" $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct 1, b, a from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER +echo "-- check that sorting properties are propagated from ReadFromMergeTree till preliminary distinct" +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain plan sorting=1 select distinct b, a from distinct_in_order_explain where a > 0" | eval $FIND_SORTING_PROPERTIES + $CLICKHOUSE_CLIENT -q "drop table if exists distinct_in_order_explain sync" From f011f721146fd2e6cef36fbf0dc46635a893379c Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Wed, 14 Sep 2022 20:55:49 +0000 Subject: [PATCH 12/54] Try to apply optimization only if optimizer reaches pre distinct node + more comments --- src/Core/SortDescription.cpp | 2 +- src/Core/SortDescription.h | 2 +- .../QueryPlan/Optimizations/Optimizations.h | 2 +- .../Optimizations/distinctReadInOrder.cpp | 56 +++++++++---------- 4 files changed, 29 insertions(+), 33 deletions(-) diff --git a/src/Core/SortDescription.cpp b/src/Core/SortDescription.cpp index 66f5b76721b..59018fb13b4 100644 --- a/src/Core/SortDescription.cpp +++ b/src/Core/SortDescription.cpp @@ -42,7 +42,7 @@ void SortColumnDescription::explain(JSONBuilder::JSONMap & map) const map.add("With Fill", with_fill); } -size_t SortDescription::hasPrefix(const SortDescription & prefix) const +bool SortDescription::hasPrefix(const SortDescription & prefix) const { if (prefix.empty()) return true; diff --git a/src/Core/SortDescription.h b/src/Core/SortDescription.h index 9eaf4ce1da3..a697323b593 100644 --- a/src/Core/SortDescription.h +++ b/src/Core/SortDescription.h @@ -121,7 +121,7 @@ public: size_t min_count_to_compile_sort_description = 3; bool compile_sort_description = false; - size_t hasPrefix(const SortDescription & prefix) const; + bool hasPrefix(const SortDescription & prefix) const; }; /** Compile sort description for header_types. diff --git a/src/Processors/QueryPlan/Optimizations/Optimizations.h b/src/Processors/QueryPlan/Optimizations/Optimizations.h index 4f2e177aaf6..25825f2f5b9 100644 --- a/src/Processors/QueryPlan/Optimizations/Optimizations.h +++ b/src/Processors/QueryPlan/Optimizations/Optimizations.h @@ -54,7 +54,7 @@ size_t tryExecuteFunctionsAfterSorting(QueryPlan::Node * parent_node, QueryPlan: /// Update information about prefix sort description in SortingStep. size_t tryReuseStorageOrderingForWindowFunctions(QueryPlan::Node * parent_node, QueryPlan::Nodes & nodes); -/// TODO: description +/// Reading in order from MergeTree table if DISTINCT columns match or form a prefix of MergeTree sorting key size_t tryDistinctReadInOrder(QueryPlan::Node * node, QueryPlan::Nodes & nodes); inline const auto & getOptimizations() diff --git a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp index f48340b0417..1f493331498 100644 --- a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp +++ b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp @@ -11,41 +11,34 @@ namespace DB::QueryPlanOptimizations { -/// size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) { - /// walk through the plan - /// (1) check if there is preliminary distinct node - /// (2) check if nodes below preliminary distinct preserve sorting - QueryPlan::Node * node = parent_node; + /// check if it is preliminary distinct node DistinctStep * pre_distinct = nullptr; - QueryPlan::Node * pre_distinct_node = nullptr; - while (!node->children.empty()) + if (auto * distinct = typeid_cast(parent_node->step.get()); distinct) { - if (pre_distinct) - { - /// check if nodes below DISTINCT preserve sorting - const auto * step = dynamic_cast(node->step.get()); - if (step) - { - const ITransformingStep::DataStreamTraits & traits = step->getDataStreamTraits(); - if (!traits.preserves_sorting) - return 0; - } - } - if (auto * tmp = typeid_cast(node->step.get()); tmp) - { - if (tmp->isPreliminary()) - { - pre_distinct_node = node; - pre_distinct = tmp; - } - } - node = node->children.front(); + if (distinct->isPreliminary()) + pre_distinct = distinct; } if (!pre_distinct) return 0; + /// walk through the plan + /// check if nodes below preliminary distinct preserve sorting + QueryPlan::Node * node = parent_node; + while (!node->children.empty()) + { + const auto * step = dynamic_cast(node->step.get()); + if (step) + { + const ITransformingStep::DataStreamTraits & traits = step->getDataStreamTraits(); + if (!traits.preserves_sorting) + return 0; + } + node = node->children.front(); + } + + /// check if we read from MergeTree auto * read_from_merge_tree = typeid_cast(node->step.get()); if (!read_from_merge_tree) return 0; @@ -55,7 +48,7 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) if (output.sort_scope != DataStream::SortScope::Chunk) return 0; - const SortDescription & sort_desc = output.sort_description; + /// find non-const columns in DISTINCT const auto & distinct_columns = pre_distinct->getOutputStream().header.getColumnsWithTypeAndName(); std::vector non_const_columns; non_const_columns.reserve(distinct_columns.size()); @@ -67,6 +60,7 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) /// apply optimization only when distinct columns match or form prefix of sorting key /// todo: check if reading in order optimization would be beneficial when sorting key is prefix of columns in DISTINCT + const SortDescription & sort_desc = output.sort_description; if (sort_desc.size() < non_const_columns.size()) return 0; @@ -83,14 +77,15 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) if (distinct_sort_desc.empty()) return 0; + /// update input order info in read_from_merge_tree step const int direction = 1; /// default direction, ASC InputOrderInfoPtr order_info = std::make_shared(distinct_sort_desc, distinct_sort_desc.size(), direction, pre_distinct->getLimitHint()); read_from_merge_tree->setQueryInfoInputOrderInfo(order_info); - /// update data stream's sorting properties + /// find all transforms between preliminary distinct step and ReadFromMergeTree std::vector steps2update; - node = pre_distinct_node; + node = parent_node; while (node && node->step.get() != read_from_merge_tree) { auto * transform = dynamic_cast(node->step.get()); @@ -103,6 +98,7 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) node = nullptr; } + /// update data stream's sorting properties for found transforms const DataStream * input_stream = &read_from_merge_tree->getOutputStream(); while (!steps2update.empty()) { From 8a4806e8c041b7761827dbee22a78512f2e28fee Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Thu, 15 Sep 2022 10:53:42 +0000 Subject: [PATCH 13/54] Fix test - remove perfomance queries which can be unstable --- tests/performance/distinct_in_order.xml | 10 ---------- .../02317_distinct_in_order_optimization_explain.sh | 11 ++++++----- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/tests/performance/distinct_in_order.xml b/tests/performance/distinct_in_order.xml index ee1094ed395..78856b507e7 100644 --- a/tests/performance/distinct_in_order.xml +++ b/tests/performance/distinct_in_order.xml @@ -4,15 +4,10 @@ INSERT INTO distinct_cardinality_high SELECT number % 1e6, number % 1e4, number % 1e2 FROM numbers_mt(1e8) SELECT DISTINCT high FROM distinct_cardinality_high FORMAT Null - SELECT DISTINCT high, low FROM distinct_cardinality_high FORMAT Null SELECT DISTINCT high, medium FROM distinct_cardinality_high FORMAT Null - SELECT DISTINCT high, medium, low FROM distinct_cardinality_high FORMAT Null SELECT DISTINCT high, medium FROM distinct_cardinality_high ORDER BY high, medium FORMAT Null SELECT DISTINCT high, medium FROM distinct_cardinality_high ORDER BY high FORMAT Null - SELECT DISTINCT high, medium FROM distinct_cardinality_high ORDER BY medium FORMAT Null - SELECT DISTINCT high, low FROM distinct_cardinality_high ORDER BY low FORMAT Null - SELECT DISTINCT high, medium, low FROM distinct_cardinality_high ORDER BY low FORMAT Null DROP TABLE IF EXISTS distinct_cardinality_high @@ -22,14 +17,9 @@ SELECT DISTINCT low FROM distinct_cardinality_low FORMAT Null SELECT DISTINCT low, medium FROM distinct_cardinality_low FORMAT Null - SELECT DISTINCT low, high FROM distinct_cardinality_low FORMAT Null - SELECT DISTINCT low, medium, high FROM distinct_cardinality_low FORMAT Null SELECT DISTINCT low, medium FROM distinct_cardinality_low ORDER BY low, medium FORMAT Null SELECT DISTINCT low, medium FROM distinct_cardinality_low ORDER BY low FORMAT Null - SELECT DISTINCT low, medium FROM distinct_cardinality_low ORDER BY medium FORMAT Null - SELECT DISTINCT low, high FROM distinct_cardinality_low ORDER BY high FORMAT Null - SELECT DISTINCT low, medium, high FROM distinct_cardinality_low ORDER BY high FORMAT Null DROP TABLE IF EXISTS distinct_cardinality_low diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh index 6ce6b9f63b9..db1ea703e61 100755 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh @@ -60,17 +60,18 @@ echo "-- Check reading in order for distinct" echo "-- disabled, distinct columns match sorting key" $CLICKHOUSE_CLIENT -nq "$DISABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_DEFAULT echo "-- enabled, distinct columns match sorting key" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER +# read_in_order_two_level_merge_threshold is set here to avoid repeating MergeTreeInOrder in output +$CLICKHOUSE_CLIENT --read_in_order_two_level_merge_threshold=2 -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER echo "-- enabled, distinct columns form prefix of sorting key" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER +$CLICKHOUSE_CLIENT --read_in_order_two_level_merge_threshold=2 -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER echo "-- enabled, distinct columns DON't form prefix of sorting key" $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct b from distinct_in_order_explain" | eval $FIND_READING_DEFAULT echo "-- enabled, distinct columns contains constant columns, non-const columns form prefix of sorting key" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct 1, a from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER +$CLICKHOUSE_CLIENT --read_in_order_two_level_merge_threshold=2 -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct 1, a from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER echo "-- enabled, distinct columns contains constant columns, non-const columns match prefix of sorting key" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct 1, b, a from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER +$CLICKHOUSE_CLIENT --read_in_order_two_level_merge_threshold=2 -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct 1, b, a from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER echo "-- check that sorting properties are propagated from ReadFromMergeTree till preliminary distinct" $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain plan sorting=1 select distinct b, a from distinct_in_order_explain where a > 0" | eval $FIND_SORTING_PROPERTIES -$CLICKHOUSE_CLIENT -q "drop table if exists distinct_in_order_explain sync" +# $CLICKHOUSE_CLIENT -q "drop table if exists distinct_in_order_explain sync" From 327d28118bd9db997d37bba85106b288f92babbd Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Thu, 15 Sep 2022 21:05:29 +0000 Subject: [PATCH 14/54] Fix flaky test --- .../02317_distinct_in_order_optimization_explain.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh index db1ea703e61..1e5800a5f60 100755 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh @@ -58,14 +58,14 @@ $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct b, echo "-- Check reading in order for distinct" echo "-- disabled, distinct columns match sorting key" -$CLICKHOUSE_CLIENT -nq "$DISABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_DEFAULT +$CLICKHOUSE_CLIENT --max_threads=0 -nq "$DISABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_DEFAULT echo "-- enabled, distinct columns match sorting key" # read_in_order_two_level_merge_threshold is set here to avoid repeating MergeTreeInOrder in output $CLICKHOUSE_CLIENT --read_in_order_two_level_merge_threshold=2 -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER echo "-- enabled, distinct columns form prefix of sorting key" $CLICKHOUSE_CLIENT --read_in_order_two_level_merge_threshold=2 -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER echo "-- enabled, distinct columns DON't form prefix of sorting key" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct b from distinct_in_order_explain" | eval $FIND_READING_DEFAULT +$CLICKHOUSE_CLIENT --max_threads=0 -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct b from distinct_in_order_explain" | eval $FIND_READING_DEFAULT echo "-- enabled, distinct columns contains constant columns, non-const columns form prefix of sorting key" $CLICKHOUSE_CLIENT --read_in_order_two_level_merge_threshold=2 -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct 1, a from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER echo "-- enabled, distinct columns contains constant columns, non-const columns match prefix of sorting key" @@ -74,4 +74,4 @@ $CLICKHOUSE_CLIENT --read_in_order_two_level_merge_threshold=2 -nq "$ENABLE_OPTI echo "-- check that sorting properties are propagated from ReadFromMergeTree till preliminary distinct" $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain plan sorting=1 select distinct b, a from distinct_in_order_explain where a > 0" | eval $FIND_SORTING_PROPERTIES -# $CLICKHOUSE_CLIENT -q "drop table if exists distinct_in_order_explain sync" +$CLICKHOUSE_CLIENT -q "drop table if exists distinct_in_order_explain sync" From ba1f7de243456477e227c2b8e6511869b2173248 Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Fri, 16 Sep 2022 10:30:30 +0000 Subject: [PATCH 15/54] Fix review comments --- src/Core/SortDescription.h | 1 - .../Optimizations/distinctReadInOrder.cpp | 31 ++++++++----------- .../QueryPlan/ReadFromMergeTree.cpp | 8 ++--- 3 files changed, 15 insertions(+), 25 deletions(-) diff --git a/src/Core/SortDescription.h b/src/Core/SortDescription.h index a697323b593..0025e44b489 100644 --- a/src/Core/SortDescription.h +++ b/src/Core/SortDescription.h @@ -48,7 +48,6 @@ struct SortColumnDescription bool with_fill; FillColumnDescription fill_description; - SortColumnDescription() = default; explicit SortColumnDescription( const std::string & column_name_, int direction_ = 1, diff --git a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp index 1f493331498..8fda4a059ca 100644 --- a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp +++ b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp @@ -29,12 +29,13 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) while (!node->children.empty()) { const auto * step = dynamic_cast(node->step.get()); - if (step) - { - const ITransformingStep::DataStreamTraits & traits = step->getDataStreamTraits(); - if (!traits.preserves_sorting) - return 0; - } + if (!step) + return 0; + + const ITransformingStep::DataStreamTraits & traits = step->getDataStreamTraits(); + if (!traits.preserves_sorting) + return 0; + node = node->children.front(); } @@ -43,24 +44,18 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) if (!read_from_merge_tree) return 0; - /// check if reading in order is already there - const DataStream & output = read_from_merge_tree->getOutputStream(); - if (output.sort_scope != DataStream::SortScope::Chunk) - return 0; - /// find non-const columns in DISTINCT const auto & distinct_columns = pre_distinct->getOutputStream().header.getColumnsWithTypeAndName(); - std::vector non_const_columns; - non_const_columns.reserve(distinct_columns.size()); + std::set non_const_columns; for (const auto & column : distinct_columns) { if (!isColumnConst(*column.column)) - non_const_columns.emplace_back(column.name); + non_const_columns.emplace(column.name); } /// apply optimization only when distinct columns match or form prefix of sorting key /// todo: check if reading in order optimization would be beneficial when sorting key is prefix of columns in DISTINCT - const SortDescription & sort_desc = output.sort_description; + const SortDescription & sort_desc = read_from_merge_tree->getOutputStream().sort_description; if (sort_desc.size() < non_const_columns.size()) return 0; @@ -69,7 +64,7 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) distinct_sort_desc.reserve(sort_desc.size()); for (const auto & column_desc : sort_desc) { - if (non_const_columns.end() == std::find(begin(non_const_columns), end(non_const_columns), column_desc.column_name)) + if (non_const_columns.end() == non_const_columns.find(column_desc.column_name)) break; distinct_sort_desc.push_back(column_desc); @@ -89,8 +84,8 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) while (node && node->step.get() != read_from_merge_tree) { auto * transform = dynamic_cast(node->step.get()); - if (transform) - steps2update.push_back(transform); + assert(transform); /// all nodes contain transformations, we checked it before + steps2update.push_back(transform); if (!node->children.empty()) node = node->children.front(); diff --git a/src/Processors/QueryPlan/ReadFromMergeTree.cpp b/src/Processors/QueryPlan/ReadFromMergeTree.cpp index 45a731878a8..51d61306db3 100644 --- a/src/Processors/QueryPlan/ReadFromMergeTree.cpp +++ b/src/Processors/QueryPlan/ReadFromMergeTree.cpp @@ -1038,12 +1038,8 @@ void ReadFromMergeTree::setQueryInfoInputOrderInfo(InputOrderInfoPtr order_info) } /// update sort info for output stream - SortDescription & current_sort_desc = output_stream->sort_description; - if (current_sort_desc.hasPrefix(order_info->sort_description_for_merging)) - { - current_sort_desc.resize(order_info->sort_description_for_merging.size()); - output_stream->sort_scope = DataStream::SortScope::Stream; - } + output_stream->sort_description = order_info->sort_description_for_merging; + output_stream->sort_scope = DataStream::SortScope::Stream; } ReadFromMergeTree::AnalysisResult ReadFromMergeTree::getAnalysisResult() const From b34ec6453b04cbdef370c46facee215e420b2bd2 Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Fri, 16 Sep 2022 17:11:08 +0000 Subject: [PATCH 16/54] Fix review comments + one more test --- .../Optimizations/distinctReadInOrder.cpp | 26 +++++-------------- ...ct_in_order_optimization_explain.reference | 2 ++ ..._distinct_in_order_optimization_explain.sh | 2 ++ 3 files changed, 11 insertions(+), 19 deletions(-) diff --git a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp index 8fda4a059ca..e5858827207 100644 --- a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp +++ b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp @@ -10,7 +10,6 @@ namespace DB::QueryPlanOptimizations { - size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) { /// check if it is preliminary distinct node @@ -24,11 +23,13 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) return 0; /// walk through the plan - /// check if nodes below preliminary distinct preserve sorting + /// (1) check if nodes below preliminary distinct preserve sorting + /// (2) gather transforming steps to update their sorting properties later + std::vector steps2update; QueryPlan::Node * node = parent_node; while (!node->children.empty()) { - const auto * step = dynamic_cast(node->step.get()); + auto * step = dynamic_cast(node->step.get()); if (!step) return 0; @@ -36,6 +37,8 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) if (!traits.preserves_sorting) return 0; + steps2update.push_back(step); + node = node->children.front(); } @@ -69,7 +72,7 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) distinct_sort_desc.push_back(column_desc); } - if (distinct_sort_desc.empty()) + if (distinct_sort_desc.size() != non_const_columns.size()) return 0; /// update input order info in read_from_merge_tree step @@ -78,21 +81,6 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) = std::make_shared(distinct_sort_desc, distinct_sort_desc.size(), direction, pre_distinct->getLimitHint()); read_from_merge_tree->setQueryInfoInputOrderInfo(order_info); - /// find all transforms between preliminary distinct step and ReadFromMergeTree - std::vector steps2update; - node = parent_node; - while (node && node->step.get() != read_from_merge_tree) - { - auto * transform = dynamic_cast(node->step.get()); - assert(transform); /// all nodes contain transformations, we checked it before - steps2update.push_back(transform); - - if (!node->children.empty()) - node = node->children.front(); - else - node = nullptr; - } - /// update data stream's sorting properties for found transforms const DataStream * input_stream = &read_from_merge_tree->getOutputStream(); while (!steps2update.empty()) diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference index de429e4869d..41700d7b027 100644 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference @@ -46,6 +46,8 @@ MergeTreeThread MergeTreeInOrder -- enabled, distinct columns contains constant columns, non-const columns match prefix of sorting key MergeTreeInOrder +-- enabled, only part of distinct columns form prefix of sorting key +MergeTreeThread -- check that sorting properties are propagated from ReadFromMergeTree till preliminary distinct Sorting (Stream): a ASC, b ASC Sorting (Stream): a ASC, b ASC diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh index 1e5800a5f60..bb35f7738e9 100755 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh @@ -70,6 +70,8 @@ echo "-- enabled, distinct columns contains constant columns, non-const columns $CLICKHOUSE_CLIENT --read_in_order_two_level_merge_threshold=2 -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct 1, a from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER echo "-- enabled, distinct columns contains constant columns, non-const columns match prefix of sorting key" $CLICKHOUSE_CLIENT --read_in_order_two_level_merge_threshold=2 -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct 1, b, a from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER +echo "-- enabled, only part of distinct columns form prefix of sorting key" +$CLICKHOUSE_CLIENT --read_in_order_two_level_merge_threshold=2 -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, c from distinct_in_order_explain" | eval $FIND_READING_DEFAULT echo "-- check that sorting properties are propagated from ReadFromMergeTree till preliminary distinct" $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain plan sorting=1 select distinct b, a from distinct_in_order_explain where a > 0" | eval $FIND_SORTING_PROPERTIES From 24de5530d6b476fa83998a63a5b243755de8718a Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Fri, 16 Sep 2022 17:16:32 +0000 Subject: [PATCH 17/54] Remove unnecessary headers --- src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp index e5858827207..3999146c7b1 100644 --- a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp +++ b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp @@ -1,11 +1,8 @@ #include -#include -#include #include #include #include #include -#include #include namespace DB::QueryPlanOptimizations From fa71d9995600a750ae35aa0f36ef05f98bf46ece Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Sat, 17 Sep 2022 08:51:53 +0000 Subject: [PATCH 18/54] Fix flaky test --- .../0_stateless/02317_distinct_in_order_optimization_explain.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh index bb35f7738e9..f1a741ff783 100755 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh @@ -71,7 +71,7 @@ $CLICKHOUSE_CLIENT --read_in_order_two_level_merge_threshold=2 -nq "$ENABLE_OPTI echo "-- enabled, distinct columns contains constant columns, non-const columns match prefix of sorting key" $CLICKHOUSE_CLIENT --read_in_order_two_level_merge_threshold=2 -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct 1, b, a from distinct_in_order_explain" | eval $FIND_READING_IN_ORDER echo "-- enabled, only part of distinct columns form prefix of sorting key" -$CLICKHOUSE_CLIENT --read_in_order_two_level_merge_threshold=2 -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, c from distinct_in_order_explain" | eval $FIND_READING_DEFAULT +$CLICKHOUSE_CLIENT --max_threads=0 -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, c from distinct_in_order_explain" | eval $FIND_READING_DEFAULT echo "-- check that sorting properties are propagated from ReadFromMergeTree till preliminary distinct" $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain plan sorting=1 select distinct b, a from distinct_in_order_explain where a > 0" | eval $FIND_SORTING_PROPERTIES From 785c33bf7d4bea81d567f9517f6bcef958e604c6 Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Sun, 18 Sep 2022 23:00:04 +0000 Subject: [PATCH 19/54] Updating sort description for ReadFromMergeTree correctly --- src/Core/SortDescription.h | 2 ++ .../Optimizations/distinctReadInOrder.cpp | 7 +++++- .../QueryPlan/ReadFromMergeTree.cpp | 24 ++++++++++++++++++- src/Storages/ReadInOrderOptimizer.cpp | 2 +- 4 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/Core/SortDescription.h b/src/Core/SortDescription.h index 0025e44b489..20a4bef8176 100644 --- a/src/Core/SortDescription.h +++ b/src/Core/SortDescription.h @@ -48,6 +48,8 @@ struct SortColumnDescription bool with_fill; FillColumnDescription fill_description; + SortColumnDescription() = default; + explicit SortColumnDescription( const std::string & column_name_, int direction_ = 1, diff --git a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp index 3999146c7b1..f78483e57d2 100644 --- a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp +++ b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp @@ -44,6 +44,11 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) if (!read_from_merge_tree) return 0; + /// if some `read in order` optimization is already applied - do not proceed + /// todo: check later case with several read in order optimizations + if (read_from_merge_tree->getOutputStream().sort_scope != DataStream::SortScope::Chunk) + return 0; + /// find non-const columns in DISTINCT const auto & distinct_columns = pre_distinct->getOutputStream().header.getColumnsWithTypeAndName(); std::set non_const_columns; @@ -75,7 +80,7 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) /// update input order info in read_from_merge_tree step const int direction = 1; /// default direction, ASC InputOrderInfoPtr order_info - = std::make_shared(distinct_sort_desc, distinct_sort_desc.size(), direction, pre_distinct->getLimitHint()); + = std::make_shared(SortDescription{}, distinct_sort_desc.size(), direction, pre_distinct->getLimitHint()); read_from_merge_tree->setQueryInfoInputOrderInfo(order_info); /// update data stream's sorting properties for found transforms diff --git a/src/Processors/QueryPlan/ReadFromMergeTree.cpp b/src/Processors/QueryPlan/ReadFromMergeTree.cpp index 51d61306db3..b222471ccde 100644 --- a/src/Processors/QueryPlan/ReadFromMergeTree.cpp +++ b/src/Processors/QueryPlan/ReadFromMergeTree.cpp @@ -143,7 +143,12 @@ ReadFromMergeTree::ReadFromMergeTree( { auto const & settings = context->getSettingsRef(); if ((settings.optimize_read_in_order || settings.optimize_aggregation_in_order) && query_info.getInputOrderInfo()) + { output_stream->sort_scope = DataStream::SortScope::Stream; + const size_t used_prefix_of_sorting_key_size = query_info.getInputOrderInfo()->used_prefix_of_sorting_key_size; + if (sort_description.size() > used_prefix_of_sorting_key_size) + sort_description.resize(used_prefix_of_sorting_key_size); + } else output_stream->sort_scope = DataStream::SortScope::Chunk; } @@ -1038,7 +1043,24 @@ void ReadFromMergeTree::setQueryInfoInputOrderInfo(InputOrderInfoPtr order_info) } /// update sort info for output stream - output_stream->sort_description = order_info->sort_description_for_merging; + SortDescription sort_description; + const Names & sorting_key_columns = storage_snapshot->getMetadataForQuery()->getSortingKeyColumns(); + const Block & header = output_stream->header; + const int sort_direction = getSortDirection(); + for (const auto & column_name : sorting_key_columns) + { + if (std::find_if(header.begin(), header.end(), [&](ColumnWithTypeAndName const & col) { return col.name == column_name; }) + == header.end()) + break; + sort_description.emplace_back(column_name, sort_direction); + } + if (sort_description.empty()) + throw Exception(ErrorCodes::LOGICAL_ERROR, "Sort desctiption can't be empty when reading in order"); + + const size_t used_prefix_of_sorting_key_size = order_info->used_prefix_of_sorting_key_size; + if (sort_description.size() > used_prefix_of_sorting_key_size) + sort_description.resize(used_prefix_of_sorting_key_size); + output_stream->sort_description = std::move(sort_description); output_stream->sort_scope = DataStream::SortScope::Stream; } diff --git a/src/Storages/ReadInOrderOptimizer.cpp b/src/Storages/ReadInOrderOptimizer.cpp index b67da14365e..1fa46c793ed 100644 --- a/src/Storages/ReadInOrderOptimizer.cpp +++ b/src/Storages/ReadInOrderOptimizer.cpp @@ -202,7 +202,7 @@ InputOrderInfoPtr ReadInOrderOptimizer::getInputOrderImpl( const ContextPtr & context, UInt64 limit) const { - auto sorting_key_columns = metadata_snapshot->getSortingKeyColumns(); + const Names & sorting_key_columns = metadata_snapshot->getSortingKeyColumns(); int read_direction = description.at(0).direction; auto fixed_sorting_columns = getFixedSortingColumns(query, sorting_key_columns, context); From b4a3ac926f2bb7fb16d1edb40610143dd58a6a9a Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Mon, 19 Sep 2022 08:26:44 +0000 Subject: [PATCH 20/54] Fix typo --- src/Processors/QueryPlan/ReadFromMergeTree.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Processors/QueryPlan/ReadFromMergeTree.cpp b/src/Processors/QueryPlan/ReadFromMergeTree.cpp index b222471ccde..7d5a04aa115 100644 --- a/src/Processors/QueryPlan/ReadFromMergeTree.cpp +++ b/src/Processors/QueryPlan/ReadFromMergeTree.cpp @@ -1055,7 +1055,7 @@ void ReadFromMergeTree::setQueryInfoInputOrderInfo(InputOrderInfoPtr order_info) sort_description.emplace_back(column_name, sort_direction); } if (sort_description.empty()) - throw Exception(ErrorCodes::LOGICAL_ERROR, "Sort desctiption can't be empty when reading in order"); + throw Exception(ErrorCodes::LOGICAL_ERROR, "Sort description can't be empty when reading in order"); const size_t used_prefix_of_sorting_key_size = order_info->used_prefix_of_sorting_key_size; if (sort_description.size() > used_prefix_of_sorting_key_size) From 78bb598d646dffdce8c942f45b21192e6474179c Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Mon, 19 Sep 2022 18:33:05 +0000 Subject: [PATCH 21/54] Handling DISTINCT in order on top of other read in order optimization --- .../Optimizations/distinctReadInOrder.cpp | 39 ++++++++++--------- ...ct_in_order_optimization_explain.reference | 17 +++++++- ..._distinct_in_order_optimization_explain.sh | 11 +++++- 3 files changed, 46 insertions(+), 21 deletions(-) diff --git a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp index f78483e57d2..bb566babe72 100644 --- a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp +++ b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp @@ -44,13 +44,8 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) if (!read_from_merge_tree) return 0; - /// if some `read in order` optimization is already applied - do not proceed - /// todo: check later case with several read in order optimizations - if (read_from_merge_tree->getOutputStream().sort_scope != DataStream::SortScope::Chunk) - return 0; - /// find non-const columns in DISTINCT - const auto & distinct_columns = pre_distinct->getOutputStream().header.getColumnsWithTypeAndName(); + const ColumnsWithTypeAndName & distinct_columns = pre_distinct->getOutputStream().header.getColumnsWithTypeAndName(); std::set non_const_columns; for (const auto & column : distinct_columns) { @@ -58,29 +53,35 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) non_const_columns.emplace(column.name); } - /// apply optimization only when distinct columns match or form prefix of sorting key - /// todo: check if reading in order optimization would be beneficial when sorting key is prefix of columns in DISTINCT - const SortDescription & sort_desc = read_from_merge_tree->getOutputStream().sort_description; - if (sort_desc.size() < non_const_columns.size()) - return 0; - + const Names& sorting_key_columns = read_from_merge_tree->getStorageMetadata()->getSortingKeyColumns(); /// check if DISTINCT has the same columns as sorting key - SortDescription distinct_sort_desc; - distinct_sort_desc.reserve(sort_desc.size()); - for (const auto & column_desc : sort_desc) + size_t number_of_sorted_distinct_columns = 0; + for (const auto & column_name : sorting_key_columns) { - if (non_const_columns.end() == non_const_columns.find(column_desc.column_name)) + if (non_const_columns.end() == non_const_columns.find(column_name)) break; - distinct_sort_desc.push_back(column_desc); + ++number_of_sorted_distinct_columns; } - if (distinct_sort_desc.size() != non_const_columns.size()) + /// apply optimization only when distinct columns match or form prefix of sorting key + /// todo: check if reading in order optimization would be beneficial when sorting key is prefix of columns in DISTINCT + if (number_of_sorted_distinct_columns != non_const_columns.size()) + return 0; + + /// check if another read in order optimization is already applied + /// apply optimization only if another read in order one uses less sorting columns + /// example: SELECT DISTINCT a, b FROM t ORDER BY a; -- sorting key: a, b + /// if read in order for ORDER BY is already applied, then output sort description will contain only column `a` + /// but we need columns `a, b`, applying read in order for distinct will still benefit `order by` + const DataStream & output_data_stream = read_from_merge_tree->getOutputStream(); + const SortDescription & output_sort_desc = output_data_stream.sort_description; + if (output_data_stream.sort_scope != DataStream::SortScope::Chunk && number_of_sorted_distinct_columns <= output_sort_desc.size()) return 0; /// update input order info in read_from_merge_tree step const int direction = 1; /// default direction, ASC InputOrderInfoPtr order_info - = std::make_shared(SortDescription{}, distinct_sort_desc.size(), direction, pre_distinct->getLimitHint()); + = std::make_shared(SortDescription{}, number_of_sorted_distinct_columns, direction, pre_distinct->getLimitHint()); read_from_merge_tree->setQueryInfoInputOrderInfo(order_info); /// update data stream's sorting properties for found transforms diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference index 41700d7b027..881856edf0f 100644 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference @@ -15,6 +15,9 @@ DistinctSortedChunkTransform -- distinct with primary key prefix and order by the same columns -> pre-distinct and final distinct optimization DistinctSortedStreamTransform DistinctSortedChunkTransform +-- distinct with primary key prefix and order by columns are prefix of distinct columns -> pre-distinct and final distinct optimization +DistinctSortedTransform +DistinctSortedChunkTransform -- distinct with primary key prefix and order by column in distinct but non-primary key prefix -> pre-distinct and final distinct optimization DistinctSortedTransform DistinctSortedChunkTransform @@ -48,8 +51,20 @@ MergeTreeInOrder MergeTreeInOrder -- enabled, only part of distinct columns form prefix of sorting key MergeTreeThread --- check that sorting properties are propagated from ReadFromMergeTree till preliminary distinct +-- enabled, check that sorting properties are propagated from ReadFromMergeTree till preliminary distinct Sorting (Stream): a ASC, b ASC Sorting (Stream): a ASC, b ASC Sorting (Stream): a ASC, b ASC Sorting (Stream): a ASC, b ASC +-- disabled, check that sorting description for ReadFromMergeTree match ORDER BY columns +Sorting (Stream): a ASC +Sorting (Stream): a ASC +Sorting (Stream): a ASC +-- enabled, check that ReadFromMergeTree sorting description is overwritten by DISTINCT optimization i.e. it contains columns 'a,b' +Sorting (Stream): a ASC, b ASC +Sorting (Stream): a ASC, b ASC +Sorting (Stream): a ASC, b ASC +-- enabled, check that ReadFromMergeTree sorting description is NOT overwritten by DISTINCT optimization i.e. it contains columns from ORDER BY clause +Sorting (Stream): a ASC, b ASC +Sorting (Stream): a ASC, b ASC +Sorting (Stream): a ASC, b ASC diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh index f1a741ff783..914fbe2060c 100755 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh @@ -38,6 +38,9 @@ $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, $CLICKHOUSE_CLIENT -q "select '-- distinct with primary key prefix and order by the same columns -> pre-distinct and final distinct optimization'" $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain order by a, b" | eval $FIND_DISTINCT +$CLICKHOUSE_CLIENT -q "select '-- distinct with primary key prefix and order by columns are prefix of distinct columns -> pre-distinct and final distinct optimization'" +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b from distinct_in_order_explain order by a" | eval $FIND_DISTINCT + $CLICKHOUSE_CLIENT -q "select '-- distinct with primary key prefix and order by column in distinct but non-primary key prefix -> pre-distinct and final distinct optimization'" $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, b, c from distinct_in_order_explain order by c" | eval $FIND_DISTINCT @@ -73,7 +76,13 @@ $CLICKHOUSE_CLIENT --read_in_order_two_level_merge_threshold=2 -nq "$ENABLE_OPTI echo "-- enabled, only part of distinct columns form prefix of sorting key" $CLICKHOUSE_CLIENT --max_threads=0 -nq "$ENABLE_OPTIMIZATION;explain pipeline select distinct a, c from distinct_in_order_explain" | eval $FIND_READING_DEFAULT -echo "-- check that sorting properties are propagated from ReadFromMergeTree till preliminary distinct" +echo "-- enabled, check that sorting properties are propagated from ReadFromMergeTree till preliminary distinct" $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain plan sorting=1 select distinct b, a from distinct_in_order_explain where a > 0" | eval $FIND_SORTING_PROPERTIES +echo "-- disabled, check that sorting description for ReadFromMergeTree match ORDER BY columns" +$CLICKHOUSE_CLIENT -nq "$DISABLE_OPTIMIZATION;explain plan sorting=1 select distinct b, a from distinct_in_order_explain order by a" | eval $FIND_SORTING_PROPERTIES +echo "-- enabled, check that ReadFromMergeTree sorting description is overwritten by DISTINCT optimization i.e. it contains columns from DISTINCT clause" +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain plan sorting=1 select distinct b, a from distinct_in_order_explain order by a" | eval $FIND_SORTING_PROPERTIES +echo "-- enabled, check that ReadFromMergeTree sorting description is NOT overwritten by DISTINCT optimization i.e. it contains columns from ORDER BY clause" +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain plan sorting=1 select distinct a from distinct_in_order_explain order by a, b" | eval $FIND_SORTING_PROPERTIES $CLICKHOUSE_CLIENT -q "drop table if exists distinct_in_order_explain sync" From 5d6326ee58f4285a0deb5a01b51cf7ef49630cb1 Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Mon, 19 Sep 2022 22:00:37 +0000 Subject: [PATCH 22/54] Fix test --- .../02317_distinct_in_order_optimization_explain.reference | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference index 881856edf0f..7b8d0ec89a9 100644 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference @@ -60,7 +60,7 @@ Sorting (Stream): a ASC, b ASC Sorting (Stream): a ASC Sorting (Stream): a ASC Sorting (Stream): a ASC --- enabled, check that ReadFromMergeTree sorting description is overwritten by DISTINCT optimization i.e. it contains columns 'a,b' +-- enabled, check that ReadFromMergeTree sorting description is overwritten by DISTINCT optimization i.e. it contains columns from DISTINCT clause Sorting (Stream): a ASC, b ASC Sorting (Stream): a ASC, b ASC Sorting (Stream): a ASC, b ASC From 88b60f861f202df3637f47965708b4689d244db3 Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Tue, 20 Sep 2022 11:27:37 +0000 Subject: [PATCH 23/54] Do not overwrite direction by distinct in order optimization + tests --- .../Optimizations/distinctReadInOrder.cpp | 3 ++- ...stinct_in_order_optimization_explain.reference | 11 ++++++++++- ...2317_distinct_in_order_optimization_explain.sh | 15 +++++++++++---- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp index bb566babe72..7887a54ea9c 100644 --- a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp +++ b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp @@ -79,7 +79,8 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) return 0; /// update input order info in read_from_merge_tree step - const int direction = 1; /// default direction, ASC + const int direction = output_sort_desc.front().direction; /// for DISTINCT direction doesn't matter, so use current direction + /// example: SELECT DISTINCT a from t ORDER BY a DESC; InputOrderInfoPtr order_info = std::make_shared(SortDescription{}, number_of_sorted_distinct_columns, direction, pre_distinct->getLimitHint()); read_from_merge_tree->setQueryInfoInputOrderInfo(order_info); diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference index 7b8d0ec89a9..b41b47200ca 100644 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference @@ -56,6 +56,7 @@ Sorting (Stream): a ASC, b ASC Sorting (Stream): a ASC, b ASC Sorting (Stream): a ASC, b ASC Sorting (Stream): a ASC, b ASC +-- check that reading in order optimization for ORDER BY and DISTINCT applied correctly in the same query -- disabled, check that sorting description for ReadFromMergeTree match ORDER BY columns Sorting (Stream): a ASC Sorting (Stream): a ASC @@ -64,7 +65,15 @@ Sorting (Stream): a ASC Sorting (Stream): a ASC, b ASC Sorting (Stream): a ASC, b ASC Sorting (Stream): a ASC, b ASC --- enabled, check that ReadFromMergeTree sorting description is NOT overwritten by DISTINCT optimization i.e. it contains columns from ORDER BY clause +-- enabled, check that ReadFromMergeTree sorting description is overwritten by DISTINCT optimization, but direction used from ORDER BY clause +Sorting (Stream): a DESC, b DESC +Sorting (Stream): a DESC, b DESC +Sorting (Stream): a DESC, b DESC +-- enabled, check that ReadFromMergeTree sorting description is NOT overwritten by DISTINCT optimization (1), - it contains columns from ORDER BY clause Sorting (Stream): a ASC, b ASC Sorting (Stream): a ASC, b ASC Sorting (Stream): a ASC, b ASC +-- enabled, check that ReadFromMergeTree sorting description is NOT overwritten by DISTINCT optimization (2), - direction used from ORDER BY clause +Sorting (Stream): a DESC, b DESC +Sorting (Stream): a DESC, b DESC +Sorting (Stream): a DESC, b DESC diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh index 914fbe2060c..f23843dc3e5 100755 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh @@ -78,11 +78,18 @@ $CLICKHOUSE_CLIENT --max_threads=0 -nq "$ENABLE_OPTIMIZATION;explain pipeline se echo "-- enabled, check that sorting properties are propagated from ReadFromMergeTree till preliminary distinct" $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain plan sorting=1 select distinct b, a from distinct_in_order_explain where a > 0" | eval $FIND_SORTING_PROPERTIES + +echo "-- check that reading in order optimization for ORDER BY and DISTINCT applied correctly in the same query" +ENABLE_READ_IN_ORDER="set optimize_read_in_order=1" echo "-- disabled, check that sorting description for ReadFromMergeTree match ORDER BY columns" -$CLICKHOUSE_CLIENT -nq "$DISABLE_OPTIMIZATION;explain plan sorting=1 select distinct b, a from distinct_in_order_explain order by a" | eval $FIND_SORTING_PROPERTIES +$CLICKHOUSE_CLIENT -nq "$DISABLE_OPTIMIZATION;$ENABLE_READ_IN_ORDER;explain plan sorting=1 select distinct b, a from distinct_in_order_explain order by a" | eval $FIND_SORTING_PROPERTIES echo "-- enabled, check that ReadFromMergeTree sorting description is overwritten by DISTINCT optimization i.e. it contains columns from DISTINCT clause" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain plan sorting=1 select distinct b, a from distinct_in_order_explain order by a" | eval $FIND_SORTING_PROPERTIES -echo "-- enabled, check that ReadFromMergeTree sorting description is NOT overwritten by DISTINCT optimization i.e. it contains columns from ORDER BY clause" -$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;explain plan sorting=1 select distinct a from distinct_in_order_explain order by a, b" | eval $FIND_SORTING_PROPERTIES +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;$ENABLE_READ_IN_ORDER;explain plan sorting=1 select distinct b, a from distinct_in_order_explain order by a" | eval $FIND_SORTING_PROPERTIES +echo "-- enabled, check that ReadFromMergeTree sorting description is overwritten by DISTINCT optimization, but direction used from ORDER BY clause" +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;$ENABLE_READ_IN_ORDER;explain plan sorting=1 select distinct b, a from distinct_in_order_explain order by a DESC" | eval $FIND_SORTING_PROPERTIES +echo "-- enabled, check that ReadFromMergeTree sorting description is NOT overwritten by DISTINCT optimization (1), - it contains columns from ORDER BY clause" +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;$ENABLE_READ_IN_ORDER;explain plan sorting=1 select distinct a from distinct_in_order_explain order by a, b" | eval $FIND_SORTING_PROPERTIES +echo "-- enabled, check that ReadFromMergeTree sorting description is NOT overwritten by DISTINCT optimization (2), - direction used from ORDER BY clause" +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;$ENABLE_READ_IN_ORDER;explain plan sorting=1 select distinct b, a from distinct_in_order_explain order by a DESC, b DESC" | eval $FIND_SORTING_PROPERTIES $CLICKHOUSE_CLIENT -q "drop table if exists distinct_in_order_explain sync" From 971cef8bd214d4cbe521d0e318d750307ff0cac8 Mon Sep 17 00:00:00 2001 From: Robert Schulze Date: Wed, 21 Sep 2022 08:39:32 +0000 Subject: [PATCH 24/54] Provide x86 SIMD flag options only on x86 --- cmake/cpu_features.cmake | 81 +++++++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 31 deletions(-) diff --git a/cmake/cpu_features.cmake b/cmake/cpu_features.cmake index 218b4deedce..9c986bff7c3 100644 --- a/cmake/cpu_features.cmake +++ b/cmake/cpu_features.cmake @@ -11,49 +11,68 @@ cmake_push_check_state () # All of them are unrelated to the instruction set at the host machine # (you can compile for newer instruction set on old machines and vice versa). -option (ENABLE_SSSE3 "Use SSSE3 instructions on x86_64" 1) -option (ENABLE_SSE41 "Use SSE4.1 instructions on x86_64" 1) -option (ENABLE_SSE42 "Use SSE4.2 instructions on x86_64" 1) -option (ENABLE_PCLMULQDQ "Use pclmulqdq instructions on x86_64" 1) -option (ENABLE_POPCNT "Use popcnt instructions on x86_64" 1) -option (ENABLE_AVX "Use AVX instructions on x86_64" 0) -option (ENABLE_AVX2 "Use AVX2 instructions on x86_64" 0) -option (ENABLE_AVX512 "Use AVX512 instructions on x86_64" 0) -option (ENABLE_AVX512_VBMI "Use AVX512_VBMI instruction on x86_64 (depends on ENABLE_AVX512)" 0) -option (ENABLE_BMI "Use BMI instructions on x86_64" 0) -option (ENABLE_AVX2_FOR_SPEC_OP "Use avx2 instructions for specific operations on x86_64" 0) -option (ENABLE_AVX512_FOR_SPEC_OP "Use avx512 instructions for specific operations on x86_64" 0) - -# X86: Allow compilation for a SSE2-only target machine. Done by a special build in CI for embedded or very old hardware. -option (NO_SSE3_OR_HIGHER "Disable SSE3 or higher on x86_64" 0) -if (NO_SSE3_OR_HIGHER) - SET(ENABLE_SSSE3 0) - SET(ENABLE_SSE41 0) - SET(ENABLE_SSE42 0) - SET(ENABLE_PCLMULQDQ 0) - SET(ENABLE_POPCNT 0) - SET(ENABLE_AVX 0) - SET(ENABLE_AVX2 0) - SET(ENABLE_AVX512 0) - SET(ENABLE_AVX512_VBMI 0) - SET(ENABLE_BMI 0) - SET(ENABLE_AVX2_FOR_SPEC_OP 0) - SET(ENABLE_AVX512_FOR_SPEC_OP 0) -endif() - option (ARCH_NATIVE "Add -march=native compiler flag. This makes your binaries non-portable but more performant code may be generated. This option overrides ENABLE_* options for specific instruction set. Highly not recommended to use." 0) if (ARCH_NATIVE) set (COMPILER_FLAGS "${COMPILER_FLAGS} -march=native") elseif (ARCH_AARCH64) - set (COMPILER_FLAGS "${COMPILER_FLAGS} -march=armv8-a+crc+simd+crypto+dotprod+ssbs") + # ARM publishes almost every year a new revision of it's ISA [1]. Each revision comes with new mandatory and optional features from + # which CPU vendors can pick and choose. This creates a lot of variability ... We provide two build "profiles", one for maximum + # compatibility intended to run on all 64-bit ARM hardware released after 2013 (e.g. Raspberry Pi 4), and one for modern ARM server + # CPUs, (e.g. Graviton). + # + # [1] https://en.wikipedia.org/wiki/AArch64 + option (NO_ARMV81_OR_HIGHER "Disable ARMv8.1 or higher on Aarch64 for maximum compatibility with older/embedded hardware." 0) + + if (NO_ARMV81_OR_HIGHER) + set (COMPILER_FLAGS "${COMPILER_FLAGS} -march=armv8") + else () + # ARMv8.2 is ancient but the baseline for Graviton 2 and 3 processors [1]. In particular, it includes LSE (first made mandatory with + # ARMv8.1) which provides nice speedups without having to fall back to v8.0 "-moutline-atomics" compat flag [2, 3, 4] that imposes + # a recent glibc with runtime dispatch helper, limiting our ability to run on old OSs. + # + # [1] https://github.com/aws/aws-graviton-getting-started/blob/main/c-c%2B%2B.md + # [2] https://community.arm.com/arm-community-blogs/b/tools-software-ides-blog/posts/making-the-most-of-the-arm-architecture-in-gcc-10 + # [3] https://mysqlonarm.github.io/ARM-LSE-and-MySQL/ + # [4] https://dev.to/aws-builders/large-system-extensions-for-aws-graviton-processors-3eci + set (COMPILER_FLAGS "${COMPILER_FLAGS} -march=armv8.2-a+crc+simd+crypto+dotprod+ssbs") + endif () elseif (ARCH_PPC64LE) # Note that gcc and clang have support for x86 SSE2 intrinsics when building for PowerPC set (COMPILER_FLAGS "${COMPILER_FLAGS} -maltivec -mcpu=power8 -D__SSE2__=1 -DNO_WARN_X86_INTRINSICS") elseif (ARCH_AMD64) + option (ENABLE_SSSE3 "Use SSSE3 instructions on x86_64" 1) + option (ENABLE_SSE41 "Use SSE4.1 instructions on x86_64" 1) + option (ENABLE_SSE42 "Use SSE4.2 instructions on x86_64" 1) + option (ENABLE_PCLMULQDQ "Use pclmulqdq instructions on x86_64" 1) + option (ENABLE_POPCNT "Use popcnt instructions on x86_64" 1) + option (ENABLE_AVX "Use AVX instructions on x86_64" 0) + option (ENABLE_AVX2 "Use AVX2 instructions on x86_64" 0) + option (ENABLE_AVX512 "Use AVX512 instructions on x86_64" 0) + option (ENABLE_AVX512_VBMI "Use AVX512_VBMI instruction on x86_64 (depends on ENABLE_AVX512)" 0) + option (ENABLE_BMI "Use BMI instructions on x86_64" 0) + option (ENABLE_AVX2_FOR_SPEC_OP "Use avx2 instructions for specific operations on x86_64" 0) + option (ENABLE_AVX512_FOR_SPEC_OP "Use avx512 instructions for specific operations on x86_64" 0) + + option (NO_SSE3_OR_HIGHER "Disable SSE3 or higher on x86_64 for maximum compatibility with older/embedded hardware." 0) + if (NO_SSE3_OR_HIGHER) + SET(ENABLE_SSSE3 0) + SET(ENABLE_SSE41 0) + SET(ENABLE_SSE42 0) + SET(ENABLE_PCLMULQDQ 0) + SET(ENABLE_POPCNT 0) + SET(ENABLE_AVX 0) + SET(ENABLE_AVX2 0) + SET(ENABLE_AVX512 0) + SET(ENABLE_AVX512_VBMI 0) + SET(ENABLE_BMI 0) + SET(ENABLE_AVX2_FOR_SPEC_OP 0) + SET(ENABLE_AVX512_FOR_SPEC_OP 0) + endif() + set (TEST_FLAG "-mssse3") set (CMAKE_REQUIRED_FLAGS "${TEST_FLAG} -O0") check_cxx_source_compiles(" From bcaa66c80460bc32409c66acb56df7104c2d5c00 Mon Sep 17 00:00:00 2001 From: Robert Schulze Date: Wed, 21 Sep 2022 13:09:24 +0000 Subject: [PATCH 25/54] Add arm-v80compat builds --- .github/workflows/master.yml | 46 ++++++++++++++++++++++++++++++ .github/workflows/pull_request.yml | 44 ++++++++++++++++++++++++++++ docker/packager/packager | 10 +++++++ tests/ci/ci_config.py | 10 +++++++ 4 files changed, 110 insertions(+) diff --git a/.github/workflows/master.yml b/.github/workflows/master.yml index 6f2fd5d678d..3d22cb984dd 100644 --- a/.github/workflows/master.yml +++ b/.github/workflows/master.yml @@ -887,6 +887,51 @@ jobs: docker ps --quiet | xargs --no-run-if-empty docker kill ||: docker ps --all --quiet | xargs --no-run-if-empty docker rm -f ||: sudo rm -fr "$TEMP_PATH" "$CACHES_PATH" + BuilderBinAarch64V80Compat: + needs: [DockerHubPush] + runs-on: [self-hosted, builder] + steps: + - name: Set envs + run: | + cat >> "$GITHUB_ENV" << 'EOF' + TEMP_PATH=${{runner.temp}}/build_check + IMAGES_PATH=${{runner.temp}}/images_path + REPO_COPY=${{runner.temp}}/build_check/ClickHouse + CACHES_PATH=${{runner.temp}}/../ccaches + BUILD_NAME=binary_aarch64_v80compat + EOF + - name: Download changed images + uses: actions/download-artifact@v2 + with: + name: changed_images + path: ${{ env.IMAGES_PATH }} + - name: Clear repository + run: | + sudo rm -fr "$GITHUB_WORKSPACE" && mkdir "$GITHUB_WORKSPACE" + - name: Check out repository code + uses: actions/checkout@v2 + with: + fetch-depth: 0 # otherwise we will have no info about contributors + - name: Build + run: | + git -C "$GITHUB_WORKSPACE" submodule sync --recursive + git -C "$GITHUB_WORKSPACE" submodule update --depth=1 --recursive --init --jobs=10 + sudo rm -fr "$TEMP_PATH" + mkdir -p "$TEMP_PATH" + cp -r "$GITHUB_WORKSPACE" "$TEMP_PATH" + cd "$REPO_COPY/tests/ci" && python3 build_check.py "$BUILD_NAME" + - name: Upload build URLs to artifacts + if: ${{ success() || failure() }} + uses: actions/upload-artifact@v2 + with: + name: ${{ env.BUILD_URLS }} + path: ${{ env.TEMP_PATH }}/${{ env.BUILD_URLS }}.json + - name: Cleanup + if: always() + run: | + docker ps --quiet | xargs --no-run-if-empty docker kill ||: + docker ps --all --quiet | xargs --no-run-if-empty docker rm -f ||: + sudo rm -fr "$TEMP_PATH" "$CACHES_PATH" ############################################################################################ ##################################### Docker images ####################################### ############################################################################################ @@ -972,6 +1017,7 @@ jobs: # - BuilderBinGCC - BuilderBinPPC64 - BuilderBinAmd64SSE2 + - BuilderBinAarch64V80Compat - BuilderBinClangTidy - BuilderDebShared runs-on: [self-hosted, style-checker] diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index 24a1c6bb714..2795dc62d6d 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -940,6 +940,49 @@ jobs: docker ps --quiet | xargs --no-run-if-empty docker kill ||: docker ps --all --quiet | xargs --no-run-if-empty docker rm -f ||: sudo rm -fr "$TEMP_PATH" "$CACHES_PATH" + BuilderBinAarch64V80Compat: + needs: [DockerHubPush, FastTest, StyleCheck] + runs-on: [self-hosted, builder] + steps: + - name: Set envs + run: | + cat >> "$GITHUB_ENV" << 'EOF' + TEMP_PATH=${{runner.temp}}/build_check + IMAGES_PATH=${{runner.temp}}/images_path + REPO_COPY=${{runner.temp}}/build_check/ClickHouse + CACHES_PATH=${{runner.temp}}/../ccaches + BUILD_NAME=binary_aarch64_v80compat + EOF + - name: Download changed images + uses: actions/download-artifact@v2 + with: + name: changed_images + path: ${{ env.IMAGES_PATH }} + - name: Clear repository + run: | + sudo rm -fr "$GITHUB_WORKSPACE" && mkdir "$GITHUB_WORKSPACE" + - name: Check out repository code + uses: actions/checkout@v2 + - name: Build + run: | + git -C "$GITHUB_WORKSPACE" submodule sync --recursive + git -C "$GITHUB_WORKSPACE" submodule update --depth=1 --recursive --init --jobs=10 + sudo rm -fr "$TEMP_PATH" + mkdir -p "$TEMP_PATH" + cp -r "$GITHUB_WORKSPACE" "$TEMP_PATH" + cd "$REPO_COPY/tests/ci" && python3 build_check.py "$BUILD_NAME" + - name: Upload build URLs to artifacts + if: ${{ success() || failure() }} + uses: actions/upload-artifact@v2 + with: + name: ${{ env.BUILD_URLS }} + path: ${{ env.TEMP_PATH }}/${{ env.BUILD_URLS }}.json + - name: Cleanup + if: always() + run: | + docker ps --quiet | xargs --no-run-if-empty docker kill ||: + docker ps --all --quiet | xargs --no-run-if-empty docker rm -f ||: + sudo rm -fr "$TEMP_PATH" "$CACHES_PATH" ############################################################################################ ##################################### Docker images ####################################### ############################################################################################ @@ -1025,6 +1068,7 @@ jobs: # - BuilderBinGCC - BuilderBinPPC64 - BuilderBinAmd64SSE2 + - BuilderBinAarch64V80Compat - BuilderBinClangTidy - BuilderDebShared runs-on: [self-hosted, style-checker] diff --git a/docker/packager/packager b/docker/packager/packager index 9da787e9006..b4aa4ebdd91 100755 --- a/docker/packager/packager +++ b/docker/packager/packager @@ -128,6 +128,7 @@ def parse_env_variables( DARWIN_SUFFIX = "-darwin" DARWIN_ARM_SUFFIX = "-darwin-aarch64" ARM_SUFFIX = "-aarch64" + ARM_V80COMPAT_SUFFIX = "-aarch64-v80compat" FREEBSD_SUFFIX = "-freebsd" PPC_SUFFIX = "-ppc64le" AMD64_SSE2_SUFFIX = "-amd64sse2" @@ -140,6 +141,7 @@ def parse_env_variables( is_cross_darwin = compiler.endswith(DARWIN_SUFFIX) is_cross_darwin_arm = compiler.endswith(DARWIN_ARM_SUFFIX) is_cross_arm = compiler.endswith(ARM_SUFFIX) + is_cross_arm_v80compat = compiler.endswith(ARM_V80COMPAT_SUFFIX) is_cross_ppc = compiler.endswith(PPC_SUFFIX) is_cross_freebsd = compiler.endswith(FREEBSD_SUFFIX) is_amd64_sse2 = compiler.endswith(AMD64_SSE2_SUFFIX) @@ -178,6 +180,13 @@ def parse_env_variables( "-DCMAKE_TOOLCHAIN_FILE=/build/cmake/linux/toolchain-aarch64.cmake" ) result.append("DEB_ARCH=arm64") + elif is_cross_arm_v80compat: + cc = compiler[: -len(ARM_V80COMPAT_SUFFIX)] + cmake_flags.append( + "-DCMAKE_TOOLCHAIN_FILE=/build/cmake/linux/toolchain-aarch64.cmake" + ) + cmake_flags.append("-DNO_ARMV81_OR_HIGHER=1") + result.append("DEB_ARCH=arm64") elif is_cross_freebsd: cc = compiler[: -len(FREEBSD_SUFFIX)] cmake_flags.append( @@ -343,6 +352,7 @@ if __name__ == "__main__": "clang-15-darwin", "clang-15-darwin-aarch64", "clang-15-aarch64", + "clang-15-aarch64-v80compat", "clang-15-ppc64le", "clang-15-amd64sse2", "clang-15-freebsd", diff --git a/tests/ci/ci_config.py b/tests/ci/ci_config.py index a31f2298a58..19513491b1e 100644 --- a/tests/ci/ci_config.py +++ b/tests/ci/ci_config.py @@ -131,6 +131,15 @@ CI_CONFIG = { "tidy": "disable", "with_coverage": False, }, + "binary_aarch64_v80compat": { + "compiler": "clang-15-aarch64-v80compat", + "build_type": "", + "sanitizer": "", + "package_type": "binary", + "libraries": "static", + "tidy": "disable", + "with_coverage": False, + }, "binary_freebsd": { "compiler": "clang-15-freebsd", "build_type": "", @@ -189,6 +198,7 @@ CI_CONFIG = { "binary_shared", "binary_darwin", "binary_aarch64", + "binary_aarch64_v80compat", "binary_freebsd", "binary_darwin_aarch64", "binary_ppc64le", From cfd8d4e1f12bef0103387008b9487e8237e27510 Mon Sep 17 00:00:00 2001 From: Robert Schulze Date: Wed, 21 Sep 2022 14:49:38 +0000 Subject: [PATCH 26/54] Add CRC32 to compat build --- cmake/cpu_features.cmake | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cmake/cpu_features.cmake b/cmake/cpu_features.cmake index 9c986bff7c3..2660244055e 100644 --- a/cmake/cpu_features.cmake +++ b/cmake/cpu_features.cmake @@ -26,7 +26,9 @@ elseif (ARCH_AARCH64) option (NO_ARMV81_OR_HIGHER "Disable ARMv8.1 or higher on Aarch64 for maximum compatibility with older/embedded hardware." 0) if (NO_ARMV81_OR_HIGHER) - set (COMPILER_FLAGS "${COMPILER_FLAGS} -march=armv8") + # In v8.0, crc32 is optional, in v8.1 it's mandatory. Enable it regardless as __crc32()* is used in lot's of places and even very + # old ARM CPUs support it. + set (COMPILER_FLAGS "${COMPILER_FLAGS} -march=armv8+crc") else () # ARMv8.2 is ancient but the baseline for Graviton 2 and 3 processors [1]. In particular, it includes LSE (first made mandatory with # ARMv8.1) which provides nice speedups without having to fall back to v8.0 "-moutline-atomics" compat flag [2, 3, 4] that imposes From 9c433c14d1329cc042a83d9816afa420b065ddcc Mon Sep 17 00:00:00 2001 From: alesapin Date: Wed, 21 Sep 2022 21:33:28 +0200 Subject: [PATCH 27/54] Add very explicit logging on disk choice for fetch --- src/Storages/MergeTree/DataPartsExchange.cpp | 40 ++++++++++++++++++-- src/Storages/MergeTree/DataPartsExchange.h | 2 +- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/src/Storages/MergeTree/DataPartsExchange.cpp b/src/Storages/MergeTree/DataPartsExchange.cpp index bf803425a86..48f5a1f330a 100644 --- a/src/Storages/MergeTree/DataPartsExchange.cpp +++ b/src/Storages/MergeTree/DataPartsExchange.cpp @@ -100,8 +100,10 @@ struct ReplicatedFetchReadCallback } -Service::Service(StorageReplicatedMergeTree & data_) : - data(data_), log(&Poco::Logger::get(data.getLogName() + " (Replicated PartsService)")) {} +Service::Service(StorageReplicatedMergeTree & data_) + : data(data_) + , log(&Poco::Logger::get(data.getStorageID().getNameForLogs() + " (Replicated PartsService)")) +{} std::string Service::getId(const std::string & node_id) const { @@ -402,6 +404,11 @@ MergeTreeData::DataPartPtr Service::findPart(const String & name) throw Exception(ErrorCodes::NO_SUCH_DATA_PART, "No part {} in table", name); } +Fetcher::Fetcher(StorageReplicatedMergeTree & data_) + : data(data_) + , log(&Poco::Logger::get(data.getStorageID().getNameForLogs() + " (Fetcher)")) +{} + MergeTreeData::MutableDataPartPtr Fetcher::fetchSelectedPart( const StorageMetadataPtr & metadata_snapshot, ContextPtr context, @@ -452,6 +459,7 @@ MergeTreeData::MutableDataPartPtr Fetcher::fetchSelectedPart( if (disk) { + LOG_TRACE(log, "Will fetch to disk {} with type {}", disk->getName(), toString(disk->getDataSourceDescription().type)); UInt64 revision = disk->getRevision(); if (revision) uri.addQueryParameter("disk_revision", toString(revision)); @@ -462,13 +470,21 @@ MergeTreeData::MutableDataPartPtr Fetcher::fetchSelectedPart( { if (!disk) { + LOG_TRACE(log, "Trying to fetch with zero-copy replication, but disk is not provided, will try to select"); Disks disks = data.getDisks(); for (const auto & data_disk : disks) + { + LOG_TRACE(log, "Checking disk {} with type {}", data_disk->getName(), toString(data_disk->getDataSourceDescription().type)); if (data_disk->supportZeroCopyReplication()) + { + LOG_TRACE(log, "Disk {} (with type {}) supports zero-copy replication", data_disk->getName(), toString(data_disk->getDataSourceDescription().type)); capability.push_back(toString(data_disk->getDataSourceDescription().type)); + } + } } else if (disk->supportZeroCopyReplication()) { + LOG_TRACE(log, "Trying to fetch with zero copy replication, provided disk {} with type {}", disk->getName(), toString(disk->getDataSourceDescription().type)); capability.push_back(toString(disk->getDataSourceDescription().type)); } } @@ -520,29 +536,47 @@ MergeTreeData::MutableDataPartPtr Fetcher::fetchSelectedPart( ReadBufferFromString ttl_infos_buffer(ttl_infos_string); assertString("ttl format version: 1\n", ttl_infos_buffer); ttl_infos.read(ttl_infos_buffer); + if (!disk) { + LOG_TRACE(log, "Disk for fetch is not provided, reserving space using storage balanced reservation"); reservation = data.balancedReservation(metadata_snapshot, sum_files_size, 0, part_name, part_info, {}, tagger_ptr, &ttl_infos, true); if (!reservation) + { + LOG_TRACE(log, "Disk for fetch is not provided, reserving space using TTL rules"); reservation = data.reserveSpacePreferringTTLRules(metadata_snapshot, sum_files_size, ttl_infos, std::time(nullptr), 0, true); + } } } else if (!disk) { + LOG_TRACE(log, "Making balanced reservation"); reservation = data.balancedReservation(metadata_snapshot, sum_files_size, 0, part_name, part_info, {}, tagger_ptr, nullptr); if (!reservation) + { + LOG_TRACE(log, "Making simple reservation"); reservation = data.reserveSpace(sum_files_size); + } } } else if (!disk) { + LOG_TRACE(log, "Making reservation on the largest disk"); /// We don't know real size of part because sender server version is too old reservation = data.makeEmptyReservationOnLargestDisk(); } + if (!disk) + { disk = reservation->getDisk(); + LOG_INFO(log, "Disk for fetch is not provided, getting disk from reservation {} with type {}", disk->getName(), toString(disk->getDataSourceDescription().type)); + } + else + { + LOG_INFO(log, "Disk for fetch is disk {} with type {}", disk->getName(), toString(disk->getDataSourceDescription().type)); + } UInt64 revision = parse(in->getResponseCookie("disk_revision", "0")); if (revision) @@ -887,7 +921,7 @@ MergeTreeData::MutableDataPartPtr Fetcher::downloadPartToDiskRemoteMeta( if (!disk->supportZeroCopyReplication() || !disk->checkUniqueId(part_id)) { - throw Exception(ErrorCodes::ZERO_COPY_REPLICATION_ERROR, "Part {} unique id {} doesn't exist on {}.", part_name, part_id, disk->getName()); + throw Exception(ErrorCodes::ZERO_COPY_REPLICATION_ERROR, "Part {} unique id {} doesn't exist on {} (with type {}).", part_name, part_id, disk->getName(), toString(disk->getDataSourceDescription().type)); } LOG_DEBUG(log, "Downloading Part {} unique id {} metadata onto disk {}.", diff --git a/src/Storages/MergeTree/DataPartsExchange.h b/src/Storages/MergeTree/DataPartsExchange.h index e2582c42dfb..17bd73e2482 100644 --- a/src/Storages/MergeTree/DataPartsExchange.h +++ b/src/Storages/MergeTree/DataPartsExchange.h @@ -63,7 +63,7 @@ private: class Fetcher final : private boost::noncopyable { public: - explicit Fetcher(StorageReplicatedMergeTree & data_) : data(data_), log(&Poco::Logger::get("Fetcher")) {} + explicit Fetcher(StorageReplicatedMergeTree & data_); /// Downloads a part to tmp_directory. If to_detached - downloads to the `detached` directory. MergeTreeData::MutableDataPartPtr fetchSelectedPart( From 49c4f1f9c6821d4c4f273c69a233b750d9699d3d Mon Sep 17 00:00:00 2001 From: Robert Schulze Date: Wed, 21 Sep 2022 21:02:48 +0000 Subject: [PATCH 28/54] Document flags --- cmake/cpu_features.cmake | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/cmake/cpu_features.cmake b/cmake/cpu_features.cmake index 2660244055e..f9b2f103f49 100644 --- a/cmake/cpu_features.cmake +++ b/cmake/cpu_features.cmake @@ -17,7 +17,7 @@ if (ARCH_NATIVE) set (COMPILER_FLAGS "${COMPILER_FLAGS} -march=native") elseif (ARCH_AARCH64) - # ARM publishes almost every year a new revision of it's ISA [1]. Each revision comes with new mandatory and optional features from + # ARM publishes almost every year a new revision of it's ISA [1]. Each version comes with new mandatory and optional features from # which CPU vendors can pick and choose. This creates a lot of variability ... We provide two build "profiles", one for maximum # compatibility intended to run on all 64-bit ARM hardware released after 2013 (e.g. Raspberry Pi 4), and one for modern ARM server # CPUs, (e.g. Graviton). @@ -26,19 +26,35 @@ elseif (ARCH_AARCH64) option (NO_ARMV81_OR_HIGHER "Disable ARMv8.1 or higher on Aarch64 for maximum compatibility with older/embedded hardware." 0) if (NO_ARMV81_OR_HIGHER) - # In v8.0, crc32 is optional, in v8.1 it's mandatory. Enable it regardless as __crc32()* is used in lot's of places and even very - # old ARM CPUs support it. + # crc32 is optional in v8.0 and mandatory in v8.1. Enable it as __crc32()* is used in lot's of places and even very old ARM CPUs + # support it. set (COMPILER_FLAGS "${COMPILER_FLAGS} -march=armv8+crc") else () - # ARMv8.2 is ancient but the baseline for Graviton 2 and 3 processors [1]. In particular, it includes LSE (first made mandatory with - # ARMv8.1) which provides nice speedups without having to fall back to v8.0 "-moutline-atomics" compat flag [2, 3, 4] that imposes - # a recent glibc with runtime dispatch helper, limiting our ability to run on old OSs. + # ARMv8.2 is quite ancient but the lowest common denominator supported by both Graviton 2 and 3 processors [1]. In particular, it + # includes LSE (made mandatory with ARMv8.1) which provides nice speedups without having to fall back to compat flag + # "-moutline-atomics" for v8.0 [2, 3, 4] that requires a recent glibc with runtime dispatch helper, limiting our ability to run on + # old OSs. + # + # simd: NEON, introduced as optional in v8.0, A few extensions were added with v8.1 but it's still not mandatory. Enables the + # compiler to auto-vectorize. + # sve: Scalable Vector Extensions, introduced as optional in v8.2. Available in Graviton 3 but not in Graviton 2, and most likely + # also not in CI machines. Compiler support for autovectorization is rudimentary at the time of writing, see [5]. Can be + # enabled one-fine-day (TM) but not now. + # ssbs: "Speculative Store Bypass Safe". Optional in v8.0, mandatory in v8.5. Meltdown/spectre countermeasure. + # crypto: SHA1, SHA256, AES. Optional in v8.0. In v8.4, further algorithms were added but it's still optional, see [6]. + # dotprod: Scalar vector product (SDOT and UDOT instructions). Probably the most obscure extra flag with doubtful performance benefits + # but it has been activated since always, so why not enable it. It's not 100% clear in which revision this flag was + # introduced as optional, either in v8.2 [7] or in v8.4 [8]. # # [1] https://github.com/aws/aws-graviton-getting-started/blob/main/c-c%2B%2B.md # [2] https://community.arm.com/arm-community-blogs/b/tools-software-ides-blog/posts/making-the-most-of-the-arm-architecture-in-gcc-10 # [3] https://mysqlonarm.github.io/ARM-LSE-and-MySQL/ # [4] https://dev.to/aws-builders/large-system-extensions-for-aws-graviton-processors-3eci - set (COMPILER_FLAGS "${COMPILER_FLAGS} -march=armv8.2-a+crc+simd+crypto+dotprod+ssbs") + # [5] https://developer.arm.com/tools-and-software/open-source-software/developer-tools/llvm-toolchain/sve-support + # [6] https://developer.arm.com/documentation/100067/0612/armclang-Command-line-Options/-mcpu?lang=en + # [7] https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html + # [8] https://developer.arm.com/documentation/102651/a/What-are-dot-product-intructions- + set (COMPILER_FLAGS "${COMPILER_FLAGS} -march=armv8.2-a+simd+crypto+dotprod+ssbs") endif () elseif (ARCH_PPC64LE) From 223c1230b6260faba055329f611a5ed03cbf6cee Mon Sep 17 00:00:00 2001 From: Zhiguo Zhou Date: Thu, 22 Sep 2022 15:48:22 +0800 Subject: [PATCH 29/54] Optimize the lock contentions for ThreadGroupStatus::mutex The release of ThreadGroupStatus::finished_threads_counters_memory via the getProfileEventsCountersAndMemoryForThreads method brings lots of lock contentions for ThreadGroupStatus::mutex and lowers the overall performance. This commit optimizes this performance issue by replacing the method call with an equivalent but more lightweight code block. --- src/Interpreters/ThreadStatusExt.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Interpreters/ThreadStatusExt.cpp b/src/Interpreters/ThreadStatusExt.cpp index b1f5749da25..9a4152415af 100644 --- a/src/Interpreters/ThreadStatusExt.cpp +++ b/src/Interpreters/ThreadStatusExt.cpp @@ -350,7 +350,10 @@ void ThreadStatus::detachQuery(bool exit_if_already_detached, bool thread_exits) /// Avoid leaking of ThreadGroupStatus::finished_threads_counters_memory /// (this is in case someone uses system thread but did not call getProfileEventsCountersAndMemoryForThreads()) - thread_group->getProfileEventsCountersAndMemoryForThreads(); + { + std::lock_guard guard(thread_group->mutex); + auto stats = std::move(thread_group->finished_threads_counters_memory); + } thread_group.reset(); From f038c7ea00ce4760d441cdca9cdc7b0945db2907 Mon Sep 17 00:00:00 2001 From: kssenii Date: Thu, 22 Sep 2022 14:49:35 +0200 Subject: [PATCH 30/54] Better config for cache --- docs/en/operations/storing-data.md | 8 ++++---- src/Interpreters/Cache/FileCacheSettings.cpp | 8 ++++++-- tests/config/config.d/storage_conf.xml | 6 +++--- tests/queries/0_stateless/02344_describe_cache.reference | 3 ++- tests/queries/0_stateless/02344_describe_cache.sql | 1 + 5 files changed, 16 insertions(+), 10 deletions(-) diff --git a/docs/en/operations/storing-data.md b/docs/en/operations/storing-data.md index 01d7b6d9195..290c2d7c96c 100644 --- a/docs/en/operations/storing-data.md +++ b/docs/en/operations/storing-data.md @@ -131,7 +131,7 @@ Example of configuration for versions later or equal to 22.8: cache s3 /s3_cache/ - 10000000 + 10Gi @@ -155,7 +155,7 @@ Example of configuration for versions earlier than 22.8: ... ... s3 configuration ... 1 - 10000000 + 10Gi @@ -172,7 +172,7 @@ Cache **configuration settings**: - `path` - path to the directory with cache. Default: None, this setting is obligatory. -- `max_size` - maximum size of the cache in bytes. When the limit is reached, cache files are evicted according to the cache eviction policy. Default: None, this setting is obligatory. +- `max_size` - maximum size of the cache in bytes or in readable format (`ki, Mi, Gi, etc`, example `10Gi`). When the limit is reached, cache files are evicted according to the cache eviction policy. Default: None, this setting is obligatory. - `cache_on_write_operations` - allow to turn on `write-through` cache (caching data on any write operations: `INSERT` queries, background merges). Default: `false`. The `write-through` cache can be disabled per query using setting `enable_filesystem_cache_on_write_operations` (data is cached only if both cache config settings and corresponding query setting are enabled). @@ -182,7 +182,7 @@ Cache **configuration settings**: - `do_not_evict_index_and_mark_files` - do not evict small frequently used files according to cache policy. Default: `false`. This setting was added in version 22.8. If you used filesystem cache before this version, then it will not work on versions starting from 22.8 if this setting is set to `true`. If you want to use this setting, clear old cache created before version 22.8 before upgrading. -- `max_file_segment_size` - a maximum size of a single cache file. Default: `104857600` (100 Mb). +- `max_file_segment_size` - a maximum size of a single cache file in bytes or in readable format (`ki, Mi, Gi, etc`, example `10Gi`). Default: `104857600` (`100Mi`). - `max_elements` - a limit for a number of cache files. Default: `1048576`. diff --git a/src/Interpreters/Cache/FileCacheSettings.cpp b/src/Interpreters/Cache/FileCacheSettings.cpp index 3b193a070c0..4b8d806bb53 100644 --- a/src/Interpreters/Cache/FileCacheSettings.cpp +++ b/src/Interpreters/Cache/FileCacheSettings.cpp @@ -2,6 +2,7 @@ #include #include +#include namespace DB { @@ -16,7 +17,7 @@ void FileCacheSettings::loadFromConfig(const Poco::Util::AbstractConfiguration & if (!config.has(config_prefix + ".max_size")) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Expected cache size (`max_size`) in configuration"); - max_size = config.getUInt64(config_prefix + ".max_size", 0); + max_size = parseWithSizeSuffix(config.getString(config_prefix + ".max_size")); if (max_size == 0) throw Exception(ErrorCodes::BAD_ARGUMENTS, "Expected non-zero size for cache configuration"); @@ -25,7 +26,10 @@ void FileCacheSettings::loadFromConfig(const Poco::Util::AbstractConfiguration & throw Exception(ErrorCodes::BAD_ARGUMENTS, "Disk Cache requires non-empty `path` field (cache base path) in config"); max_elements = config.getUInt64(config_prefix + ".max_elements", REMOTE_FS_OBJECTS_CACHE_DEFAULT_MAX_ELEMENTS); - max_file_segment_size = config.getUInt64(config_prefix + ".max_file_segment_size", REMOTE_FS_OBJECTS_CACHE_DEFAULT_MAX_FILE_SEGMENT_SIZE); + if (config.has(config_prefix + ".max_file_segment_size")) + max_file_segment_size = parseWithSizeSuffix(config.getString(config_prefix + ".max_file_segment_size")); + else + max_file_segment_size = REMOTE_FS_OBJECTS_CACHE_DEFAULT_MAX_FILE_SEGMENT_SIZE; cache_on_write_operations = config.getUInt64(config_prefix + ".cache_on_write_operations", false); enable_filesystem_query_cache_limit = config.getUInt64(config_prefix + ".enable_filesystem_query_cache_limit", false); diff --git a/tests/config/config.d/storage_conf.xml b/tests/config/config.d/storage_conf.xml index de41478f74a..3c24633d413 100644 --- a/tests/config/config.d/storage_conf.xml +++ b/tests/config/config.d/storage_conf.xml @@ -48,8 +48,8 @@ cache s3_disk - s3_disk_cache/ - 22548578304 + s3_cache/ + 2147483648 1 0 @@ -57,7 +57,7 @@ cache s3_disk_2 s3_cache_2/ - 22548578304 + 2Gi 0 diff --git a/tests/queries/0_stateless/02344_describe_cache.reference b/tests/queries/0_stateless/02344_describe_cache.reference index 25cf99dd775..e49e1c6b193 100644 --- a/tests/queries/0_stateless/02344_describe_cache.reference +++ b/tests/queries/0_stateless/02344_describe_cache.reference @@ -1 +1,2 @@ -22548578304 1048576 104857600 1 0 0 0 s3_disk_cache/ 0 +22548578304 1048576 104857600 1 0 0 0 s3_cache/ 0 +22548578304 1048576 104857600 1 0 0 0 s3_cache_2/ 0 diff --git a/tests/queries/0_stateless/02344_describe_cache.sql b/tests/queries/0_stateless/02344_describe_cache.sql index 132ce3e6c40..8b3831bcaa8 100644 --- a/tests/queries/0_stateless/02344_describe_cache.sql +++ b/tests/queries/0_stateless/02344_describe_cache.sql @@ -1,3 +1,4 @@ -- Tags: no-fasttest DESCRIBE FILESYSTEM CACHE 's3_cache'; +DESCRIBE FILESYSTEM CACHE 's3_cache_2'; From 60a5ff91e5a9ddbe577657659e415b1e321f9353 Mon Sep 17 00:00:00 2001 From: avogar Date: Thu, 22 Sep 2022 13:19:45 +0000 Subject: [PATCH 31/54] Add test for setting output_format_json_validate_utf8 --- .../02452_json_utf8_validation.reference | 174 ++++++++++++++++++ .../02452_json_utf8_validation.sql | 42 +++++ 2 files changed, 216 insertions(+) create mode 100644 tests/queries/0_stateless/02452_json_utf8_validation.reference create mode 100644 tests/queries/0_stateless/02452_json_utf8_validation.sql diff --git a/tests/queries/0_stateless/02452_json_utf8_validation.reference b/tests/queries/0_stateless/02452_json_utf8_validation.reference new file mode 100644 index 00000000000..c7155832e1e --- /dev/null +++ b/tests/queries/0_stateless/02452_json_utf8_validation.reference @@ -0,0 +1,174 @@ +JSONCompact +{ + "meta": + [ + { + "name": "s", + "type": "String" + } + ], + + "data": + [ + ["� �"] + ], + + "rows": 1 +} +JSON +{ + "meta": + [ + { + "name": "s", + "type": "String" + } + ], + + "data": + [ + { + "s": "� �" + } + ], + + "rows": 1 +} +XML + + + + + + s + String + + + + + + � � + + + 1 + +JSONColumnsWithMetadata +{ + "meta": + [ + { + "name": "s", + "type": "String" + } + ], + + "data": + { + "s": ["� �"] + }, + + "rows": 1 +} +JSONEachRow +{"s":"� �"} +JSONCompactEachRow +["� �"] +JSONColumns +{ + "s": ["� �"] +} +JSONCompactColumns +[ + ["� �"] +] +JSONObjectEachRow +{ + "row_1": {"s":"� �"} +} +JSONCompact +{ + "meta": + [ + { + "name": "s", + "type": "String" + } + ], + + "data": + [ + ["� �"] + ], + + "rows": 1 +} +JSON +{ + "meta": + [ + { + "name": "s", + "type": "String" + } + ], + + "data": + [ + { + "s": "� �" + } + ], + + "rows": 1 +} +XML + + + + + + s + String + + + + + + � � + + + 1 + +JSONColumnsWithMetadata +{ + "meta": + [ + { + "name": "s", + "type": "String" + } + ], + + "data": + { + "s": ["� �"] + }, + + "rows": 1 +} +JSONEachRow +{"s":"í ¨"} +JSONCompactEachRow +["í ¨"] +JSONColumns +{ + "s": ["í ¨"] +} +JSONCompactColumns +[ + ["í ¨"] +] +JSONObjectEachRow +{ + "row_1": {"s":"í ¨"} +} diff --git a/tests/queries/0_stateless/02452_json_utf8_validation.sql b/tests/queries/0_stateless/02452_json_utf8_validation.sql new file mode 100644 index 00000000000..e0ddbcdc919 --- /dev/null +++ b/tests/queries/0_stateless/02452_json_utf8_validation.sql @@ -0,0 +1,42 @@ +SET output_format_write_statistics = 0; +SET output_format_json_validate_utf8 = 1; +SELECT 'JSONCompact'; +SELECT '\xED\x20\xA8' AS s FORMAT JSONCompact; +SELECT 'JSON'; +SELECT '\xED\x20\xA8' AS s FORMAT JSON; +SELECT 'XML'; +SELECT '\xED\x20\xA8' AS s FORMAT XML; +SELECT 'JSONColumnsWithMetadata'; +SELECT '\xED\x20\xA8' AS s FORMAT JSONColumnsWithMetadata; +SELECT 'JSONEachRow'; +SELECT '\xED\x20\xA8' AS s FORMAT JSONEachRow; +SELECT 'JSONCompactEachRow'; +SELECT '\xED\x20\xA8' AS s FORMAT JSONCompactEachRow; +SELECT 'JSONColumns'; +SELECT '\xED\x20\xA8' AS s FORMAT JSONColumns; +SELECT 'JSONCompactColumns'; +SELECT '\xED\x20\xA8' AS s FORMAT JSONCompactColumns; +SELECT 'JSONObjectEachRow'; +SELECT '\xED\x20\xA8' AS s FORMAT JSONObjectEachRow; + +SET output_format_json_validate_utf8 = 0; +SELECT 'JSONCompact'; +SELECT '\xED\x20\xA8' AS s FORMAT JSONCompact; +SELECT 'JSON'; +SELECT '\xED\x20\xA8' AS s FORMAT JSON; +SELECT 'XML'; +SELECT '\xED\x20\xA8' AS s FORMAT XML; +SELECT 'JSONColumnsWithMetadata'; +SELECT '\xED\x20\xA8' AS s FORMAT JSONColumnsWithMetadata; +SELECT 'JSONEachRow'; +SELECT '\xED\x20\xA8' AS s FORMAT JSONEachRow; +SELECT 'JSONCompactEachRow'; +SELECT '\xED\x20\xA8' AS s FORMAT JSONCompactEachRow; +SELECT 'JSONColumns'; +SELECT '\xED\x20\xA8' AS s FORMAT JSONColumns; +SELECT 'JSONCompactColumns'; +SELECT '\xED\x20\xA8' AS s FORMAT JSONCompactColumns; +SELECT 'JSONObjectEachRow'; +SELECT '\xED\x20\xA8' AS s FORMAT JSONObjectEachRow; + + From fd2dc17f98914e4f9299d2829add0de8c4934708 Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Thu, 22 Sep 2022 16:58:32 +0200 Subject: [PATCH 32/54] Update 02344_describe_cache.reference --- tests/queries/0_stateless/02344_describe_cache.reference | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/queries/0_stateless/02344_describe_cache.reference b/tests/queries/0_stateless/02344_describe_cache.reference index e49e1c6b193..0933cb4e107 100644 --- a/tests/queries/0_stateless/02344_describe_cache.reference +++ b/tests/queries/0_stateless/02344_describe_cache.reference @@ -1,2 +1,2 @@ -22548578304 1048576 104857600 1 0 0 0 s3_cache/ 0 -22548578304 1048576 104857600 1 0 0 0 s3_cache_2/ 0 +2147483648 1048576 104857600 1 0 0 0 s3_cache/ 0 +2147483648 1048576 104857600 1 0 0 0 s3_cache_2/ 0 From 133446a40402da733dbf26d0f8e1aa71611cf31a Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Thu, 22 Sep 2022 17:00:48 +0200 Subject: [PATCH 33/54] Update storage_conf.xml --- tests/config/config.d/storage_conf.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/config/config.d/storage_conf.xml b/tests/config/config.d/storage_conf.xml index 3c24633d413..dcf4d8e9100 100644 --- a/tests/config/config.d/storage_conf.xml +++ b/tests/config/config.d/storage_conf.xml @@ -59,6 +59,7 @@ s3_cache_2/ 2Gi 0 + 100Mi cache From a6b1586b17b230ed691be8cfb2736b839f806147 Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Thu, 22 Sep 2022 17:03:19 +0200 Subject: [PATCH 34/54] Update storing-data.md --- docs/en/operations/storing-data.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/en/operations/storing-data.md b/docs/en/operations/storing-data.md index 290c2d7c96c..53c763eeeb5 100644 --- a/docs/en/operations/storing-data.md +++ b/docs/en/operations/storing-data.md @@ -155,7 +155,7 @@ Example of configuration for versions earlier than 22.8: ... ... s3 configuration ... 1 - 10Gi + 2147483648 @@ -172,7 +172,7 @@ Cache **configuration settings**: - `path` - path to the directory with cache. Default: None, this setting is obligatory. -- `max_size` - maximum size of the cache in bytes or in readable format (`ki, Mi, Gi, etc`, example `10Gi`). When the limit is reached, cache files are evicted according to the cache eviction policy. Default: None, this setting is obligatory. +- `max_size` - maximum size of the cache in bytes or in readable format, e.g. `ki, Mi, Gi, etc`, example `10Gi` (such format works starting from `22.9` version). When the limit is reached, cache files are evicted according to the cache eviction policy. Default: None, this setting is obligatory. - `cache_on_write_operations` - allow to turn on `write-through` cache (caching data on any write operations: `INSERT` queries, background merges). Default: `false`. The `write-through` cache can be disabled per query using setting `enable_filesystem_cache_on_write_operations` (data is cached only if both cache config settings and corresponding query setting are enabled). From 951a85a0fb407d0e912296bc1b803d06d0a5a699 Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Thu, 22 Sep 2022 17:04:44 +0200 Subject: [PATCH 35/54] Update storing-data.md --- docs/en/operations/storing-data.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/en/operations/storing-data.md b/docs/en/operations/storing-data.md index 53c763eeeb5..bc7b3236039 100644 --- a/docs/en/operations/storing-data.md +++ b/docs/en/operations/storing-data.md @@ -155,7 +155,7 @@ Example of configuration for versions earlier than 22.8: ... ... s3 configuration ... 1 - 2147483648 + 10737418240 From 72b5c845ef4b871a11cc13c3db82ccdfb1fe086f Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Thu, 22 Sep 2022 19:37:35 +0000 Subject: [PATCH 36/54] Add test --- .../02317_distinct_in_order_optimization_explain.reference | 4 ++++ .../02317_distinct_in_order_optimization_explain.sh | 3 +++ 2 files changed, 7 insertions(+) diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference index b41b47200ca..2511c806e1b 100644 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.reference @@ -77,3 +77,7 @@ Sorting (Stream): a ASC, b ASC Sorting (Stream): a DESC, b DESC Sorting (Stream): a DESC, b DESC Sorting (Stream): a DESC, b DESC +-- enabled, check that disabling other 'read in order' optimizations do not disable distinct in order optimization +Sorting (Stream): a ASC, b ASC +Sorting (Stream): a ASC, b ASC +Sorting (Stream): a ASC, b ASC diff --git a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh index f23843dc3e5..46919ae49b2 100755 --- a/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh +++ b/tests/queries/0_stateless/02317_distinct_in_order_optimization_explain.sh @@ -92,4 +92,7 @@ $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;$ENABLE_READ_IN_ORDER;explain plan echo "-- enabled, check that ReadFromMergeTree sorting description is NOT overwritten by DISTINCT optimization (2), - direction used from ORDER BY clause" $CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;$ENABLE_READ_IN_ORDER;explain plan sorting=1 select distinct b, a from distinct_in_order_explain order by a DESC, b DESC" | eval $FIND_SORTING_PROPERTIES +echo "-- enabled, check that disabling other 'read in order' optimizations do not disable distinct in order optimization" +$CLICKHOUSE_CLIENT -nq "$ENABLE_OPTIMIZATION;set optimize_read_in_order=0;set optimize_aggregation_in_order=0;set optimize_read_in_window_order=0;explain plan sorting=1 select distinct a,b from distinct_in_order_explain" | eval $FIND_SORTING_PROPERTIES + $CLICKHOUSE_CLIENT -q "drop table if exists distinct_in_order_explain sync" From 9ea277c0477344298b2d1b5a064bb2df25398f07 Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Thu, 22 Sep 2022 19:52:02 +0000 Subject: [PATCH 37/54] Remove unnecessary method from ReadFromMergeTree --- .../reuseStorageOrderingForWindowFunctions.cpp | 2 -- src/Processors/QueryPlan/ReadFromMergeTree.cpp | 12 ------------ src/Processors/QueryPlan/ReadFromMergeTree.h | 1 - 3 files changed, 15 deletions(-) diff --git a/src/Processors/QueryPlan/Optimizations/reuseStorageOrderingForWindowFunctions.cpp b/src/Processors/QueryPlan/Optimizations/reuseStorageOrderingForWindowFunctions.cpp index 401774b390e..03b7d476f17 100644 --- a/src/Processors/QueryPlan/Optimizations/reuseStorageOrderingForWindowFunctions.cpp +++ b/src/Processors/QueryPlan/Optimizations/reuseStorageOrderingForWindowFunctions.cpp @@ -91,8 +91,6 @@ size_t tryReuseStorageOrderingForWindowFunctions(QueryPlan::Node * parent_node, window->getWindowDescription().full_sort_description, query_info.syntax_analyzer_result); - read_from_merge_tree->setQueryInfoOrderOptimizer(order_optimizer); - /// If we don't have filtration, we can pushdown limit to reading stage for optimizations. UInt64 limit = (select_query->hasFiltration() || select_query->groupBy()) ? 0 : InterpreterSelectQuery::getLimitForSorting(*select_query, context); diff --git a/src/Processors/QueryPlan/ReadFromMergeTree.cpp b/src/Processors/QueryPlan/ReadFromMergeTree.cpp index 54f73b6a819..6e1fab2c14f 100644 --- a/src/Processors/QueryPlan/ReadFromMergeTree.cpp +++ b/src/Processors/QueryPlan/ReadFromMergeTree.cpp @@ -1018,18 +1018,6 @@ MergeTreeDataSelectAnalysisResultPtr ReadFromMergeTree::selectRangesToRead( return std::make_shared(MergeTreeDataSelectAnalysisResult{.result = std::move(result)}); } -void ReadFromMergeTree::setQueryInfoOrderOptimizer(std::shared_ptr order_optimizer) -{ - if (query_info.projection) - { - query_info.projection->order_optimizer = order_optimizer; - } - else - { - query_info.order_optimizer = order_optimizer; - } -} - void ReadFromMergeTree::setQueryInfoInputOrderInfo(InputOrderInfoPtr order_info) { if (query_info.projection) diff --git a/src/Processors/QueryPlan/ReadFromMergeTree.h b/src/Processors/QueryPlan/ReadFromMergeTree.h index 43e7eecac5d..f90eaef2c63 100644 --- a/src/Processors/QueryPlan/ReadFromMergeTree.h +++ b/src/Processors/QueryPlan/ReadFromMergeTree.h @@ -151,7 +151,6 @@ public: const SelectQueryInfo & getQueryInfo() const { return query_info; } StorageMetadataPtr getStorageMetadata() const { return metadata_for_reading; } - void setQueryInfoOrderOptimizer(std::shared_ptr read_in_order_optimizer); void setQueryInfoInputOrderInfo(InputOrderInfoPtr order_info); private: From 6551966dc7a67ce0dbcd68cd7ae6f5b23e4e5fdc Mon Sep 17 00:00:00 2001 From: Igor Nikonov Date: Thu, 22 Sep 2022 20:47:00 +0000 Subject: [PATCH 38/54] Change the way reading in order is requested from plan optimizations --- .../QueryPlan/Optimizations/distinctReadInOrder.cpp | 7 ++----- .../reuseStorageOrderingForWindowFunctions.cpp | 2 +- src/Processors/QueryPlan/ReadFromMergeTree.cpp | 11 ++++++----- src/Processors/QueryPlan/ReadFromMergeTree.h | 2 +- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp index 7887a54ea9c..6ec7ee98d08 100644 --- a/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp +++ b/src/Processors/QueryPlan/Optimizations/distinctReadInOrder.cpp @@ -79,11 +79,8 @@ size_t tryDistinctReadInOrder(QueryPlan::Node * parent_node, QueryPlan::Nodes &) return 0; /// update input order info in read_from_merge_tree step - const int direction = output_sort_desc.front().direction; /// for DISTINCT direction doesn't matter, so use current direction - /// example: SELECT DISTINCT a from t ORDER BY a DESC; - InputOrderInfoPtr order_info - = std::make_shared(SortDescription{}, number_of_sorted_distinct_columns, direction, pre_distinct->getLimitHint()); - read_from_merge_tree->setQueryInfoInputOrderInfo(order_info); + const int direction = 0; /// for DISTINCT direction doesn't matter, ReadFromMergeTree will choose proper one + read_from_merge_tree->requestReadingInOrder(number_of_sorted_distinct_columns, direction, pre_distinct->getLimitHint()); /// update data stream's sorting properties for found transforms const DataStream * input_stream = &read_from_merge_tree->getOutputStream(); diff --git a/src/Processors/QueryPlan/Optimizations/reuseStorageOrderingForWindowFunctions.cpp b/src/Processors/QueryPlan/Optimizations/reuseStorageOrderingForWindowFunctions.cpp index 03b7d476f17..a8431d38a78 100644 --- a/src/Processors/QueryPlan/Optimizations/reuseStorageOrderingForWindowFunctions.cpp +++ b/src/Processors/QueryPlan/Optimizations/reuseStorageOrderingForWindowFunctions.cpp @@ -101,7 +101,7 @@ size_t tryReuseStorageOrderingForWindowFunctions(QueryPlan::Node * parent_node, if (order_info) { - read_from_merge_tree->setQueryInfoInputOrderInfo(order_info); + read_from_merge_tree->requestReadingInOrder(order_info->used_prefix_of_sorting_key_size, order_info->direction, order_info->limit); sorting->convertToFinishSorting(order_info->sort_description_for_merging); } diff --git a/src/Processors/QueryPlan/ReadFromMergeTree.cpp b/src/Processors/QueryPlan/ReadFromMergeTree.cpp index 6e1fab2c14f..57eeb5dba2d 100644 --- a/src/Processors/QueryPlan/ReadFromMergeTree.cpp +++ b/src/Processors/QueryPlan/ReadFromMergeTree.cpp @@ -1018,16 +1018,17 @@ MergeTreeDataSelectAnalysisResultPtr ReadFromMergeTree::selectRangesToRead( return std::make_shared(MergeTreeDataSelectAnalysisResult{.result = std::move(result)}); } -void ReadFromMergeTree::setQueryInfoInputOrderInfo(InputOrderInfoPtr order_info) +void ReadFromMergeTree::requestReadingInOrder(size_t prefix_size, int direction, size_t limit) { + /// if dirction is not set, use current one + if (!direction) + direction = getSortDirection(); + + auto order_info = std::make_shared(SortDescription{}, prefix_size, direction, limit); if (query_info.projection) - { query_info.projection->input_order_info = order_info; - } else - { query_info.input_order_info = order_info; - } /// update sort info for output stream SortDescription sort_description; diff --git a/src/Processors/QueryPlan/ReadFromMergeTree.h b/src/Processors/QueryPlan/ReadFromMergeTree.h index f90eaef2c63..0a013748e91 100644 --- a/src/Processors/QueryPlan/ReadFromMergeTree.h +++ b/src/Processors/QueryPlan/ReadFromMergeTree.h @@ -151,7 +151,7 @@ public: const SelectQueryInfo & getQueryInfo() const { return query_info; } StorageMetadataPtr getStorageMetadata() const { return metadata_for_reading; } - void setQueryInfoInputOrderInfo(InputOrderInfoPtr order_info); + void requestReadingInOrder(size_t prefix_size, int direction, size_t limit); private: int getSortDirection() const From 3165e7c1fcab044e9069c8108fa41f8914efe8ad Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Fri, 23 Sep 2022 12:53:29 +0200 Subject: [PATCH 39/54] Update 02344_describe_cache.reference --- tests/queries/0_stateless/02344_describe_cache.reference | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/02344_describe_cache.reference b/tests/queries/0_stateless/02344_describe_cache.reference index 0933cb4e107..d3bb37af5cf 100644 --- a/tests/queries/0_stateless/02344_describe_cache.reference +++ b/tests/queries/0_stateless/02344_describe_cache.reference @@ -1,2 +1,2 @@ 2147483648 1048576 104857600 1 0 0 0 s3_cache/ 0 -2147483648 1048576 104857600 1 0 0 0 s3_cache_2/ 0 +2147483648 1048576 104857600 0 0 0 0 s3_cache_2/ 0 From 4b170cc1cf349856b0e6cde6d81b855d6b606a42 Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Fri, 23 Sep 2022 12:54:10 +0200 Subject: [PATCH 40/54] Update storing-data.md --- docs/en/operations/storing-data.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/en/operations/storing-data.md b/docs/en/operations/storing-data.md index bc7b3236039..43623577e66 100644 --- a/docs/en/operations/storing-data.md +++ b/docs/en/operations/storing-data.md @@ -172,7 +172,7 @@ Cache **configuration settings**: - `path` - path to the directory with cache. Default: None, this setting is obligatory. -- `max_size` - maximum size of the cache in bytes or in readable format, e.g. `ki, Mi, Gi, etc`, example `10Gi` (such format works starting from `22.9` version). When the limit is reached, cache files are evicted according to the cache eviction policy. Default: None, this setting is obligatory. +- `max_size` - maximum size of the cache in bytes or in readable format, e.g. `ki, Mi, Gi, etc`, example `10Gi` (such format works starting from `22.10` version). When the limit is reached, cache files are evicted according to the cache eviction policy. Default: None, this setting is obligatory. - `cache_on_write_operations` - allow to turn on `write-through` cache (caching data on any write operations: `INSERT` queries, background merges). Default: `false`. The `write-through` cache can be disabled per query using setting `enable_filesystem_cache_on_write_operations` (data is cached only if both cache config settings and corresponding query setting are enabled). From 8d2052307e427730ea2e249266086e950ba3f171 Mon Sep 17 00:00:00 2001 From: robot-clickhouse Date: Fri, 23 Sep 2022 11:53:04 +0000 Subject: [PATCH 41/54] Update version_date.tsv and changelogs after v22.9.2.7-stable --- docker/server/Dockerfile.alpine | 2 +- docker/server/Dockerfile.ubuntu | 2 +- docs/changelogs/v22.9.2.7-stable.md | 20 ++++++++++++++++++++ utils/list-versions/version_date.tsv | 2 ++ 4 files changed, 24 insertions(+), 2 deletions(-) create mode 100644 docs/changelogs/v22.9.2.7-stable.md diff --git a/docker/server/Dockerfile.alpine b/docker/server/Dockerfile.alpine index 1a672f30a74..d40e6dd5379 100644 --- a/docker/server/Dockerfile.alpine +++ b/docker/server/Dockerfile.alpine @@ -33,7 +33,7 @@ RUN arch=${TARGETARCH:-amd64} \ # lts / testing / prestable / etc ARG REPO_CHANNEL="stable" ARG REPOSITORY="https://packages.clickhouse.com/tgz/${REPO_CHANNEL}" -ARG VERSION="22.8.5.29" +ARG VERSION="22.9.2.7" ARG PACKAGES="clickhouse-client clickhouse-server clickhouse-common-static" # user/group precreated explicitly with fixed uid/gid on purpose. diff --git a/docker/server/Dockerfile.ubuntu b/docker/server/Dockerfile.ubuntu index db76a9fab1d..1d70c46d9ba 100644 --- a/docker/server/Dockerfile.ubuntu +++ b/docker/server/Dockerfile.ubuntu @@ -21,7 +21,7 @@ RUN sed -i "s|http://archive.ubuntu.com|${apt_archive}|g" /etc/apt/sources.list ARG REPO_CHANNEL="stable" ARG REPOSITORY="deb https://packages.clickhouse.com/deb ${REPO_CHANNEL} main" -ARG VERSION="22.8.5.29" +ARG VERSION="22.9.2.7" ARG PACKAGES="clickhouse-client clickhouse-server clickhouse-common-static" # set non-empty deb_location_url url to create a docker image diff --git a/docs/changelogs/v22.9.2.7-stable.md b/docs/changelogs/v22.9.2.7-stable.md new file mode 100644 index 00000000000..d5bbc8ac8c5 --- /dev/null +++ b/docs/changelogs/v22.9.2.7-stable.md @@ -0,0 +1,20 @@ +--- +sidebar_position: 1 +sidebar_label: 2022 +--- + +# 2022 Changelog + +### ClickHouse release v22.9.2.7-stable (362e2cefcef) FIXME as compared to v22.9.1.2603-stable (3030d4c7ff0) + +#### Improvement +* Backported in [#41709](https://github.com/ClickHouse/ClickHouse/issues/41709): Check file path for path traversal attacks in errors logger for input formats. [#41694](https://github.com/ClickHouse/ClickHouse/pull/41694) ([Kruglov Pavel](https://github.com/Avogar)). + +#### Bug Fix (user-visible misbehavior in official stable or prestable release) + +* Backported in [#41696](https://github.com/ClickHouse/ClickHouse/issues/41696): Fixes issue when docker run will fail if "https_port" is not present in config. [#41693](https://github.com/ClickHouse/ClickHouse/pull/41693) ([Yakov Olkhovskiy](https://github.com/yakov-olkhovskiy)). + +#### NOT FOR CHANGELOG / INSIGNIFICANT + +* Fix typos in JSON formats after [#40910](https://github.com/ClickHouse/ClickHouse/issues/40910) [#41614](https://github.com/ClickHouse/ClickHouse/pull/41614) ([Kruglov Pavel](https://github.com/Avogar)). + diff --git a/utils/list-versions/version_date.tsv b/utils/list-versions/version_date.tsv index 399f6066f15..6f774b3f7aa 100644 --- a/utils/list-versions/version_date.tsv +++ b/utils/list-versions/version_date.tsv @@ -1,3 +1,5 @@ +v22.9.2.7-stable 2022-09-23 +v22.9.1.2603-stable 2022-09-22 v22.8.5.29-lts 2022-09-13 v22.8.4.7-lts 2022-08-31 v22.8.3.13-lts 2022-08-29 From dff61267b4ac104efefe8f9bae9f70a5c67afdcc Mon Sep 17 00:00:00 2001 From: alesapin Date: Fri, 23 Sep 2022 14:30:50 +0200 Subject: [PATCH 42/54] More explicit logs --- src/Disks/StoragePolicy.cpp | 12 +++++++++++- src/Disks/StoragePolicy.h | 2 ++ src/Storages/MergeTree/MergeTreeData.cpp | 14 ++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/Disks/StoragePolicy.cpp b/src/Disks/StoragePolicy.cpp index 3dd60ac02d4..de55b207a45 100644 --- a/src/Disks/StoragePolicy.cpp +++ b/src/Disks/StoragePolicy.cpp @@ -39,6 +39,7 @@ StoragePolicy::StoragePolicy( const String & config_prefix, DiskSelectorPtr disks) : name(std::move(name_)) + , log(&Poco::Logger::get("StoragePolicy (" + name + ")")) { Poco::Util::AbstractConfiguration::Keys keys; String volumes_prefix = config_prefix + ".volumes"; @@ -81,11 +82,15 @@ StoragePolicy::StoragePolicy( throw Exception("Disk move factor have to be in [0., 1.] interval, but set to " + toString(move_factor) + " in storage policy " + backQuote(name), ErrorCodes::LOGICAL_ERROR); buildVolumeIndices(); + LOG_TRACE(log, "Storage policy {} created, total volumes {}", name, volumes.size()); } StoragePolicy::StoragePolicy(String name_, Volumes volumes_, double move_factor_) - : volumes(std::move(volumes_)), name(std::move(name_)), move_factor(move_factor_) + : volumes(std::move(volumes_)) + , name(std::move(name_)) + , move_factor(move_factor_) + , log(&Poco::Logger::get("StoragePolicy (" + name + ")")) { if (volumes.empty()) throw Exception("Storage policy " + backQuote(name) + " must contain at least one Volume.", ErrorCodes::NO_ELEMENTS_IN_CONFIG); @@ -94,6 +99,7 @@ StoragePolicy::StoragePolicy(String name_, Volumes volumes_, double move_factor_ throw Exception("Disk move factor have to be in [0., 1.] interval, but set to " + toString(move_factor) + " in storage policy " + backQuote(name), ErrorCodes::LOGICAL_ERROR); buildVolumeIndices(); + LOG_TRACE(log, "Storage policy {} created, total volumes {}", name, volumes.size()); } @@ -206,12 +212,16 @@ UInt64 StoragePolicy::getMaxUnreservedFreeSpace() const ReservationPtr StoragePolicy::reserve(UInt64 bytes, size_t min_volume_index) const { + LOG_TRACE(log, "Reserving bytes {} from volume index {}, total volumes {}", bytes, min_volume_index, volumes.size()); for (size_t i = min_volume_index; i < volumes.size(); ++i) { const auto & volume = volumes[i]; auto reservation = volume->reserve(bytes); if (reservation) + { + LOG_TRACE(log, "Successfuly reserved {} bytes on volume index {}", bytes, i); return reservation; + } } return {}; } diff --git a/src/Disks/StoragePolicy.h b/src/Disks/StoragePolicy.h index f8c40acd26a..b0e92bdad67 100644 --- a/src/Disks/StoragePolicy.h +++ b/src/Disks/StoragePolicy.h @@ -104,6 +104,8 @@ private: double move_factor = 0.1; /// by default move factor is 10% void buildVolumeIndices(); + + Poco::Logger * log; }; diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index d9eaff6ea45..c7dffa28ac5 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -4923,12 +4923,14 @@ ReservationPtr MergeTreeData::tryReserveSpacePreferringTTLRules( { expected_size = std::max(RESERVATION_MIN_ESTIMATION_SIZE, expected_size); + LOG_TRACE(log, "Trying reserve {} bytes preffering TTL rules", expected_size); ReservationPtr reservation; auto move_ttl_entry = selectTTLDescriptionForTTLInfos(metadata_snapshot->getMoveTTLs(), ttl_infos.moves_ttl, time_of_move, true); if (move_ttl_entry) { + LOG_TRACE(log, "Got move TTL entry, will try to reserver destination for move"); SpacePtr destination_ptr = getDestinationForMoveTTL(*move_ttl_entry, is_insert); if (!destination_ptr) { @@ -4949,10 +4951,15 @@ ReservationPtr MergeTreeData::tryReserveSpacePreferringTTLRules( } else { + LOG_TRACE(log, "Reserving bytes on selected destination"); reservation = destination_ptr->reserve(expected_size); if (reservation) + { + LOG_TRACE(log, "Reservation sucessful"); return reservation; + } else + { if (move_ttl_entry->destination_type == DataDestinationType::VOLUME) LOG_WARNING( log, @@ -4965,15 +4972,22 @@ ReservationPtr MergeTreeData::tryReserveSpacePreferringTTLRules( "Would like to reserve space on disk '{}' by TTL rule of table '{}' but there is not enough space", move_ttl_entry->destination_name, *std::atomic_load(&log_name)); + } } } // Prefer selected_disk if (selected_disk) + { + LOG_DEBUG(log, "Disk for reservation provided: {} (with type {})", selected_disk->getName(), toString(selected_disk->getDataSourceDescription().type)); reservation = selected_disk->reserve(expected_size); + } if (!reservation) + { + LOG_DEBUG(log, "No reservation, reserving using storage policy from min volume index {}", min_volume_index); reservation = getStoragePolicy()->reserve(expected_size, min_volume_index); + } return reservation; } From 148f018eae59fe132d7a117ad4f94ff2dea581c3 Mon Sep 17 00:00:00 2001 From: alesapin Date: Fri, 23 Sep 2022 14:32:19 +0200 Subject: [PATCH 43/54] Fix style: --- src/Disks/StoragePolicy.cpp | 2 +- src/Storages/MergeTree/MergeTreeData.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Disks/StoragePolicy.cpp b/src/Disks/StoragePolicy.cpp index de55b207a45..1c8c522a568 100644 --- a/src/Disks/StoragePolicy.cpp +++ b/src/Disks/StoragePolicy.cpp @@ -219,7 +219,7 @@ ReservationPtr StoragePolicy::reserve(UInt64 bytes, size_t min_volume_index) con auto reservation = volume->reserve(bytes); if (reservation) { - LOG_TRACE(log, "Successfuly reserved {} bytes on volume index {}", bytes, i); + LOG_TRACE(log, "Successfully reserved {} bytes on volume index {}", bytes, i); return reservation; } } diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index c7dffa28ac5..1f591c2d23f 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -4955,7 +4955,7 @@ ReservationPtr MergeTreeData::tryReserveSpacePreferringTTLRules( reservation = destination_ptr->reserve(expected_size); if (reservation) { - LOG_TRACE(log, "Reservation sucessful"); + LOG_TRACE(log, "Reservation successful"); return reservation; } else From 2c16232b02e6b0ee2ca5b2e3bf713133e4a0a11e Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Fri, 23 Sep 2022 14:39:05 +0200 Subject: [PATCH 44/54] fix part removal retries --- .../MergeTree/DataPartStorageOnDisk.cpp | 87 +++++++++++-------- 1 file changed, 51 insertions(+), 36 deletions(-) diff --git a/src/Storages/MergeTree/DataPartStorageOnDisk.cpp b/src/Storages/MergeTree/DataPartStorageOnDisk.cpp index 5245bc89e0c..7f04d8b85d4 100644 --- a/src/Storages/MergeTree/DataPartStorageOnDisk.cpp +++ b/src/Storages/MergeTree/DataPartStorageOnDisk.cpp @@ -223,54 +223,69 @@ void DataPartStorageOnDisk::remove( /// NOTE relative_path can contain not only part name itself, but also some prefix like /// "moving/all_1_1_1" or "detached/all_2_3_5". We should handle this case more properly. - if (part_dir_without_slash.has_parent_path()) - { - auto parent_path = part_dir_without_slash.parent_path(); - if (parent_path == "detached") - throw Exception(ErrorCodes::LOGICAL_ERROR, "Trying to remove detached part {} with path {} in remove function. It shouldn't happen", part_dir, root_path); - - part_dir_without_slash = parent_path / ("delete_tmp_" + std::string{part_dir_without_slash.filename()}); - } - else - { - part_dir_without_slash = ("delete_tmp_" + std::string{part_dir_without_slash.filename()}); - } + /// File might be already renamed on previous try + bool has_delete_prefix = part_dir_without_slash.filename().string().starts_with("delete_tmp_"); + std::optional can_remove_description; + auto disk = volume->getDisk(); fs::path to = fs::path(root_path) / part_dir_without_slash; - std::optional can_remove_description; - - auto disk = volume->getDisk(); - if (disk->exists(to)) + if (!has_delete_prefix) { - LOG_WARNING(log, "Directory {} (to which part must be renamed before removing) already exists. Most likely this is due to unclean restart or race condition. Removing it.", fullPath(disk, to)); + if (part_dir_without_slash.has_parent_path()) + { + auto parent_path = part_dir_without_slash.parent_path(); + if (parent_path == "detached") + throw Exception( + ErrorCodes::LOGICAL_ERROR, + "Trying to remove detached part {} with path {} in remove function. It shouldn't happen", + part_dir, + root_path); + + part_dir_without_slash = parent_path / ("delete_tmp_" + std::string{part_dir_without_slash.filename()}); + } + else + { + part_dir_without_slash = ("delete_tmp_" + std::string{part_dir_without_slash.filename()}); + } + + to = fs::path(root_path) / part_dir_without_slash; + + if (disk->exists(to)) + { + LOG_WARNING(log, "Directory {} (to which part must be renamed before removing) already exists. " + "Most likely this is due to unclean restart or race condition. Removing it.", fullPath(disk, to)); + try + { + can_remove_description.emplace(can_remove_callback()); + disk->removeSharedRecursive( + fs::path(to) / "", !can_remove_description->can_remove_anything, can_remove_description->files_not_to_remove); + } + catch (...) + { + LOG_ERROR( + log, "Cannot recursively remove directory {}. Exception: {}", fullPath(disk, to), getCurrentExceptionMessage(false)); + throw; + } + } + try { - can_remove_description.emplace(can_remove_callback()); - disk->removeSharedRecursive(fs::path(to) / "", !can_remove_description->can_remove_anything, can_remove_description->files_not_to_remove); + disk->moveDirectory(from, to); + onRename(root_path, part_dir_without_slash); } - catch (...) + catch (const fs::filesystem_error & e) { - LOG_ERROR(log, "Cannot recursively remove directory {}. Exception: {}", fullPath(disk, to), getCurrentExceptionMessage(false)); + if (e.code() == std::errc::no_such_file_or_directory) + { + LOG_ERROR(log, "Directory {} (part to remove) doesn't exist or one of nested files has gone. " + "Most likely this is due to manual removing. This should be discouraged. Ignoring.", fullPath(disk, to)); + return; + } throw; } } - try - { - disk->moveDirectory(from, to); - onRename(root_path, part_dir_without_slash); - } - catch (const fs::filesystem_error & e) - { - if (e.code() == std::errc::no_such_file_or_directory) - { - LOG_ERROR(log, "Directory {} (part to remove) doesn't exist or one of nested files has gone. Most likely this is due to manual removing. This should be discouraged. Ignoring.", fullPath(disk, to)); - return; - } - throw; - } - if (!can_remove_description) can_remove_description.emplace(can_remove_callback()); From c122e4dd1f09189b3b9565a0c94a287002e7b60e Mon Sep 17 00:00:00 2001 From: kssenii Date: Fri, 23 Sep 2022 15:32:05 +0200 Subject: [PATCH 45/54] Refactor log levels --- .../IO/CachedOnDiskReadBufferFromFile.cpp | 19 ++++++------------- .../Cache/LRUFileCachePriority.cpp | 6 +++--- 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/Disks/IO/CachedOnDiskReadBufferFromFile.cpp b/src/Disks/IO/CachedOnDiskReadBufferFromFile.cpp index da7f8c871cb..b6184aa4b15 100644 --- a/src/Disks/IO/CachedOnDiskReadBufferFromFile.cpp +++ b/src/Disks/IO/CachedOnDiskReadBufferFromFile.cpp @@ -240,7 +240,6 @@ CachedOnDiskReadBufferFromFile::ImplementationBufferPtr CachedOnDiskReadBufferFromFile::getReadBufferForFileSegment(FileSegmentPtr & file_segment) { auto download_state = file_segment->state(); - LOG_TEST(log, "getReadBufferForFileSegment: {}", file_segment->getInfoForLog()); if (settings.read_from_filesystem_cache_if_exists_otherwise_bypass_cache) { @@ -251,7 +250,7 @@ CachedOnDiskReadBufferFromFile::getReadBufferForFileSegment(FileSegmentPtr & fil } else { - LOG_DEBUG(log, "Bypassing cache because `read_from_filesystem_cache_if_exists_otherwise_bypass_cache` option is used"); + LOG_TRACE(log, "Bypassing cache because `read_from_filesystem_cache_if_exists_otherwise_bypass_cache` option is used"); read_type = ReadType::REMOTE_FS_READ_BYPASS_CACHE; return getRemoteFSReadBuffer(*file_segment, read_type); } @@ -263,7 +262,7 @@ CachedOnDiskReadBufferFromFile::getReadBufferForFileSegment(FileSegmentPtr & fil { case FileSegment::State::SKIP_CACHE: { - LOG_DEBUG(log, "Bypassing cache because file segment state is `SKIP_CACHE`"); + LOG_TRACE(log, "Bypassing cache because file segment state is `SKIP_CACHE`"); read_type = ReadType::REMOTE_FS_READ_BYPASS_CACHE; return getRemoteFSReadBuffer(*file_segment, read_type); } @@ -358,7 +357,7 @@ CachedOnDiskReadBufferFromFile::getReadBufferForFileSegment(FileSegmentPtr & fil } else { - LOG_DEBUG( + LOG_TRACE( log, "Bypassing cache because file segment state is `PARTIALLY_DOWNLOADED_NO_CONTINUATION` and downloaded part already used"); read_type = ReadType::REMOTE_FS_READ_BYPASS_CACHE; @@ -658,7 +657,7 @@ void CachedOnDiskReadBufferFromFile::predownload(FileSegmentPtr & file_segment) implementation_buffer->setReadUntilPosition(file_segment->range().right + 1); /// [..., range.right] implementation_buffer->seek(file_offset_of_buffer_end, SEEK_SET); - LOG_TEST( + LOG_TRACE( log, "Predownload failed because of space limit. " "Will read from remote filesystem starting from offset: {}", @@ -786,10 +785,7 @@ bool CachedOnDiskReadBufferFromFile::nextImplStep() assertCorrectness(); if (file_offset_of_buffer_end == read_until_position) - { - LOG_TEST(log, "Read finished on offset {}", file_offset_of_buffer_end); return false; - } if (!initialized) initialize(file_offset_of_buffer_end, getTotalSizeToRead()); @@ -813,10 +809,7 @@ bool CachedOnDiskReadBufferFromFile::nextImplStep() { bool need_complete_file_segment = file_segment->isDownloader(); if (need_complete_file_segment) - { - LOG_TEST(log, "Resetting downloader {} from scope exit", file_segment->getDownloader()); file_segment->completePartAndResetDownloader(); - } } chassert(!file_segment->isDownloader()); @@ -956,12 +949,12 @@ bool CachedOnDiskReadBufferFromFile::nextImplStep() else { chassert(file_segment->state() == FileSegment::State::PARTIALLY_DOWNLOADED_NO_CONTINUATION); - LOG_TEST(log, "Bypassing cache because writeCache method failed"); + LOG_TRACE(log, "Bypassing cache because writeCache method failed"); } } else { - LOG_DEBUG(log, "No space left in cache, will continue without cache download"); + LOG_TRACE(log, "No space left in cache, will continue without cache download"); file_segment->completeWithState(FileSegment::State::PARTIALLY_DOWNLOADED_NO_CONTINUATION); } diff --git a/src/Interpreters/Cache/LRUFileCachePriority.cpp b/src/Interpreters/Cache/LRUFileCachePriority.cpp index 17fbd2c2092..8010b9c682b 100644 --- a/src/Interpreters/Cache/LRUFileCachePriority.cpp +++ b/src/Interpreters/Cache/LRUFileCachePriority.cpp @@ -34,7 +34,7 @@ IFileCachePriority::WriteIterator LRUFileCachePriority::add(const Key & key, siz CurrentMetrics::add(CurrentMetrics::FilesystemCacheSize, size); CurrentMetrics::add(CurrentMetrics::FilesystemCacheElements); - LOG_DEBUG(log, "Added entry into LRU queue, key: {}, offset: {}", key.toString(), offset); + LOG_TRACE(log, "Added entry into LRU queue, key: {}, offset: {}", key.toString(), offset); return std::make_shared(this, iter); } @@ -54,7 +54,7 @@ void LRUFileCachePriority::removeAll(std::lock_guard &) CurrentMetrics::sub(CurrentMetrics::FilesystemCacheSize, cache_size); CurrentMetrics::sub(CurrentMetrics::FilesystemCacheElements, queue.size()); - LOG_DEBUG(log, "Removed all entries from LRU queue"); + LOG_TRACE(log, "Removed all entries from LRU queue"); queue.clear(); cache_size = 0; @@ -88,7 +88,7 @@ void LRUFileCachePriority::LRUFileCacheIterator::removeAndGetNext(std::lock_guar CurrentMetrics::sub(CurrentMetrics::FilesystemCacheSize, queue_iter->size); CurrentMetrics::sub(CurrentMetrics::FilesystemCacheElements); - LOG_DEBUG(cache_priority->log, "Removed entry from LRU queue, key: {}, offset: {}", queue_iter->key.toString(), queue_iter->offset); + LOG_TRACE(cache_priority->log, "Removed entry from LRU queue, key: {}, offset: {}", queue_iter->key.toString(), queue_iter->offset); queue_iter = cache_priority->queue.erase(queue_iter); } From 06e0f554d8aa45c17ee1f04a6747a29d73c23c7f Mon Sep 17 00:00:00 2001 From: alesapin Date: Fri, 23 Sep 2022 16:46:53 +0200 Subject: [PATCH 46/54] Fix fetch to local disk --- docker/test/stress/run.sh | 2 +- tests/config/config.d/s3_storage_policy_by_default.xml | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/docker/test/stress/run.sh b/docker/test/stress/run.sh index 25bc64261f3..b1cfe42d28f 100755 --- a/docker/test/stress/run.sh +++ b/docker/test/stress/run.sh @@ -243,7 +243,7 @@ export USE_S3_STORAGE_FOR_MERGE_TREE=1 configure # But we still need default disk because some tables loaded only into it -sudo cat /etc/clickhouse-server/config.d/s3_storage_policy_by_default.xml | sed "s|s3|s3default|" > /etc/clickhouse-server/config.d/s3_storage_policy_by_default.xml.tmp +sudo cat /etc/clickhouse-server/config.d/s3_storage_policy_by_default.xml | sed "s|
s3
|
s3
default|" > /etc/clickhouse-server/config.d/s3_storage_policy_by_default.xml.tmp mv /etc/clickhouse-server/config.d/s3_storage_policy_by_default.xml.tmp /etc/clickhouse-server/config.d/s3_storage_policy_by_default.xml sudo chown clickhouse /etc/clickhouse-server/config.d/s3_storage_policy_by_default.xml sudo chgrp clickhouse /etc/clickhouse-server/config.d/s3_storage_policy_by_default.xml diff --git a/tests/config/config.d/s3_storage_policy_by_default.xml b/tests/config/config.d/s3_storage_policy_by_default.xml index b4a2d697c78..9685512a12a 100644 --- a/tests/config/config.d/s3_storage_policy_by_default.xml +++ b/tests/config/config.d/s3_storage_policy_by_default.xml @@ -13,9 +13,7 @@ -
- s3 -
+
s3
 From cc6776fc5d54a19c49af55219dfcada1e15ac9b9 Mon Sep 17 00:00:00 2001 From: alesapin Date: Fri, 23 Sep 2022 17:16:32 +0200 Subject: [PATCH 47/54] Fix test --- tests/queries/0_stateless/02067_lost_part_s3.sql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/queries/0_stateless/02067_lost_part_s3.sql b/tests/queries/0_stateless/02067_lost_part_s3.sql index 85ce2acddda..c4e69f68a5d 100644 --- a/tests/queries/0_stateless/02067_lost_part_s3.sql +++ b/tests/queries/0_stateless/02067_lost_part_s3.sql @@ -15,6 +15,8 @@ INSERT INTO partslost_0 SELECT toString(number) AS x from system.numbers LIMIT 1 ALTER TABLE partslost_0 ADD INDEX idx x TYPE tokenbf_v1(285000, 3, 12345) GRANULARITY 3; +SET mutations_sync = 2; + ALTER TABLE partslost_0 MATERIALIZE INDEX idx; -- In worst case doesn't check anything, but it's not flaky From 307314e1bd8cc93bcfa52abdef6d605e9381ce4e Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Sat, 24 Sep 2022 13:42:58 +0200 Subject: [PATCH 48/54] Update CachedOnDiskReadBufferFromFile.cpp --- src/Disks/IO/CachedOnDiskReadBufferFromFile.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Disks/IO/CachedOnDiskReadBufferFromFile.cpp b/src/Disks/IO/CachedOnDiskReadBufferFromFile.cpp index b6184aa4b15..1c33adac44a 100644 --- a/src/Disks/IO/CachedOnDiskReadBufferFromFile.cpp +++ b/src/Disks/IO/CachedOnDiskReadBufferFromFile.cpp @@ -250,7 +250,7 @@ CachedOnDiskReadBufferFromFile::getReadBufferForFileSegment(FileSegmentPtr & fil } else { - LOG_TRACE(log, "Bypassing cache because `read_from_filesystem_cache_if_exists_otherwise_bypass_cache` option is used"); + LOG_TEST(log, "Bypassing cache because `read_from_filesystem_cache_if_exists_otherwise_bypass_cache` option is used"); read_type = ReadType::REMOTE_FS_READ_BYPASS_CACHE; return getRemoteFSReadBuffer(*file_segment, read_type); } From ad48c249a5758a978375b64f404694176cf17e9a Mon Sep 17 00:00:00 2001 From: alesapin Date: Sat, 24 Sep 2022 16:14:22 +0200 Subject: [PATCH 49/54] No bc for test --- .../0_stateless/02381_compress_marks_and_primary_key.sql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/queries/0_stateless/02381_compress_marks_and_primary_key.sql b/tests/queries/0_stateless/02381_compress_marks_and_primary_key.sql index b929c9a3a2d..cf5ca15adeb 100644 --- a/tests/queries/0_stateless/02381_compress_marks_and_primary_key.sql +++ b/tests/queries/0_stateless/02381_compress_marks_and_primary_key.sql @@ -1,3 +1,5 @@ +-- Tags: no-backward-compatibility-check + drop table if exists test_02381; create table test_02381(a UInt64, b UInt64) ENGINE = MergeTree order by (a, b); insert into test_02381 select number, number * 10 from system.numbers limit 1000000; From e8978165722603b3f902430552abdc3453e096c6 Mon Sep 17 00:00:00 2001 From: Nikita Mikhaylov Date: Sun, 25 Sep 2022 14:06:13 +0200 Subject: [PATCH 50/54] Evict stale DNS entries from cache in case of network errors (#41707) --- base/base/CachedFn.h | 75 --------------------------- src/Client/Connection.cpp | 6 +++ src/Common/DNSResolver.cpp | 76 ++++++++++++++++++++-------- src/Common/DNSResolver.h | 4 ++ src/Common/StackTrace.cpp | 27 ++++++++-- src/Common/tests/gtest_cached_fn.cpp | 54 -------------------- src/IO/S3/PocoHTTPClient.cpp | 8 +++ 7 files changed, 94 insertions(+), 156 deletions(-) delete mode 100644 base/base/CachedFn.h delete mode 100644 src/Common/tests/gtest_cached_fn.cpp diff --git a/base/base/CachedFn.h b/base/base/CachedFn.h deleted file mode 100644 index 19b2a8ce2c0..00000000000 --- a/base/base/CachedFn.h +++ /dev/null @@ -1,75 +0,0 @@ -#pragma once - -#include -#include -#include -#include "FnTraits.h" - -/** - * Caching proxy for a functor that decays to a pointer-to-function. - * Saves pairs (func args, func result on args). - * Cache size is unlimited. Cache items are evicted only on manual drop. - * Invocation/update is O(log(saved cache values)). - * - * See Common/tests/cached_fn.cpp for examples. - */ -template -struct CachedFn -{ -private: - using Traits = FnTraits; - using DecayedArgs = TypeListMap; - using Key = TypeListChangeRoot; - using Result = typename Traits::Ret; - - std::map cache; // Can't use hashmap as tuples are unhashable by default - mutable std::mutex mutex; - -public: - template - Result operator()(Args && ...args) - { - Key key{std::forward(args)...}; - - { - std::lock_guard lock(mutex); - - if (auto it = cache.find(key); it != cache.end()) - return it->second; - } - - Result res = std::apply(Func, key); - - { - std::lock_guard lock(mutex); - cache.emplace(std::move(key), res); - } - - return res; - } - - template - void update(Args && ...args) - { - Key key{std::forward(args)...}; - Result res = std::apply(Func, key); - - { - std::lock_guard lock(mutex); - // TODO Can't use emplace(std::move(key), ..), causes test_host_ip_change errors. - cache[key] = std::move(res); - } - } - - size_t size() const - { - std::lock_guard lock(mutex); - return cache.size(); - } - - void drop() - { - std::lock_guard lock(mutex); - cache.clear(); - } -}; diff --git a/src/Client/Connection.cpp b/src/Client/Connection.cpp index 7a663195655..a9795e75b28 100644 --- a/src/Client/Connection.cpp +++ b/src/Client/Connection.cpp @@ -179,6 +179,9 @@ void Connection::connect(const ConnectionTimeouts & timeouts) { disconnect(); + /// Remove this possible stale entry from cache + DNSResolver::instance().removeHostFromCache(host); + /// Add server address to exception. Also Exception will remember stack trace. It's a pity that more precise exception type is lost. throw NetException(e.displayText() + " (" + getDescription() + ")", ErrorCodes::NETWORK_ERROR); } @@ -186,6 +189,9 @@ void Connection::connect(const ConnectionTimeouts & timeouts) { disconnect(); + /// Remove this possible stale entry from cache + DNSResolver::instance().removeHostFromCache(host); + /// Add server address to exception. Also Exception will remember stack trace. It's a pity that more precise exception type is lost. /// This exception can only be thrown from socket->connect(), so add information about connection timeout. const auto & connection_timeout = static_cast(secure) ? timeouts.secure_connection_timeout : timeouts.connection_timeout; diff --git a/src/Common/DNSResolver.cpp b/src/Common/DNSResolver.cpp index 67d87f757c7..1e5ec09f262 100644 --- a/src/Common/DNSResolver.cpp +++ b/src/Common/DNSResolver.cpp @@ -1,7 +1,8 @@ #include "DNSResolver.h" -#include +#include #include #include +#include #include #include #include @@ -12,6 +13,7 @@ #include #include #include +#include #include "DNSPTRResolverProvider.h" namespace ProfileEvents @@ -41,9 +43,11 @@ namespace ErrorCodes extern const int DNS_ERROR; } +namespace +{ /// Slightly altered implementation from https://github.com/pocoproject/poco/blob/poco-1.6.1/Net/src/SocketAddress.cpp#L86 -static void splitHostAndPort(const std::string & host_and_port, std::string & out_host, UInt16 & out_port) +void splitHostAndPort(const std::string & host_and_port, std::string & out_host, UInt16 & out_port) { String port_str; out_host.clear(); @@ -84,7 +88,7 @@ static void splitHostAndPort(const std::string & host_and_port, std::string & ou throw Exception("Port must be numeric", ErrorCodes::BAD_ARGUMENTS); } -static DNSResolver::IPAddresses hostByName(const std::string & host) +DNSResolver::IPAddresses hostByName(const std::string & host) { /// Do not resolve IPv6 (or IPv4) if no local IPv6 (or IPv4) addresses are configured. /// It should not affect client address checking, since client cannot connect from IPv6 address @@ -112,7 +116,7 @@ static DNSResolver::IPAddresses hostByName(const std::string & host) return addresses; } -static DNSResolver::IPAddresses resolveIPAddressImpl(const std::string & host) +DNSResolver::IPAddresses resolveIPAddressImpl(const std::string & host) { Poco::Net::IPAddress ip; @@ -136,7 +140,13 @@ static DNSResolver::IPAddresses resolveIPAddressImpl(const std::string & host) return addresses; } -static std::unordered_set reverseResolveImpl(const Poco::Net::IPAddress & address) +DNSResolver::IPAddresses resolveIPAddressWithCache(CacheBase & cache, const std::string & host) +{ + auto [result, _ ] = cache.getOrSet(host, [&host]() { return std::make_shared(resolveIPAddressImpl(host)); }); + return *result; +} + +std::unordered_set reverseResolveImpl(const Poco::Net::IPAddress & address) { auto ptr_resolver = DB::DNSPTRResolverProvider::get(); @@ -149,13 +159,27 @@ static std::unordered_set reverseResolveImpl(const Poco::Net::IPAddress } } +std::unordered_set reverseResolveWithCache( + CacheBase> & cache, const Poco::Net::IPAddress & address) +{ + auto [result, _ ] = cache.getOrSet(address, [&address]() { return std::make_shared>(reverseResolveImpl(address)); }); + return *result; +} + +Poco::Net::IPAddress pickAddress(const DNSResolver::IPAddresses & addresses) +{ + return addresses.front(); +} + +} + struct DNSResolver::Impl { using HostWithConsecutiveFailures = std::unordered_map; using AddressWithConsecutiveFailures = std::unordered_map; - CachedFn<&resolveIPAddressImpl> cache_host; - CachedFn<&reverseResolveImpl> cache_address; + CacheBase cache_host{100}; + CacheBase> cache_address{100}; std::mutex drop_mutex; std::mutex update_mutex; @@ -180,7 +204,7 @@ DNSResolver::DNSResolver() : impl(std::make_unique()), log(&P Poco::Net::IPAddress DNSResolver::resolveHost(const std::string & host) { - return resolveHostAll(host).front(); + return pickAddress(resolveHostAll(host)); } DNSResolver::IPAddresses DNSResolver::resolveHostAll(const std::string & host) @@ -189,7 +213,7 @@ DNSResolver::IPAddresses DNSResolver::resolveHostAll(const std::string & host) return resolveIPAddressImpl(host); addToNewHosts(host); - return impl->cache_host(host); + return resolveIPAddressWithCache(impl->cache_host, host); } Poco::Net::SocketAddress DNSResolver::resolveAddress(const std::string & host_and_port) @@ -202,7 +226,7 @@ Poco::Net::SocketAddress DNSResolver::resolveAddress(const std::string & host_an splitHostAndPort(host_and_port, host, port); addToNewHosts(host); - return Poco::Net::SocketAddress(impl->cache_host(host).front(), port); + return Poco::Net::SocketAddress(pickAddress(resolveIPAddressWithCache(impl->cache_host, host)), port); } Poco::Net::SocketAddress DNSResolver::resolveAddress(const std::string & host, UInt16 port) @@ -211,7 +235,7 @@ Poco::Net::SocketAddress DNSResolver::resolveAddress(const std::string & host, U return Poco::Net::SocketAddress(host, port); addToNewHosts(host); - return Poco::Net::SocketAddress(impl->cache_host(host).front(), port); + return Poco::Net::SocketAddress(pickAddress(resolveIPAddressWithCache(impl->cache_host, host)), port); } std::vector DNSResolver::resolveAddressList(const std::string & host, UInt16 port) @@ -224,7 +248,7 @@ std::vector DNSResolver::resolveAddressList(const std: if (!impl->disable_cache) addToNewHosts(host); - std::vector ips = impl->disable_cache ? hostByName(host) : impl->cache_host(host); + std::vector ips = impl->disable_cache ? hostByName(host) : resolveIPAddressWithCache(impl->cache_host, host); auto ips_end = std::unique(ips.begin(), ips.end()); addresses.reserve(ips_end - ips.begin()); @@ -240,13 +264,13 @@ std::unordered_set DNSResolver::reverseResolve(const Poco::Net::IPAddres return reverseResolveImpl(address); addToNewAddresses(address); - return impl->cache_address(address); + return reverseResolveWithCache(impl->cache_address, address); } void DNSResolver::dropCache() { - impl->cache_host.drop(); - impl->cache_address.drop(); + impl->cache_host.reset(); + impl->cache_address.reset(); std::scoped_lock lock(impl->update_mutex, impl->drop_mutex); @@ -257,6 +281,11 @@ void DNSResolver::dropCache() impl->host_name.reset(); } +void DNSResolver::removeHostFromCache(const std::string & host) +{ + impl->cache_host.remove(host); +} + void DNSResolver::setDisableCacheFlag(bool is_disabled) { impl->disable_cache = is_disabled; @@ -378,17 +407,20 @@ bool DNSResolver::updateCache(UInt32 max_consecutive_failures) bool DNSResolver::updateHost(const String & host) { - /// Usage of updateHost implies that host is already in cache and there is no extra computations - auto old_value = impl->cache_host(host); - impl->cache_host.update(host); - return old_value != impl->cache_host(host); + const auto old_value = resolveIPAddressWithCache(impl->cache_host, host); + auto new_value = resolveIPAddressImpl(host); + const bool result = old_value != new_value; + impl->cache_host.set(host, std::make_shared(std::move(new_value))); + return result; } bool DNSResolver::updateAddress(const Poco::Net::IPAddress & address) { - auto old_value = impl->cache_address(address); - impl->cache_address.update(address); - return old_value == impl->cache_address(address); + const auto old_value = reverseResolveWithCache(impl->cache_address, address); + auto new_value = reverseResolveImpl(address); + const bool result = old_value != new_value; + impl->cache_address.set(address, std::make_shared>(std::move(new_value))); + return result; } void DNSResolver::addToNewHosts(const String & host) diff --git a/src/Common/DNSResolver.h b/src/Common/DNSResolver.h index 83de616d81a..a05456d3de8 100644 --- a/src/Common/DNSResolver.h +++ b/src/Common/DNSResolver.h @@ -18,6 +18,7 @@ class DNSResolver : private boost::noncopyable { public: using IPAddresses = std::vector; + using IPAddressesPtr = std::shared_ptr; static DNSResolver & instance(); @@ -48,6 +49,9 @@ public: /// Drops all caches void dropCache(); + /// Removes an entry from cache or does nothing + void removeHostFromCache(const std::string & host); + /// Updates all known hosts in cache. /// Returns true if IP of any host has been changed or an element was dropped (too many failures) bool updateCache(UInt32 max_consecutive_failures); diff --git a/src/Common/StackTrace.cpp b/src/Common/StackTrace.cpp index 70f80b62868..37ce3a03cd8 100644 --- a/src/Common/StackTrace.cpp +++ b/src/Common/StackTrace.cpp @@ -4,14 +4,15 @@ #include #include #include -#include #include #include #include #include +#include #include #include +#include #include @@ -462,20 +463,36 @@ std::string StackTrace::toString(void ** frame_pointers_, size_t offset, size_t return toStringStatic(frame_pointers_copy, offset, size); } -static CachedFn<&toStringImpl> & cacheInstance() +using StackTraceRepresentation = std::tuple; +using StackTraceCache = std::map; + +static StackTraceCache & cacheInstance() { - static CachedFn<&toStringImpl> cache; + static StackTraceCache cache; return cache; } +static std::mutex stacktrace_cache_mutex; + std::string StackTrace::toStringStatic(const StackTrace::FramePointers & frame_pointers, size_t offset, size_t size) { /// Calculation of stack trace text is extremely slow. /// We use simple cache because otherwise the server could be overloaded by trash queries. - return cacheInstance()(frame_pointers, offset, size); + /// Note that this cache can grow unconditionally, but practically it should be small. + std::lock_guard lock{stacktrace_cache_mutex}; + + StackTraceRepresentation key{frame_pointers, offset, size}; + auto & cache = cacheInstance(); + if (cache.contains(key)) + return cache[key]; + + auto result = toStringImpl(frame_pointers, offset, size); + cache[key] = result; + return result; } void StackTrace::dropCache() { - cacheInstance().drop(); + std::lock_guard lock{stacktrace_cache_mutex}; + cacheInstance().clear(); } diff --git a/src/Common/tests/gtest_cached_fn.cpp b/src/Common/tests/gtest_cached_fn.cpp deleted file mode 100644 index ab15a1ee5e1..00000000000 --- a/src/Common/tests/gtest_cached_fn.cpp +++ /dev/null @@ -1,54 +0,0 @@ -#include -#include -#include -#include - -using namespace std::chrono_literals; - -constexpr int add(int x, int y) -{ - return x + y; -} - -int longFunction(int x, int y) -{ - std::this_thread::sleep_for(1s); - return x + y; -} - -auto f = [](int x, int y) { return x - y; }; - -TEST(CachedFn, Basic) -{ - CachedFn<&add> fn; - - const int res = fn(1, 2); - EXPECT_EQ(fn(1, 2), res); - - /// In GCC, lambda can't be placed in TEST, producing " has no linkage". - /// Assuming http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4268.html, - /// this is a GCC bug. - CachedFn<+f> fn2; - - const int res2 = fn2(1, 2); - EXPECT_EQ(fn2(1, 2), res2); -} - -TEST(CachedFn, CachingResults) -{ - CachedFn<&longFunction> fn; - - for (int x = 0; x < 2; ++x) - { - for (int y = 0; y < 2; ++y) - { - const int res = fn(x, y); - const time_t start = time(nullptr); - - for (int count = 0; count < 1000; ++count) - EXPECT_EQ(fn(x, y), res); - - EXPECT_LT(time(nullptr) - start, 10); - } - } -} diff --git a/src/IO/S3/PocoHTTPClient.cpp b/src/IO/S3/PocoHTTPClient.cpp index 9fe10aecda5..30373816eca 100644 --- a/src/IO/S3/PocoHTTPClient.cpp +++ b/src/IO/S3/PocoHTTPClient.cpp @@ -1,3 +1,4 @@ +#include "Common/DNSResolver.h" #include #if USE_AWS_S3 @@ -257,6 +258,9 @@ void PocoHTTPClient::makeRequestInternal( if (!request_configuration.proxy_host.empty()) { + if (enable_s3_requests_logging) + LOG_TEST(log, "Due to reverse proxy host name ({}) won't be resolved on ClickHouse side", uri); + /// Reverse proxy can replace host header with resolved ip address instead of host name. /// This can lead to request signature difference on S3 side. session = makeHTTPSession(target_uri, timeouts, /* resolve_host = */ false); @@ -443,6 +447,10 @@ void PocoHTTPClient::makeRequestInternal( response->SetClientErrorMessage(getCurrentExceptionMessage(false)); addMetric(request, S3MetricType::Errors); + + /// Probably this is socket timeout or something more or less related to DNS + /// Let's just remove this host from DNS cache to be more safe + DNSResolver::instance().removeHostFromCache(Poco::URI(uri).getHost()); } } From e3a6f2381b24280575a652136bb40ee2d05646d3 Mon Sep 17 00:00:00 2001 From: Alexey Milovidov Date: Sun, 25 Sep 2022 17:03:52 +0200 Subject: [PATCH 51/54] Revert "Merge pull request #40033 from ClickHouse/reenable-avx512-vbmi-columnvector-filter" This reverts commit 70f63d2aae2fc3d04cb33e638569a46fb5a31f39, reversing changes made to a0693c3a845e219a3d8e82efde3439701e8469d4. --- src/Columns/ColumnVector.cpp | 160 ++++------------------ src/Columns/tests/gtest_column_vector.cpp | 158 --------------------- src/Common/CpuId.h | 6 - src/Common/TargetSpecific.cpp | 7 +- src/Common/TargetSpecific.h | 33 +---- 5 files changed, 32 insertions(+), 332 deletions(-) delete mode 100644 src/Columns/tests/gtest_column_vector.cpp diff --git a/src/Columns/ColumnVector.cpp b/src/Columns/ColumnVector.cpp index 74bcdfa1768..cb570c87498 100644 --- a/src/Columns/ColumnVector.cpp +++ b/src/Columns/ColumnVector.cpp @@ -12,14 +12,12 @@ #include #include #include -#include #include #include #include #include #include -#include #include #include @@ -27,10 +25,6 @@ # include #endif -#if USE_MULTITARGET_CODE -# include -#endif - #if USE_EMBEDDED_COMPILER #include #include @@ -477,128 +471,6 @@ void ColumnVector::insertRangeFrom(const IColumn & src, size_t start, size_t memcpy(data.data() + old_size, &src_vec.data[start], length * sizeof(data[0])); } -static inline UInt64 blsr(UInt64 mask) -{ -#ifdef __BMI__ - return _blsr_u64(mask); -#else - return mask & (mask-1); -#endif -} - -DECLARE_DEFAULT_CODE( -template -inline void doFilterAligned(const UInt8 *& filt_pos, const UInt8 *& filt_end_aligned, const T *& data_pos, Container & res_data) -{ - while (filt_pos < filt_end_aligned) - { - UInt64 mask = bytes64MaskToBits64Mask(filt_pos); - - if (0xffffffffffffffff == mask) - { - res_data.insert(data_pos, data_pos + SIMD_BYTES); - } - else - { - while (mask) - { - size_t index = std::countr_zero(mask); - res_data.push_back(data_pos[index]); - mask = blsr(mask); - } - } - - filt_pos += SIMD_BYTES; - data_pos += SIMD_BYTES; - } -} -) - -namespace -{ -template -void resize(Container & res_data, size_t reserve_size) -{ -#if defined(MEMORY_SANITIZER) - res_data.resize_fill(reserve_size, static_cast(0)); // MSan doesn't recognize that all allocated memory is written by AVX-512 intrinsics. -#else - res_data.resize(reserve_size); -#endif -} -} - -DECLARE_AVX512VBMI2_SPECIFIC_CODE( -template -inline void compressStoreAVX512(const void *src, void *dst, const UInt64 mask) -{ - __m512i vsrc = _mm512_loadu_si512(src); - if constexpr (ELEMENT_WIDTH == 1) - _mm512_mask_compressstoreu_epi8(dst, static_cast<__mmask64>(mask), vsrc); - else if constexpr (ELEMENT_WIDTH == 2) - _mm512_mask_compressstoreu_epi16(dst, static_cast<__mmask32>(mask), vsrc); - else if constexpr (ELEMENT_WIDTH == 4) - _mm512_mask_compressstoreu_epi32(dst, static_cast<__mmask16>(mask), vsrc); - else if constexpr (ELEMENT_WIDTH == 8) - _mm512_mask_compressstoreu_epi64(dst, static_cast<__mmask8>(mask), vsrc); -} - -template -inline void doFilterAligned(const UInt8 *& filt_pos, const UInt8 *& filt_end_aligned, const T *& data_pos, Container & res_data) -{ - static constexpr size_t VEC_LEN = 64; /// AVX512 vector length - 64 bytes - static constexpr size_t ELEMENT_WIDTH = sizeof(T); - static constexpr size_t ELEMENTS_PER_VEC = VEC_LEN / ELEMENT_WIDTH; - static constexpr UInt64 KMASK = 0xffffffffffffffff >> (64 - ELEMENTS_PER_VEC); - - size_t current_offset = res_data.size(); - size_t reserve_size = res_data.size(); - size_t alloc_size = SIMD_BYTES * 2; - - while (filt_pos < filt_end_aligned) - { - /// to avoid calling resize too frequently, resize to reserve buffer. - if (reserve_size - current_offset < SIMD_BYTES) - { - reserve_size += alloc_size; - resize(res_data, reserve_size); - alloc_size *= 2; - } - - UInt64 mask = bytes64MaskToBits64Mask(filt_pos); - - if (0xffffffffffffffff == mask) - { - for (size_t i = 0; i < SIMD_BYTES; i += ELEMENTS_PER_VEC) - _mm512_storeu_si512(reinterpret_cast(&res_data[current_offset + i]), - _mm512_loadu_si512(reinterpret_cast(data_pos + i))); - current_offset += SIMD_BYTES; - } - else - { - if (mask) - { - for (size_t i = 0; i < SIMD_BYTES; i += ELEMENTS_PER_VEC) - { - compressStoreAVX512(reinterpret_cast(data_pos + i), - reinterpret_cast(&res_data[current_offset]), mask & KMASK); - current_offset += std::popcount(mask & KMASK); - /// prepare mask for next iter, if ELEMENTS_PER_VEC = 64, no next iter - if (ELEMENTS_PER_VEC < 64) - { - mask >>= ELEMENTS_PER_VEC; - } - } - } - } - - filt_pos += SIMD_BYTES; - data_pos += SIMD_BYTES; - } - /// resize to the real size. - res_data.resize(current_offset); -} -) - template ColumnPtr ColumnVector::filter(const IColumn::Filter & filt, ssize_t result_size_hint) const { @@ -624,13 +496,31 @@ ColumnPtr ColumnVector::filter(const IColumn::Filter & filt, ssize_t result_s static constexpr size_t SIMD_BYTES = 64; const UInt8 * filt_end_aligned = filt_pos + size / SIMD_BYTES * SIMD_BYTES; -#if USE_MULTITARGET_CODE - static constexpr bool VBMI2_CAPABLE = sizeof(T) == 1 || sizeof(T) == 2 || sizeof(T) == 4 || sizeof(T) == 8; - if (VBMI2_CAPABLE && isArchSupported(TargetArch::AVX512VBMI2)) - TargetSpecific::AVX512VBMI2::doFilterAligned(filt_pos, filt_end_aligned, data_pos, res_data); - else -#endif - TargetSpecific::Default::doFilterAligned(filt_pos, filt_end_aligned, data_pos, res_data); + while (filt_pos < filt_end_aligned) + { + UInt64 mask = bytes64MaskToBits64Mask(filt_pos); + + if (0xffffffffffffffff == mask) + { + res_data.insert(data_pos, data_pos + SIMD_BYTES); + } + else + { + while (mask) + { + size_t index = std::countr_zero(mask); + res_data.push_back(data_pos[index]); + #ifdef __BMI__ + mask = _blsr_u64(mask); + #else + mask = mask & (mask-1); + #endif + } + } + + filt_pos += SIMD_BYTES; + data_pos += SIMD_BYTES; + } while (filt_pos < filt_end) { diff --git a/src/Columns/tests/gtest_column_vector.cpp b/src/Columns/tests/gtest_column_vector.cpp deleted file mode 100644 index 9dfb8c5aeb6..00000000000 --- a/src/Columns/tests/gtest_column_vector.cpp +++ /dev/null @@ -1,158 +0,0 @@ -#include -#include -#include -#include -#include -#include - -using namespace DB; - -static pcg64 rng(randomSeed()); -static constexpr int error_code = 12345; -static constexpr size_t TEST_RUNS = 500; -static constexpr size_t MAX_ROWS = 10000; -static const std::vector filter_ratios = {1, 2, 5, 11, 32, 64, 100, 1000}; -static const size_t K = filter_ratios.size(); - -template -static MutableColumnPtr createColumn(size_t n) -{ - auto column = ColumnVector::create(); - auto & values = column->getData(); - - for (size_t i = 0; i < n; ++i) - { - values.push_back(i); - } - - return column; -} - -bool checkFilter(const PaddedPODArray &flit, const IColumn & src, const IColumn & dst) -{ - size_t n = flit.size(); - size_t dst_size = dst.size(); - size_t j = 0; /// index of dest - for (size_t i = 0; i < n; ++i) - { - if (flit[i] != 0) - { - if ((dst_size <= j) || (src.compareAt(i, j, dst, 0) != 0)) - return false; - j++; - } - } - return dst_size == j; /// filtered size check -} - -template -static void testFilter() -{ - auto test_case = [&](size_t rows, size_t filter_ratio) - { - auto vector_column = createColumn(rows); - PaddedPODArray flit(rows); - for (size_t i = 0; i < rows; ++i) - flit[i] = rng() % filter_ratio == 0; - auto res_column = vector_column->filter(flit, -1); - - if (!checkFilter(flit, *vector_column, *res_column)) - throw Exception(error_code, "VectorColumn filter failure, type: {}", typeid(T).name()); - }; - - try - { - for (size_t i = 0; i < TEST_RUNS; ++i) - { - size_t rows = rng() % MAX_ROWS + 1; - size_t filter_ratio = filter_ratios[rng() % K]; - - test_case(rows, filter_ratio); - } - } - catch (const Exception & e) - { - FAIL() << e.displayText(); - } -} - -TEST(ColumnVector, Filter) -{ - testFilter(); - testFilter(); - testFilter(); - testFilter(); - testFilter(); - testFilter(); - testFilter(); - testFilter(); - testFilter(); -} - -template -static MutableColumnPtr createIndexColumn(size_t limit, size_t rows) -{ - auto column = ColumnVector::create(); - auto & values = column->getData(); - auto max = std::numeric_limits::max(); - limit = limit > max ? max : limit; - - for (size_t i = 0; i < rows; ++i) - { - T val = rng() % limit; - values.push_back(val); - } - - return column; -} - -template -static void testIndex() -{ - static const std::vector column_sizes = {64, 128, 196, 256, 512}; - - auto test_case = [&](size_t rows, size_t index_rows, size_t limit) - { - auto vector_column = createColumn(rows); - auto index_column = createIndexColumn(rows, index_rows); - auto res_column = vector_column->index(*index_column, limit); - if (limit == 0) - limit = index_column->size(); - - /// check results - if (limit != res_column->size()) - throw Exception(error_code, "ColumnVector index size not match to limit: {} {}", typeid(T).name(), typeid(IndexType).name()); - for (size_t i = 0; i < limit; ++i) - { - /// vector_column data is the same as index, so indexed column's value will equals to index_column. - if (res_column->get64(i) != index_column->get64(i)) - throw Exception(error_code, "ColumnVector index fail: {} {}", typeid(T).name(), typeid(IndexType).name()); - } - }; - - try - { - for (size_t i = 0; i < TEST_RUNS; ++i) - { - /// make sure rows distribute in (column_sizes[r-1], colulmn_sizes[r]] - size_t row_idx = rng() % column_sizes.size(); - size_t row_base = row_idx > 0 ? column_sizes[row_idx - 1] : 0; - size_t rows = row_base + (rng() % (column_sizes[row_idx] - row_base) + 1); - size_t index_rows = rng() % MAX_ROWS + 1; - - test_case(rows, index_rows, 0); - test_case(rows, index_rows, static_cast(0.5 * index_rows)); - } - } - catch (const Exception & e) - { - FAIL() << e.displayText(); - } -} - -TEST(ColumnVector, Index) -{ - testIndex(); - testIndex(); - testIndex(); -} diff --git a/src/Common/CpuId.h b/src/Common/CpuId.h index 1e54ccf62b3..167fa22faf6 100644 --- a/src/Common/CpuId.h +++ b/src/Common/CpuId.h @@ -82,7 +82,6 @@ inline bool cpuid(UInt32 op, UInt32 * res) noexcept /// NOLINT OP(AVX512BW) \ OP(AVX512VL) \ OP(AVX512VBMI) \ - OP(AVX512VBMI2) \ OP(PREFETCHWT1) \ OP(SHA) \ OP(ADX) \ @@ -303,11 +302,6 @@ bool haveAVX512VBMI() noexcept return haveAVX512F() && ((CpuInfo(0x7, 0).registers.ecx >> 1) & 1u); } -bool haveAVX512VBMI2() noexcept -{ - return haveAVX512F() && ((CpuInfo(0x7, 0).registers.ecx >> 6) & 1u); -} - bool haveRDRAND() noexcept { return CpuInfo(0x0).registers.eax >= 0x7 && ((CpuInfo(0x1).registers.ecx >> 30) & 1u); diff --git a/src/Common/TargetSpecific.cpp b/src/Common/TargetSpecific.cpp index a5fbe7de078..70b03833775 100644 --- a/src/Common/TargetSpecific.cpp +++ b/src/Common/TargetSpecific.cpp @@ -20,8 +20,6 @@ UInt32 getSupportedArchs() result |= static_cast(TargetArch::AVX512BW); if (Cpu::CpuFlagsCache::have_AVX512VBMI) result |= static_cast(TargetArch::AVX512VBMI); - if (Cpu::CpuFlagsCache::have_AVX512VBMI2) - result |= static_cast(TargetArch::AVX512VBMI2); return result; } @@ -40,9 +38,8 @@ String toString(TargetArch arch) case TargetArch::AVX: return "avx"; case TargetArch::AVX2: return "avx2"; case TargetArch::AVX512F: return "avx512f"; - case TargetArch::AVX512BW: return "avx512bw"; - case TargetArch::AVX512VBMI: return "avx512vbmi"; - case TargetArch::AVX512VBMI2: return "avx512vbmi"; + case TargetArch::AVX512BW: return "avx512bw"; + case TargetArch::AVX512VBMI: return "avx512vbmi"; } __builtin_unreachable(); diff --git a/src/Common/TargetSpecific.h b/src/Common/TargetSpecific.h index 250642f6ee4..f078c0e3ffc 100644 --- a/src/Common/TargetSpecific.h +++ b/src/Common/TargetSpecific.h @@ -31,7 +31,7 @@ * int funcImpl() { * return 2; * } - * ) // DECLARE_AVX2_SPECIFIC_CODE + * ) // DECLARE_DEFAULT_CODE * * int func() { * #if USE_MULTITARGET_CODE @@ -80,9 +80,8 @@ enum class TargetArch : UInt32 AVX = (1 << 1), AVX2 = (1 << 2), AVX512F = (1 << 3), - AVX512BW = (1 << 4), - AVX512VBMI = (1 << 5), - AVX512VBMI2 = (1 << 6), + AVX512BW = (1 << 4), + AVX512VBMI = (1 << 5), }; /// Runtime detection. @@ -101,7 +100,6 @@ String toString(TargetArch arch); #if defined(__clang__) -#define AVX512VBMI2_FUNCTION_SPECIFIC_ATTRIBUTE __attribute__((target("sse,sse2,sse3,ssse3,sse4,popcnt,avx,avx2,avx512f,avx512bw,avx512vl,avx512vbmi,avx512vbmi2"))) #define AVX512VBMI_FUNCTION_SPECIFIC_ATTRIBUTE __attribute__((target("sse,sse2,sse3,ssse3,sse4,popcnt,avx,avx2,avx512f,avx512bw,avx512vl,avx512vbmi"))) #define AVX512BW_FUNCTION_SPECIFIC_ATTRIBUTE __attribute__((target("sse,sse2,sse3,ssse3,sse4,popcnt,avx,avx2,avx512f,avx512bw"))) #define AVX512_FUNCTION_SPECIFIC_ATTRIBUTE __attribute__((target("sse,sse2,sse3,ssse3,sse4,popcnt,avx,avx2,avx512f"))) @@ -110,8 +108,6 @@ String toString(TargetArch arch); #define SSE42_FUNCTION_SPECIFIC_ATTRIBUTE __attribute__((target("sse,sse2,sse3,ssse3,sse4,popcnt"))) #define DEFAULT_FUNCTION_SPECIFIC_ATTRIBUTE -# define BEGIN_AVX512VBMI2_SPECIFIC_CODE \ - _Pragma("clang attribute push(__attribute__((target(\"sse,sse2,sse3,ssse3,sse4,popcnt,avx,avx2,avx512f,avx512bw,avx512vl,avx512vbmi,avx512vbmi2\"))),apply_to=function)") # define BEGIN_AVX512VBMI_SPECIFIC_CODE \ _Pragma("clang attribute push(__attribute__((target(\"sse,sse2,sse3,ssse3,sse4,popcnt,avx,avx2,avx512f,avx512bw,avx512vl,avx512vbmi\"))),apply_to=function)") # define BEGIN_AVX512BW_SPECIFIC_CODE \ @@ -133,7 +129,6 @@ String toString(TargetArch arch); # define DUMMY_FUNCTION_DEFINITION [[maybe_unused]] void _dummy_function_definition(); #else -#define AVX512VBMI2_FUNCTION_SPECIFIC_ATTRIBUTE __attribute__((target("sse,sse2,sse3,ssse3,sse4,popcnt,avx,avx2,avx512f,avx512bw,avx512vl,avx512vbmi,avx512vbmi2,tune=native"))) #define AVX512VBMI_FUNCTION_SPECIFIC_ATTRIBUTE __attribute__((target("sse,sse2,sse3,ssse3,sse4,popcnt,avx,avx2,avx512f,avx512bw,avx512vl,avx512vbmi,tune=native"))) #define AVX512BW_FUNCTION_SPECIFIC_ATTRIBUTE __attribute__((target("sse,sse2,sse3,ssse3,sse4,popcnt,avx,avx2,avx512f,avx512bw,tune=native"))) #define AVX512_FUNCTION_SPECIFIC_ATTRIBUTE __attribute__((target("sse,sse2,sse3,ssse3,sse4,popcnt,avx,avx2,avx512f,tune=native"))) @@ -142,9 +137,6 @@ String toString(TargetArch arch); #define SSE42_FUNCTION_SPECIFIC_ATTRIBUTE __attribute__((target("sse,sse2,sse3,ssse3,sse4,popcnt",tune=native))) #define DEFAULT_FUNCTION_SPECIFIC_ATTRIBUTE -# define BEGIN_AVX512VBMI2_SPECIFIC_CODE \ - _Pragma("GCC push_options") \ - _Pragma("GCC target(\"sse,sse2,sse3,ssse3,sse4,popcnt,avx,avx2,avx512f,avx512bw,avx512vl,avx512vbmi,avx512vbmi2,tune=native\")") # define BEGIN_AVX512VBMI_SPECIFIC_CODE \ _Pragma("GCC push_options") \ _Pragma("GCC target(\"sse,sse2,sse3,ssse3,sse4,popcnt,avx,avx2,avx512f,avx512bw,avx512vl,avx512vbmi,tune=native\")") @@ -225,16 +217,6 @@ namespace TargetSpecific::AVX512VBMI { \ } \ END_TARGET_SPECIFIC_CODE -#define DECLARE_AVX512VBMI2_SPECIFIC_CODE(...) \ -BEGIN_AVX512VBMI2_SPECIFIC_CODE \ -namespace TargetSpecific::AVX512VBMI2 { \ - DUMMY_FUNCTION_DEFINITION \ - using namespace DB::TargetSpecific::AVX512VBMI2; \ - __VA_ARGS__ \ -} \ -END_TARGET_SPECIFIC_CODE - - #else #define USE_MULTITARGET_CODE 0 @@ -247,7 +229,6 @@ END_TARGET_SPECIFIC_CODE #define DECLARE_AVX512F_SPECIFIC_CODE(...) #define DECLARE_AVX512BW_SPECIFIC_CODE(...) #define DECLARE_AVX512VBMI_SPECIFIC_CODE(...) -#define DECLARE_AVX512VBMI2_SPECIFIC_CODE(...) #endif @@ -264,9 +245,8 @@ DECLARE_SSE42_SPECIFIC_CODE (__VA_ARGS__) \ DECLARE_AVX_SPECIFIC_CODE (__VA_ARGS__) \ DECLARE_AVX2_SPECIFIC_CODE (__VA_ARGS__) \ DECLARE_AVX512F_SPECIFIC_CODE(__VA_ARGS__) \ -DECLARE_AVX512BW_SPECIFIC_CODE (__VA_ARGS__) \ -DECLARE_AVX512VBMI_SPECIFIC_CODE (__VA_ARGS__) \ -DECLARE_AVX512VBMI2_SPECIFIC_CODE (__VA_ARGS__) +DECLARE_AVX512BW_SPECIFIC_CODE(__VA_ARGS__) \ +DECLARE_AVX512VBMI_SPECIFIC_CODE(__VA_ARGS__) DECLARE_DEFAULT_CODE( constexpr auto BuildArch = TargetArch::Default; /// NOLINT @@ -296,9 +276,6 @@ DECLARE_AVX512VBMI_SPECIFIC_CODE( constexpr auto BuildArch = TargetArch::AVX512VBMI; /// NOLINT ) // DECLARE_AVX512VBMI_SPECIFIC_CODE -DECLARE_AVX512VBMI2_SPECIFIC_CODE( - constexpr auto BuildArch = TargetArch::AVX512VBMI2; /// NOLINT -) // DECLARE_AVX512VBMI2_SPECIFIC_CODE /** Runtime Dispatch helpers for class members. * From aabcfea5ede60766d0f601ec9881a0fbdc36d123 Mon Sep 17 00:00:00 2001 From: Alexander Tokmakov Date: Mon, 26 Sep 2022 13:41:27 +0300 Subject: [PATCH 52/54] Update 02354_annoy.sql --- tests/queries/0_stateless/02354_annoy.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/02354_annoy.sql b/tests/queries/0_stateless/02354_annoy.sql index d25b7333a89..8a8d023a104 100644 --- a/tests/queries/0_stateless/02354_annoy.sql +++ b/tests/queries/0_stateless/02354_annoy.sql @@ -1,4 +1,4 @@ --- Tags: no-fasttest, no-ubsan, no-cpu-aarch64 +-- Tags: no-fasttest, no-ubsan, no-cpu-aarch64, no-backward-compatibility-check SET allow_experimental_annoy_index = 1; From a760c71a0beea2aa8b2bffc29b9c8dabcf1638aa Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Mon, 26 Sep 2022 12:52:12 +0200 Subject: [PATCH 53/54] Fix the typo preventing building latest images --- tests/ci/docker_images_check.py | 2 +- tests/ci/pr_info.py | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/ci/docker_images_check.py b/tests/ci/docker_images_check.py index 773f3ac1b57..fb7228628fd 100644 --- a/tests/ci/docker_images_check.py +++ b/tests/ci/docker_images_check.py @@ -164,7 +164,7 @@ def gen_versions( # The order is important, PR number is used as cache during the build versions = [str(pr_info.number), pr_commit_version] result_version = pr_commit_version - if pr_info.number == 0 and pr_info.base_name == "master": + if pr_info.number == 0 and pr_info.base_ref == "master": # First get the latest for cache versions.insert(0, "latest") diff --git a/tests/ci/pr_info.py b/tests/ci/pr_info.py index 77421ddac32..dc016a7eed9 100644 --- a/tests/ci/pr_info.py +++ b/tests/ci/pr_info.py @@ -132,9 +132,13 @@ class PRInfo: self.commit_html_url = f"{repo_prefix}/commits/{self.sha}" self.pr_html_url = f"{repo_prefix}/pull/{self.number}" + # master or backport/xx.x/xxxxx - where the PR will be merged self.base_ref = github_event["pull_request"]["base"]["ref"] + # ClickHouse/ClickHouse self.base_name = github_event["pull_request"]["base"]["repo"]["full_name"] + # any_branch-name - the name of working branch name self.head_ref = github_event["pull_request"]["head"]["ref"] + # UserName/ClickHouse or ClickHouse/ClickHouse self.head_name = github_event["pull_request"]["head"]["repo"]["full_name"] self.body = github_event["pull_request"]["body"] self.labels = { From 51c9f81dce1a43b0fea346018fca46b191e10aa2 Mon Sep 17 00:00:00 2001 From: "Mikhail f. Shiryaev" Date: Mon, 26 Sep 2022 13:58:38 +0200 Subject: [PATCH 54/54] Fix tests for docker-ci --- tests/ci/docker_test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ci/docker_test.py b/tests/ci/docker_test.py index 740cae5bc97..1848300e2f6 100644 --- a/tests/ci/docker_test.py +++ b/tests/ci/docker_test.py @@ -99,11 +99,11 @@ class TestDockerImageCheck(unittest.TestCase): def test_gen_version(self): pr_info = PRInfo(PRInfo.default_event.copy()) - pr_info.base_name = "anything-else" + pr_info.base_ref = "anything-else" versions, result_version = di.gen_versions(pr_info, None) self.assertEqual(versions, ["0", "0-HEAD"]) self.assertEqual(result_version, "0-HEAD") - pr_info.base_name = "master" + pr_info.base_ref = "master" versions, result_version = di.gen_versions(pr_info, None) self.assertEqual(versions, ["latest", "0", "0-HEAD"]) self.assertEqual(result_version, "0-HEAD")