From 9cb78214fa2e3af2f29738aaddb4108520cae4e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Mon, 11 Nov 2024 14:21:45 +0100 Subject: [PATCH 01/91] Add some missing tiers --- src/Core/Settings.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Core/Settings.cpp b/src/Core/Settings.cpp index 4c8761e503e..3696ca27ac1 100644 --- a/src/Core/Settings.cpp +++ b/src/Core/Settings.cpp @@ -4312,7 +4312,7 @@ Disable limit on kafka_num_consumers that depends on the number of available CPU )", 0) \ DECLARE(Bool, allow_experimental_kafka_offsets_storage_in_keeper, false, R"( Allow experimental feature to store Kafka related offsets in ClickHouse Keeper. When enabled a ClickHouse Keeper path and replica name can be specified to the Kafka table engine. As a result instead of the regular Kafka engine, a new type of storage engine will be used that stores the committed offsets primarily in ClickHouse Keeper -)", 0) \ +)", EXPERIMENTAL) \ DECLARE(Bool, enable_software_prefetch_in_aggregation, true, R"( Enable use of software prefetch in aggregation )", 0) \ @@ -5813,7 +5813,7 @@ If it is set to true, allow to use experimental full-text index. \ DECLARE(Bool, allow_experimental_join_condition, false, R"( Support join with inequal conditions which involve columns from both left and right table. e.g. t1.y < t2.y. -)", 0) \ +)", EXPERIMENTAL) \ \ DECLARE(Bool, allow_experimental_live_view, false, R"( Allows creation of a deprecated LIVE VIEW. @@ -5822,7 +5822,7 @@ Possible values: - 0 — Working with live views is disabled. - 1 — Working with live views is enabled. -)", 0) \ +)", EXPERIMENTAL) \ DECLARE(Seconds, live_view_heartbeat_interval, 15, R"( The heartbeat interval in seconds to indicate live query is alive. )", EXPERIMENTAL) \ From 356393b4888442e38ed6089633fd8b8fa8cd75e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Mon, 11 Nov 2024 14:34:53 +0100 Subject: [PATCH 02/91] Some typos --- src/Access/SettingsConstraints.cpp | 2 +- src/Access/SettingsConstraints.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Access/SettingsConstraints.cpp b/src/Access/SettingsConstraints.cpp index cdf3dac192e..7ab0cf8cc26 100644 --- a/src/Access/SettingsConstraints.cpp +++ b/src/Access/SettingsConstraints.cpp @@ -397,7 +397,7 @@ SettingsConstraints::Checker SettingsConstraints::getChecker(const Settings & cu /** The `readonly` value is understood as follows: * 0 - no read-only restrictions. - * 1 - only read requests, as well as changing settings with `changable_in_readonly` flag. + * 1 - only read requests, as well as changing settings with `changeable_in_readonly` flag. * 2 - only read requests, as well as changing settings, except for the `readonly` setting. */ diff --git a/src/Access/SettingsConstraints.h b/src/Access/SettingsConstraints.h index 5bbff86ff61..f5e4335252c 100644 --- a/src/Access/SettingsConstraints.h +++ b/src/Access/SettingsConstraints.h @@ -40,7 +40,7 @@ class AccessControl; * * * - * + * * * * @@ -50,7 +50,7 @@ class AccessControl; * If a setting cannot be change due to the read-only mode this class throws an exception. * The value of `readonly` is understood as follows: * 0 - not read-only mode, no additional checks. - * 1 - only read queries, as well as changing settings with flag. + * 1 - only read queries, as well as changing settings with flag. * 2 - only read queries and you can change the settings, except for the `readonly` setting. * */ From 5efbae615c4c5d8e67a50e1be86a1c1385a8cc68 Mon Sep 17 00:00:00 2001 From: avogar Date: Mon, 11 Nov 2024 15:07:38 +0000 Subject: [PATCH 03/91] Forbid Dynamic type in min/max functions to avoid confusion --- .../AggregateFunctionsArgMinArgMax.cpp | 8 ++++++++ src/AggregateFunctions/AggregateFunctionsMinMax.cpp | 8 ++++++++ .../AggregateFunctionCombinatorsArgMinArgMax.cpp | 8 ++++++++ src/Storages/MergeTree/MergeTreeIndexMinMax.cpp | 8 ++++++++ .../0_stateless/03271_dynamic_in_min_max.reference | 0 tests/queries/0_stateless/03271_dynamic_in_min_max.sql | 9 +++++++++ 6 files changed, 41 insertions(+) create mode 100644 tests/queries/0_stateless/03271_dynamic_in_min_max.reference create mode 100644 tests/queries/0_stateless/03271_dynamic_in_min_max.sql diff --git a/src/AggregateFunctions/AggregateFunctionsArgMinArgMax.cpp b/src/AggregateFunctions/AggregateFunctionsArgMinArgMax.cpp index 9608ca26f37..cc72a26af16 100644 --- a/src/AggregateFunctions/AggregateFunctionsArgMinArgMax.cpp +++ b/src/AggregateFunctions/AggregateFunctionsArgMinArgMax.cpp @@ -79,6 +79,14 @@ public: "Illegal type {} of second argument of aggregate function {} because the values of that data type are not comparable", type_val->getName(), getName()); + + if (isDynamic(this->type_val) || isVariant(this->type_val)) + throw Exception( + ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT, + "Illegal type {} of argument of aggregate function {} because the column of that type can contain values with different " + "data types. Consider using typed subcolumns or cast column to a specific data type", + this->type_val->getName(), + getName()); } void create(AggregateDataPtr __restrict place) const override /// NOLINT diff --git a/src/AggregateFunctions/AggregateFunctionsMinMax.cpp b/src/AggregateFunctions/AggregateFunctionsMinMax.cpp index 5fa9a4ff5d1..0c21be574c4 100644 --- a/src/AggregateFunctions/AggregateFunctionsMinMax.cpp +++ b/src/AggregateFunctions/AggregateFunctionsMinMax.cpp @@ -35,6 +35,14 @@ public: "Illegal type {} of argument of aggregate function {} because the values of that data type are not comparable", this->result_type->getName(), getName()); + + if (isDynamic(this->result_type) || isVariant(this->result_type)) + throw Exception( + ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT, + "Illegal type {} of argument of aggregate function {} because the column of that type can contain values with different " + "data types. Consider using typed subcolumns or cast column to a specific data type", + this->result_type->getName(), + getName()); } String getName() const override diff --git a/src/AggregateFunctions/Combinators/AggregateFunctionCombinatorsArgMinArgMax.cpp b/src/AggregateFunctions/Combinators/AggregateFunctionCombinatorsArgMinArgMax.cpp index a1716f18725..fc8b5b6d4cd 100644 --- a/src/AggregateFunctions/Combinators/AggregateFunctionCombinatorsArgMinArgMax.cpp +++ b/src/AggregateFunctions/Combinators/AggregateFunctionCombinatorsArgMinArgMax.cpp @@ -63,6 +63,14 @@ public: "Illegal type {} for combinator {} because the values of that data type are not comparable", arguments[key_col]->getName(), getName()); + + if (isDynamic(arguments[key_col]) || isVariant(arguments[key_col])) + throw Exception( + ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT, + "Illegal type {} of argument of aggregate function {} because the column of that type can contain values with different " + "data types. Consider using typed subcolumns or cast column to a specific data type", + arguments[key_col]->getName(), + getName()); } String getName() const override diff --git a/src/Storages/MergeTree/MergeTreeIndexMinMax.cpp b/src/Storages/MergeTree/MergeTreeIndexMinMax.cpp index 07fd873a000..c859053b976 100644 --- a/src/Storages/MergeTree/MergeTreeIndexMinMax.cpp +++ b/src/Storages/MergeTree/MergeTreeIndexMinMax.cpp @@ -231,6 +231,14 @@ void minmaxIndexValidator(const IndexDescription & index, bool attach) "Data type of argument for minmax index must be comparable, got {} type for column {} instead", column.type->getName(), column.name); } + + if (isDynamic(column.type) || isVariant(column.type)) + { + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "{} data type of column {} is not allowed in minmax index because the column of that type can contain values with different data " + "types. Consider using typed subcolumns or cast column to a specific data type", + column.type->getName(), column.name); + } } } diff --git a/tests/queries/0_stateless/03271_dynamic_in_min_max.reference b/tests/queries/0_stateless/03271_dynamic_in_min_max.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/queries/0_stateless/03271_dynamic_in_min_max.sql b/tests/queries/0_stateless/03271_dynamic_in_min_max.sql new file mode 100644 index 00000000000..77ad9f0dc93 --- /dev/null +++ b/tests/queries/0_stateless/03271_dynamic_in_min_max.sql @@ -0,0 +1,9 @@ +set allow_experimental_dynamic_type=1; +select max(number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select min(number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select argMax(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select argMin(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select anyArgMax(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select anyArgMin(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +create table test (d Dynamic, index idx d type minmax); -- {serverError BAD_ARGUMENTS} + From b62aac446e55bea82278a72a64d64b6001378993 Mon Sep 17 00:00:00 2001 From: avogar Date: Mon, 11 Nov 2024 15:11:55 +0000 Subject: [PATCH 04/91] Better tests --- .../03271_dynamic_variant_in_min_max.reference | 0 .../03271_dynamic_variant_in_min_max.sql | 18 ++++++++++++++++++ 2 files changed, 18 insertions(+) create mode 100644 tests/queries/0_stateless/03271_dynamic_variant_in_min_max.reference create mode 100644 tests/queries/0_stateless/03271_dynamic_variant_in_min_max.sql diff --git a/tests/queries/0_stateless/03271_dynamic_variant_in_min_max.reference b/tests/queries/0_stateless/03271_dynamic_variant_in_min_max.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/queries/0_stateless/03271_dynamic_variant_in_min_max.sql b/tests/queries/0_stateless/03271_dynamic_variant_in_min_max.sql new file mode 100644 index 00000000000..a7fea941548 --- /dev/null +++ b/tests/queries/0_stateless/03271_dynamic_variant_in_min_max.sql @@ -0,0 +1,18 @@ +set allow_experimental_dynamic_type=1; +select max(number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select min(number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select argMax(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select argMin(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select anyArgMax(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select anyArgMin(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +create table test (d Dynamic, index idx d type minmax); -- {serverError BAD_ARGUMENTS} + +set allow_experimental_variant_type=1; +select max(number::Variant(UInt64)) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select min(number::Variant(UInt64)) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select argMax(number, number::Variant(UInt64)) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select argMin(number, number::Variant(UInt64)) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select anyArgMax(number, number::Variant(UInt64)) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select anyArgMin(number, number::Variant(UInt64)) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +create table test (d Variant(UInt64), index idx d type minmax); -- {serverError BAD_ARGUMENTS} + From 75cbf0ca9a832b4b3d8950f544ca770ecfd27d24 Mon Sep 17 00:00:00 2001 From: avogar Date: Mon, 11 Nov 2024 15:12:30 +0000 Subject: [PATCH 05/91] Remove old test --- .../0_stateless/03271_dynamic_in_min_max.reference | 0 tests/queries/0_stateless/03271_dynamic_in_min_max.sql | 9 --------- 2 files changed, 9 deletions(-) delete mode 100644 tests/queries/0_stateless/03271_dynamic_in_min_max.reference delete mode 100644 tests/queries/0_stateless/03271_dynamic_in_min_max.sql diff --git a/tests/queries/0_stateless/03271_dynamic_in_min_max.reference b/tests/queries/0_stateless/03271_dynamic_in_min_max.reference deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/tests/queries/0_stateless/03271_dynamic_in_min_max.sql b/tests/queries/0_stateless/03271_dynamic_in_min_max.sql deleted file mode 100644 index 77ad9f0dc93..00000000000 --- a/tests/queries/0_stateless/03271_dynamic_in_min_max.sql +++ /dev/null @@ -1,9 +0,0 @@ -set allow_experimental_dynamic_type=1; -select max(number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} -select min(number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} -select argMax(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} -select argMin(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} -select anyArgMax(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} -select anyArgMin(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} -create table test (d Dynamic, index idx d type minmax); -- {serverError BAD_ARGUMENTS} - From 237a81ff8c9ae0f0ef90e05a09e4b36a1a38487d Mon Sep 17 00:00:00 2001 From: avogar Date: Mon, 11 Nov 2024 21:11:31 +0000 Subject: [PATCH 06/91] Slightly better JSON type parsing --- src/Formats/JSONExtractTree.cpp | 3 ++- .../03273_better_json_subcolumns_parsing.reference | 3 +++ .../0_stateless/03273_better_json_subcolumns_parsing.sql | 8 ++++++++ 3 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 tests/queries/0_stateless/03273_better_json_subcolumns_parsing.reference create mode 100644 tests/queries/0_stateless/03273_better_json_subcolumns_parsing.sql diff --git a/src/Formats/JSONExtractTree.cpp b/src/Formats/JSONExtractTree.cpp index ae6051823b7..fb078fdb352 100644 --- a/src/Formats/JSONExtractTree.cpp +++ b/src/Formats/JSONExtractTree.cpp @@ -1442,7 +1442,8 @@ public: auto shared_variant_discr = column_dynamic.getSharedVariantDiscriminator(); auto insert_settings_with_no_type_conversion = insert_settings; insert_settings_with_no_type_conversion.allow_type_conversion = false; - for (size_t i = 0; i != variant_info.variant_names.size(); ++i) + auto order = SerializationVariant::getVariantsDeserializeTextOrder(assert_cast(*variant_info.variant_type).getVariants()); + for (size_t i : order) { if (i != shared_variant_discr) { diff --git a/tests/queries/0_stateless/03273_better_json_subcolumns_parsing.reference b/tests/queries/0_stateless/03273_better_json_subcolumns_parsing.reference new file mode 100644 index 00000000000..ce734e472f4 --- /dev/null +++ b/tests/queries/0_stateless/03273_better_json_subcolumns_parsing.reference @@ -0,0 +1,3 @@ +Int64 42 +Float64 42.42 +Int64 43 diff --git a/tests/queries/0_stateless/03273_better_json_subcolumns_parsing.sql b/tests/queries/0_stateless/03273_better_json_subcolumns_parsing.sql new file mode 100644 index 00000000000..0f6b51d8987 --- /dev/null +++ b/tests/queries/0_stateless/03273_better_json_subcolumns_parsing.sql @@ -0,0 +1,8 @@ +set allow_experimental_json_type = 1; +drop table if exists test; +create table test (json JSON) engine=Memory; +insert into test format JSONAsObject {"a" : 42}, {"a" : 42.42}, {"a" : 43}; + +select dynamicType(json.a), json.a from test; +drop table test; + From 3dae5def485d0bdf9b0bdb3444db5eee7358890d Mon Sep 17 00:00:00 2001 From: Pavel Kruglov <48961922+Avogar@users.noreply.github.com> Date: Tue, 12 Nov 2024 16:25:17 +0100 Subject: [PATCH 07/91] Update 01825_new_type_json_12.reference --- tests/queries/0_stateless/01825_new_type_json_12.reference | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/01825_new_type_json_12.reference b/tests/queries/0_stateless/01825_new_type_json_12.reference index 10770498c4a..36f014f0178 100644 --- a/tests/queries/0_stateless/01825_new_type_json_12.reference +++ b/tests/queries/0_stateless/01825_new_type_json_12.reference @@ -9,5 +9,5 @@ ('key_6','String') ('key_7','Float64') ('key_7','Int64') -{"obj":{"id":"1","key_0":[{"key_1":[{"key_3":[{"key_4":"1048576","key_5":0.0001048576,"key_6":25.5,"key_7":"1025"},{"key_6":"","key_7":"2"}]}]},{},{"key_1":[{"key_3":[{"key_5":-1,"key_6":"aqbjfiruu","key_7":-922337203685477600},{"key_4":"","key_6":"","key_7":65537}]},{"key_3":[{"key_4":"ghdqyeiom","key_5":1048575,"key_7":21474836.48}]}]}]}} +{"obj":{"id":"1","key_0":[{"key_1":[{"key_3":[{"key_4":"1048576","key_5":0.0001048576,"key_6":25.5,"key_7":"1025"},{"key_6":"","key_7":"2"}]}]},{},{"key_1":[{"key_3":[{"key_5":-1,"key_6":"aqbjfiruu","key_7":-922337203685477600},{"key_4":"","key_6":"","key_7":"65537"}]},{"key_3":[{"key_4":"ghdqyeiom","key_5":1048575,"key_7":21474836.48}]}]}]}} [[[1048576,NULL]],[],[[NULL,''],['ghdqyeiom']]] [[[0.0001048576,NULL]],[],[[-1,NULL],[1048575]]] [[[25.5,'']],[],[['aqbjfiruu',''],[NULL]]] [[[1025,2]],[],[[-922337203685477600,65537],[21474836.48]]] From 6ad72f1b8fe30a21a75139d7ddb0f22183ea821b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Tue, 12 Nov 2024 16:56:01 +0100 Subject: [PATCH 08/91] Implement ServerSetting 'allowed_feature_tier' --- programs/server/Server.cpp | 2 + src/Access/AccessControl.cpp | 24 +++++++++++ src/Access/AccessControl.h | 7 ++++ src/Access/SettingsConstraints.cpp | 22 ++++++++++ src/Core/BaseSettings.h | 13 ++++++ src/Core/ServerSettings.cpp | 65 +++++++++++++++++------------- src/Core/Settings.cpp | 5 +++ src/Core/Settings.h | 2 + 8 files changed, 112 insertions(+), 28 deletions(-) diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index 68f262079ff..9c25d50c3c4 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -166,6 +166,7 @@ namespace MergeTreeSetting namespace ServerSetting { + extern const ServerSettingsUInt32 allowed_feature_tier; extern const ServerSettingsUInt32 asynchronous_heavy_metrics_update_period_s; extern const ServerSettingsUInt32 asynchronous_metrics_update_period_s; extern const ServerSettingsBool asynchronous_metrics_enable_heavy_metrics; @@ -1771,6 +1772,7 @@ try global_context->setMaxDictionaryNumToWarn(new_server_settings[ServerSetting::max_dictionary_num_to_warn]); global_context->setMaxDatabaseNumToWarn(new_server_settings[ServerSetting::max_database_num_to_warn]); global_context->setMaxPartNumToWarn(new_server_settings[ServerSetting::max_part_num_to_warn]); + global_context->getAccessControl().setAllowTierSettings(new_server_settings[ServerSetting::allowed_feature_tier]); /// Only for system.server_settings global_context->setConfigReloaderInterval(new_server_settings[ServerSetting::config_reload_interval_ms]); diff --git a/src/Access/AccessControl.cpp b/src/Access/AccessControl.cpp index 9b3b8d2a977..1583ccecf94 100644 --- a/src/Access/AccessControl.cpp +++ b/src/Access/AccessControl.cpp @@ -876,4 +876,28 @@ void AccessControl::allowAllSettings() custom_settings_prefixes->registerPrefixes({""}); } +void AccessControl::setAllowTierSettings(UInt32 value) +{ + allow_experimental_tier_settings = value == 0; + allow_beta_tier_settings = value <= 1; +} + +UInt32 AccessControl::getAllowTierSettings() const +{ + if (allow_experimental_tier_settings) + return 0; + if (allow_beta_tier_settings) + return 1; + return 2; +} + +bool AccessControl::getAllowExperimentalTierSettings() const +{ + return allow_experimental_tier_settings; +} + +bool AccessControl::getAllowBetaTierSettings() const +{ + return allow_beta_tier_settings; +} } diff --git a/src/Access/AccessControl.h b/src/Access/AccessControl.h index a342c5300bf..1fbe7a3fccd 100644 --- a/src/Access/AccessControl.h +++ b/src/Access/AccessControl.h @@ -243,6 +243,11 @@ public: /// Allow all setting names - this can be used in clients to pass-through unknown settings to the server. void allowAllSettings(); + void setAllowTierSettings(UInt32 value); + UInt32 getAllowTierSettings() const; + bool getAllowExperimentalTierSettings() const; + bool getAllowBetaTierSettings() const; + private: class ContextAccessCache; class CustomSettingsPrefixes; @@ -272,6 +277,8 @@ private: std::atomic_bool table_engines_require_grant = false; std::atomic_int bcrypt_workfactor = 12; std::atomic default_password_type = AuthenticationType::SHA256_PASSWORD; + std::atomic_bool allow_experimental_tier_settings = true; + std::atomic_bool allow_beta_tier_settings = true; }; } diff --git a/src/Access/SettingsConstraints.cpp b/src/Access/SettingsConstraints.cpp index 7ab0cf8cc26..cb1d433766a 100644 --- a/src/Access/SettingsConstraints.cpp +++ b/src/Access/SettingsConstraints.cpp @@ -404,6 +404,28 @@ SettingsConstraints::Checker SettingsConstraints::getChecker(const Settings & cu if (current_settings[Setting::readonly] > 1 && resolved_name == "readonly") return Checker(PreformattedMessage::create("Cannot modify 'readonly' setting in readonly mode"), ErrorCodes::READONLY); + if (access_control) + { + bool allowed_experimental = access_control->getAllowExperimentalTierSettings(); + bool allowed_beta = access_control->getAllowBetaTierSettings(); + if (!allowed_experimental || !allowed_beta) + { + auto setting_tier = current_settings.getTier(resolved_name); + if (setting_tier == SettingsTierType::EXPERIMENTAL && !allowed_experimental) + return Checker( + PreformattedMessage::create( + "Cannot modify setting '{}'. Changes to EXPERIMENTAL settings are disabled in the server config ('allowed_feature_tier')", + setting_name), + ErrorCodes::READONLY); + if (setting_tier == SettingsTierType::BETA && !allowed_beta) + return Checker( + PreformattedMessage::create( + "Cannot modify setting '{}'. Changes to BETA settings are disabled in the server config ('allowed_feature_tier')", + setting_name), + ErrorCodes::READONLY); + } + } + auto it = constraints.find(resolved_name); if (current_settings[Setting::readonly] == 1) { diff --git a/src/Core/BaseSettings.h b/src/Core/BaseSettings.h index 949b884636f..201b586f067 100644 --- a/src/Core/BaseSettings.h +++ b/src/Core/BaseSettings.h @@ -131,6 +131,7 @@ public: const char * getTypeName(std::string_view name) const; const char * getDescription(std::string_view name) const; + SettingsTierType getTier(std::string_view name) const; /// Checks if it's possible to assign a field to a specified value and throws an exception if not. /// This function doesn't change the fields, it performs check only. @@ -380,6 +381,18 @@ const char * BaseSettings::getDescription(std::string_view name) const BaseSettingsHelpers::throwSettingNotFound(name); } +template +SettingsTierType BaseSettings::getTier(std::string_view name) const +{ + name = TTraits::resolveName(name); + const auto & accessor = Traits::Accessor::instance(); + if (size_t index = accessor.find(name); index != static_cast(-1)) + return accessor.getTier(index); + if (tryGetCustomSetting(name)) + return SettingsTierType::PRODUCTION; + BaseSettingsHelpers::throwSettingNotFound(name); +} + template void BaseSettings::checkCanSet(std::string_view name, const Field & value) { diff --git a/src/Core/ServerSettings.cpp b/src/Core/ServerSettings.cpp index d573377fc8b..9c51ea75e51 100644 --- a/src/Core/ServerSettings.cpp +++ b/src/Core/ServerSettings.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -26,6 +27,8 @@ extern const Metric BackgroundMessageBrokerSchedulePoolSize; namespace DB { +// clang-format off + #define LIST_OF_SERVER_SETTINGS(DECLARE, ALIAS) \ DECLARE(Bool, show_addresses_in_stack_traces, true, "If it is set true will show addresses in stack traces", 0) \ DECLARE(Bool, shutdown_wait_unfinished_queries, false, "If set true ClickHouse will wait for running queries finish before shutdown.", 0) \ @@ -198,7 +201,11 @@ namespace DB DECLARE(UInt64, load_marks_threadpool_pool_size, 50, "Size of background pool for marks loading", 0) \ DECLARE(UInt64, load_marks_threadpool_queue_size, 1000000, "Number of tasks which is possible to push into prefetches pool", 0) \ DECLARE(UInt64, threadpool_writer_pool_size, 100, "Size of background pool for write requests to object storages", 0) \ - DECLARE(UInt64, threadpool_writer_queue_size, 1000000, "Number of tasks which is possible to push into background pool for write requests to object storages", 0) + DECLARE(UInt64, threadpool_writer_queue_size, 1000000, "Number of tasks which is possible to push into background pool for write requests to object storages", 0) \ + DECLARE(UInt32, allowed_feature_tier, 0, "0 - All feature tiers allowed (experimental, beta, production). 1 - Only beta and production feature tiers allowed. 2 - Only production feature tier allowed", 0) \ + + +// clang-format on /// If you add a setting which can be updated at runtime, please update 'changeable_settings' map in dumpToSystemServerSettingsColumns below @@ -281,40 +288,42 @@ void ServerSettings::dumpToSystemServerSettingsColumns(ServerSettingColumnsParam /// with new the setting values but the settings themselves are not stored between re-loads. As a result, if one wants to know the /// current setting values, one needs to ask the components directly. std::unordered_map> changeable_settings - = {{"max_server_memory_usage", {std::to_string(total_memory_tracker.getHardLimit()), ChangeableWithoutRestart::Yes}}, + = { + {"max_server_memory_usage", {std::to_string(total_memory_tracker.getHardLimit()), ChangeableWithoutRestart::Yes}}, - {"max_table_size_to_drop", {std::to_string(context->getMaxTableSizeToDrop()), ChangeableWithoutRestart::Yes}}, - {"max_partition_size_to_drop", {std::to_string(context->getMaxPartitionSizeToDrop()), ChangeableWithoutRestart::Yes}}, + {"max_table_size_to_drop", {std::to_string(context->getMaxTableSizeToDrop()), ChangeableWithoutRestart::Yes}}, + {"max_partition_size_to_drop", {std::to_string(context->getMaxPartitionSizeToDrop()), ChangeableWithoutRestart::Yes}}, - {"max_concurrent_queries", {std::to_string(context->getProcessList().getMaxSize()), ChangeableWithoutRestart::Yes}}, - {"max_concurrent_insert_queries", + {"max_concurrent_queries", {std::to_string(context->getProcessList().getMaxSize()), ChangeableWithoutRestart::Yes}}, + {"max_concurrent_insert_queries", {std::to_string(context->getProcessList().getMaxInsertQueriesAmount()), ChangeableWithoutRestart::Yes}}, - {"max_concurrent_select_queries", + {"max_concurrent_select_queries", {std::to_string(context->getProcessList().getMaxSelectQueriesAmount()), ChangeableWithoutRestart::Yes}}, - {"max_waiting_queries", {std::to_string(context->getProcessList().getMaxWaitingQueriesAmount()), ChangeableWithoutRestart::Yes}}, + {"max_waiting_queries", {std::to_string(context->getProcessList().getMaxWaitingQueriesAmount()), ChangeableWithoutRestart::Yes}}, - {"background_buffer_flush_schedule_pool_size", - {std::to_string(CurrentMetrics::get(CurrentMetrics::BackgroundBufferFlushSchedulePoolSize)), - ChangeableWithoutRestart::IncreaseOnly}}, - {"background_schedule_pool_size", - {std::to_string(CurrentMetrics::get(CurrentMetrics::BackgroundSchedulePoolSize)), ChangeableWithoutRestart::IncreaseOnly}}, - {"background_message_broker_schedule_pool_size", - {std::to_string(CurrentMetrics::get(CurrentMetrics::BackgroundMessageBrokerSchedulePoolSize)), - ChangeableWithoutRestart::IncreaseOnly}}, - {"background_distributed_schedule_pool_size", - {std::to_string(CurrentMetrics::get(CurrentMetrics::BackgroundDistributedSchedulePoolSize)), - ChangeableWithoutRestart::IncreaseOnly}}, + {"background_buffer_flush_schedule_pool_size", + {std::to_string(CurrentMetrics::get(CurrentMetrics::BackgroundBufferFlushSchedulePoolSize)), ChangeableWithoutRestart::IncreaseOnly}}, + {"background_schedule_pool_size", + {std::to_string(CurrentMetrics::get(CurrentMetrics::BackgroundSchedulePoolSize)), ChangeableWithoutRestart::IncreaseOnly}}, + {"background_message_broker_schedule_pool_size", + {std::to_string(CurrentMetrics::get(CurrentMetrics::BackgroundMessageBrokerSchedulePoolSize)), ChangeableWithoutRestart::IncreaseOnly}}, + {"background_distributed_schedule_pool_size", + {std::to_string(CurrentMetrics::get(CurrentMetrics::BackgroundDistributedSchedulePoolSize)), ChangeableWithoutRestart::IncreaseOnly}}, - {"mark_cache_size", {std::to_string(context->getMarkCache()->maxSizeInBytes()), ChangeableWithoutRestart::Yes}}, - {"uncompressed_cache_size", {std::to_string(context->getUncompressedCache()->maxSizeInBytes()), ChangeableWithoutRestart::Yes}}, - {"index_mark_cache_size", {std::to_string(context->getIndexMarkCache()->maxSizeInBytes()), ChangeableWithoutRestart::Yes}}, - {"index_uncompressed_cache_size", - {std::to_string(context->getIndexUncompressedCache()->maxSizeInBytes()), ChangeableWithoutRestart::Yes}}, - {"mmap_cache_size", {std::to_string(context->getMMappedFileCache()->maxSizeInBytes()), ChangeableWithoutRestart::Yes}}, + {"mark_cache_size", {std::to_string(context->getMarkCache()->maxSizeInBytes()), ChangeableWithoutRestart::Yes}}, + {"uncompressed_cache_size", {std::to_string(context->getUncompressedCache()->maxSizeInBytes()), ChangeableWithoutRestart::Yes}}, + {"index_mark_cache_size", {std::to_string(context->getIndexMarkCache()->maxSizeInBytes()), ChangeableWithoutRestart::Yes}}, + {"index_uncompressed_cache_size", + {std::to_string(context->getIndexUncompressedCache()->maxSizeInBytes()), ChangeableWithoutRestart::Yes}}, + {"mmap_cache_size", {std::to_string(context->getMMappedFileCache()->maxSizeInBytes()), ChangeableWithoutRestart::Yes}}, - {"merge_workload", {context->getMergeWorkload(), ChangeableWithoutRestart::Yes}}, - {"mutation_workload", {context->getMutationWorkload(), ChangeableWithoutRestart::Yes}}, - {"config_reload_interval_ms", {std::to_string(context->getConfigReloaderInterval()), ChangeableWithoutRestart::Yes}}}; + {"merge_workload", {context->getMergeWorkload(), ChangeableWithoutRestart::Yes}}, + {"mutation_workload", {context->getMutationWorkload(), ChangeableWithoutRestart::Yes}}, + {"config_reload_interval_ms", {std::to_string(context->getConfigReloaderInterval()), ChangeableWithoutRestart::Yes}}, + + {"allowed_feature_tier", + {std::to_string(context->getAccessControl().getAllowTierSettings()), ChangeableWithoutRestart::Yes}}, + }; if (context->areBackgroundExecutorsInitialized()) { diff --git a/src/Core/Settings.cpp b/src/Core/Settings.cpp index 3696ca27ac1..ec2c457b904 100644 --- a/src/Core/Settings.cpp +++ b/src/Core/Settings.cpp @@ -6176,6 +6176,11 @@ bool Settings::isChanged(std::string_view name) const return impl->isChanged(name); } +SettingsTierType Settings::getTier(std::string_view name) const +{ + return impl->getTier(name); +} + bool Settings::tryGet(std::string_view name, Field & value) const { return impl->tryGet(name, value); diff --git a/src/Core/Settings.h b/src/Core/Settings.h index ac3b1fe651e..b66f4403ddf 100644 --- a/src/Core/Settings.h +++ b/src/Core/Settings.h @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -117,6 +118,7 @@ struct Settings /// General API as needed bool has(std::string_view name) const; bool isChanged(std::string_view name) const; + SettingsTierType getTier(std::string_view name) const; bool tryGet(std::string_view name, Field & value) const; Field get(std::string_view name) const; From cd269f82d3fe34d1e63a2ae885b79762e0148511 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Tue, 12 Nov 2024 19:39:01 +0100 Subject: [PATCH 09/91] Missing experimental tag --- src/Core/Settings.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Core/Settings.cpp b/src/Core/Settings.cpp index ec2c457b904..aeb13a73f34 100644 --- a/src/Core/Settings.cpp +++ b/src/Core/Settings.cpp @@ -5740,7 +5740,7 @@ Possible values: - 0 — the [TimeSeries](../../engines/table-engines/integrations/time-series.md) table engine is disabled. - 1 — the [TimeSeries](../../engines/table-engines/integrations/time-series.md) table engine is enabled. -)", 0) \ +)", EXPERIMENTAL) \ DECLARE(Bool, allow_experimental_vector_similarity_index, false, R"( Allow experimental vector similarity index )", EXPERIMENTAL) \ From cd97103526675874110d0c298f8a99852eb56227 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Tue, 12 Nov 2024 20:23:26 +0100 Subject: [PATCH 10/91] Allow changes in default profile --- .../server-configuration-parameters/settings.md | 14 ++++++++++++++ src/Interpreters/Context.cpp | 7 ++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/docs/en/operations/server-configuration-parameters/settings.md b/docs/en/operations/server-configuration-parameters/settings.md index c5f92ccdf68..09318834299 100644 --- a/docs/en/operations/server-configuration-parameters/settings.md +++ b/docs/en/operations/server-configuration-parameters/settings.md @@ -3261,3 +3261,17 @@ Use the legacy MongoDB integration implementation. Deprecated. Type: Bool Default value: `true`. + +## allowed_feature_tier + +Controls if the user can change settings related to the different feature tiers. +0 - Changes to any setting are allowed (experimental, beta, production). +1 - Only changes to beta and production feature settings are allowed. Changes to experimental settings are rejected. +2 - Only changes to production settings are allowed. Changes to experimental or beta settings are rejected. + +This is equivalent to setting a readonly constraint on all EXPERIMENTAL / BETA features. +``` + +Type: UInt32 + +Default value: `0` (all settings can be changed). diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index c1fa2c8549a..fec7b41baa5 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -4970,7 +4970,12 @@ void Context::setDefaultProfiles(const Poco::Util::AbstractConfiguration & confi getAccessControl().setDefaultProfileName(shared->default_profile_name); shared->system_profile_name = config.getString("system_profile", shared->default_profile_name); - setCurrentProfile(shared->system_profile_name); + + /// Don't check for constraints on first load. This makes the default profile consistent with other users, where + /// the default value set in the config might be outside of the constraints range + /// It makes it possible to change the value of experimental settings with `allowed_feature_tier` != 2 + bool check_constraints = false; + setCurrentProfile(shared->system_profile_name, check_constraints); applySettingsQuirks(*settings, getLogger("SettingsQuirks")); doSettingsSanityCheckClamp(*settings, getLogger("SettingsSanity")); From c383a743f793cf471a12089acbc5655e498ff423 Mon Sep 17 00:00:00 2001 From: udiz Date: Wed, 13 Nov 2024 20:02:31 +0000 Subject: [PATCH 11/91] arrayWithConstant size estimation using single value size --- src/Functions/array/arrayWithConstant.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Functions/array/arrayWithConstant.cpp b/src/Functions/array/arrayWithConstant.cpp index 4cbc6404b9b..70d5080d1f9 100644 --- a/src/Functions/array/arrayWithConstant.cpp +++ b/src/Functions/array/arrayWithConstant.cpp @@ -62,16 +62,17 @@ public: for (size_t i = 0; i < num_rows; ++i) { auto array_size = col_num->getInt(i); + auto element_size = col_value->byteSizeAt(i); if (unlikely(array_size < 0)) throw Exception(ErrorCodes::TOO_LARGE_ARRAY_SIZE, "Array size {} cannot be negative: while executing function {}", array_size, getName()); Int64 estimated_size = 0; - if (unlikely(common::mulOverflow(array_size, col_value->byteSize(), estimated_size))) - throw Exception(ErrorCodes::TOO_LARGE_ARRAY_SIZE, "Array size {} with element size {} bytes is too large: while executing function {}", array_size, col_value->byteSize(), getName()); + if (unlikely(common::mulOverflow(array_size, element_size, estimated_size))) + throw Exception(ErrorCodes::TOO_LARGE_ARRAY_SIZE, "Array size {} with element size {} bytes is too large: while executing function {}", array_size, element_size, getName()); if (unlikely(estimated_size > max_array_size_in_columns_bytes)) - throw Exception(ErrorCodes::TOO_LARGE_ARRAY_SIZE, "Array size {} with element size {} bytes is too large: while executing function {}", array_size, col_value->byteSize(), getName()); + throw Exception(ErrorCodes::TOO_LARGE_ARRAY_SIZE, "Array size {} with element size {} bytes is too large: while executing function {}", array_size, element_size, getName()); offset += array_size; From 3145aeda846e5febcba501cec83d038fd081e283 Mon Sep 17 00:00:00 2001 From: Anton Ivashkin Date: Wed, 13 Nov 2024 22:24:37 +0100 Subject: [PATCH 12/91] Cluster autodiscovery with auxiliary keeper test --- .../test_cluster_discovery/config/config.xml | 5 - .../config/config_discovery_path.xml | 9 ++ ...config_discovery_path_auxiliary_keeper.xml | 9 ++ .../config/config_keepers.xml | 24 ++++ .../config/config_observer.xml | 1 - .../config/config_shard1.xml | 1 - .../config/config_shard3.xml | 1 - .../test_cluster_discovery/config/macros0.xml | 6 + .../test_cluster_discovery/config/macros1.xml | 6 + .../test_cluster_discovery/config/macros2.xml | 6 + .../test_cluster_discovery/config/macros3.xml | 6 + .../test_cluster_discovery/config/macros4.xml | 6 + .../config/macros_o.xml | 6 + .../test_cluster_discovery/test.py | 2 +- .../test_auxiliary_keeper.py | 121 ++++++++++++++++++ 15 files changed, 200 insertions(+), 9 deletions(-) create mode 100644 tests/integration/test_cluster_discovery/config/config_discovery_path.xml create mode 100644 tests/integration/test_cluster_discovery/config/config_discovery_path_auxiliary_keeper.xml create mode 100644 tests/integration/test_cluster_discovery/config/config_keepers.xml create mode 100644 tests/integration/test_cluster_discovery/config/macros0.xml create mode 100644 tests/integration/test_cluster_discovery/config/macros1.xml create mode 100644 tests/integration/test_cluster_discovery/config/macros2.xml create mode 100644 tests/integration/test_cluster_discovery/config/macros3.xml create mode 100644 tests/integration/test_cluster_discovery/config/macros4.xml create mode 100644 tests/integration/test_cluster_discovery/config/macros_o.xml create mode 100644 tests/integration/test_cluster_discovery/test_auxiliary_keeper.py diff --git a/tests/integration/test_cluster_discovery/config/config.xml b/tests/integration/test_cluster_discovery/config/config.xml index a63ca3e5438..ed2d3b27259 100644 --- a/tests/integration/test_cluster_discovery/config/config.xml +++ b/tests/integration/test_cluster_discovery/config/config.xml @@ -1,11 +1,6 @@ 1 - - - /clickhouse/discovery/test_auto_cluster - - diff --git a/tests/integration/test_cluster_discovery/config/config_discovery_path.xml b/tests/integration/test_cluster_discovery/config/config_discovery_path.xml new file mode 100644 index 00000000000..8eaecb813ab --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/config_discovery_path.xml @@ -0,0 +1,9 @@ + + + + + /clickhouse/discovery/test_auto_cluster + + + + diff --git a/tests/integration/test_cluster_discovery/config/config_discovery_path_auxiliary_keeper.xml b/tests/integration/test_cluster_discovery/config/config_discovery_path_auxiliary_keeper.xml new file mode 100644 index 00000000000..3e3835dfcab --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/config_discovery_path_auxiliary_keeper.xml @@ -0,0 +1,9 @@ + + + + + zookeeper2:/clickhouse/discovery/test_auto_cluster + + + + diff --git a/tests/integration/test_cluster_discovery/config/config_keepers.xml b/tests/integration/test_cluster_discovery/config/config_keepers.xml new file mode 100644 index 00000000000..4e976578223 --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/config_keepers.xml @@ -0,0 +1,24 @@ + + + + zoo1 + 2181 + + + zoo2 + 2181 + + + zoo3 + 2181 + + + + + + zoo1 + 2181 + + + + diff --git a/tests/integration/test_cluster_discovery/config/config_observer.xml b/tests/integration/test_cluster_discovery/config/config_observer.xml index 84d9539169e..c100c9f5348 100644 --- a/tests/integration/test_cluster_discovery/config/config_observer.xml +++ b/tests/integration/test_cluster_discovery/config/config_observer.xml @@ -3,7 +3,6 @@ - /clickhouse/discovery/test_auto_cluster diff --git a/tests/integration/test_cluster_discovery/config/config_shard1.xml b/tests/integration/test_cluster_discovery/config/config_shard1.xml index 06a77a37263..89590bed607 100644 --- a/tests/integration/test_cluster_discovery/config/config_shard1.xml +++ b/tests/integration/test_cluster_discovery/config/config_shard1.xml @@ -3,7 +3,6 @@ - /clickhouse/discovery/test_auto_cluster 1 diff --git a/tests/integration/test_cluster_discovery/config/config_shard3.xml b/tests/integration/test_cluster_discovery/config/config_shard3.xml index 2c706f7c268..9badd0a6132 100644 --- a/tests/integration/test_cluster_discovery/config/config_shard3.xml +++ b/tests/integration/test_cluster_discovery/config/config_shard3.xml @@ -3,7 +3,6 @@ - /clickhouse/discovery/test_auto_cluster 3 diff --git a/tests/integration/test_cluster_discovery/config/macros0.xml b/tests/integration/test_cluster_discovery/config/macros0.xml new file mode 100644 index 00000000000..a4cabff3005 --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/macros0.xml @@ -0,0 +1,6 @@ + + + shard0 + replica0 + + diff --git a/tests/integration/test_cluster_discovery/config/macros1.xml b/tests/integration/test_cluster_discovery/config/macros1.xml new file mode 100644 index 00000000000..bae1ce11925 --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/macros1.xml @@ -0,0 +1,6 @@ + + + shard1 + replica1 + + diff --git a/tests/integration/test_cluster_discovery/config/macros2.xml b/tests/integration/test_cluster_discovery/config/macros2.xml new file mode 100644 index 00000000000..989555d8225 --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/macros2.xml @@ -0,0 +1,6 @@ + + + shard2 + replica2 + + diff --git a/tests/integration/test_cluster_discovery/config/macros3.xml b/tests/integration/test_cluster_discovery/config/macros3.xml new file mode 100644 index 00000000000..e0d5ea23696 --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/macros3.xml @@ -0,0 +1,6 @@ + + + shard3 + replica3 + + diff --git a/tests/integration/test_cluster_discovery/config/macros4.xml b/tests/integration/test_cluster_discovery/config/macros4.xml new file mode 100644 index 00000000000..38b4c43d1b9 --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/macros4.xml @@ -0,0 +1,6 @@ + + + shard4 + replica4 + + diff --git a/tests/integration/test_cluster_discovery/config/macros_o.xml b/tests/integration/test_cluster_discovery/config/macros_o.xml new file mode 100644 index 00000000000..4ac6241b0a6 --- /dev/null +++ b/tests/integration/test_cluster_discovery/config/macros_o.xml @@ -0,0 +1,6 @@ + + + shard_o + replica_o + + diff --git a/tests/integration/test_cluster_discovery/test.py b/tests/integration/test_cluster_discovery/test.py index b47c8d06269..59317d2be48 100644 --- a/tests/integration/test_cluster_discovery/test.py +++ b/tests/integration/test_cluster_discovery/test.py @@ -20,7 +20,7 @@ shard_configs = { nodes = { node_name: cluster.add_instance( node_name, - main_configs=[shard_config], + main_configs=[shard_config, "config/config_discovery_path.xml"], stay_alive=True, with_zookeeper=True, ) diff --git a/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py b/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py new file mode 100644 index 00000000000..cfc94a05b58 --- /dev/null +++ b/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py @@ -0,0 +1,121 @@ +import functools + +import pytest + +from helpers.cluster import ClickHouseCluster + +from .common import check_on_cluster + +cluster = ClickHouseCluster(__file__) + +shard_configs = { + "node0": ["config/config.xml", "config/macros0.xml"], + "node1": ["config/config_shard1.xml", "config/macros1.xml"], + "node2": ["config/config.xml", "config/macros2.xml"], + "node3": ["config/config_shard3.xml", "config/macros3.xml"], + "node4": ["config/config.xml", "config/macros4.xml"], + "node_observer": ["config/config_observer.xml", "config/macros_o.xml"], +} + +nodes = { + node_name: cluster.add_instance( + node_name, + main_configs=shard_config + ["config/config_discovery_path_auxiliary_keeper.xml", "config/config_keepers.xml"], + stay_alive=True, + with_zookeeper=True, + #use_keeper=False, + ) + for node_name, shard_config in shard_configs.items() +} + + +@pytest.fixture(scope="module") +def start_cluster(): + try: + cluster.start() + yield cluster + finally: + cluster.shutdown() + + +def test_cluster_discovery_with_auxiliary_keeper_startup_and_stop(start_cluster): + """ + Start cluster, check nodes count in system.clusters, + then stop/start some nodes and check that it (dis)appeared in cluster. + """ + + check_nodes_count = functools.partial( + check_on_cluster, what="count()", msg="Wrong nodes count in cluster" + ) + check_shard_num = functools.partial( + check_on_cluster, + what="count(DISTINCT shard_num)", + msg="Wrong shard_num count in cluster", + ) + + total_shards = 3 + total_nodes = 5 + + check_nodes_count( + [nodes["node0"], nodes["node2"], nodes["node_observer"]], total_nodes + ) + check_shard_num( + [nodes["node0"], nodes["node2"], nodes["node_observer"]], total_shards + ) + + # test ON CLUSTER query + nodes["node0"].query( + "CREATE TABLE tbl ON CLUSTER 'test_auto_cluster' (x UInt64) ENGINE = ReplicatedMergeTree('zookeeper2:/clickhouse/{shard}/tbl', '{replica}') ORDER BY x" + ) + nodes["node0"].query("INSERT INTO tbl VALUES (1)") + nodes["node1"].query("INSERT INTO tbl VALUES (2)") + + assert ( + int( + nodes["node_observer"] + .query( + "SELECT sum(x) FROM clusterAllReplicas(test_auto_cluster, default.tbl)" + ) + .strip() + ) + == 3 + ) + + # Query SYSTEM DROP DNS CACHE may reload cluster configuration + # check that it does not affect cluster discovery + nodes["node1"].query("SYSTEM DROP DNS CACHE") + nodes["node0"].query("SYSTEM DROP DNS CACHE") + + check_shard_num( + [nodes["node0"], nodes["node2"], nodes["node_observer"]], total_shards + ) + + nodes["node1"].stop_clickhouse(kill=True) + check_nodes_count( + [nodes["node0"], nodes["node2"], nodes["node_observer"]], total_nodes - 1 + ) + + # node1 was the only node in shard '1' + check_shard_num( + [nodes["node0"], nodes["node2"], nodes["node_observer"]], total_shards - 1 + ) + + nodes["node3"].stop_clickhouse() + check_nodes_count( + [nodes["node0"], nodes["node2"], nodes["node_observer"]], total_nodes - 2 + ) + + nodes["node1"].start_clickhouse() + check_nodes_count( + [nodes["node0"], nodes["node2"], nodes["node_observer"]], total_nodes - 1 + ) + + nodes["node3"].start_clickhouse() + check_nodes_count( + [nodes["node0"], nodes["node2"], nodes["node_observer"]], total_nodes + ) + + # regular cluster is not affected + check_nodes_count( + [nodes["node1"], nodes["node2"]], 2, cluster_name="two_shards", retries=1 + ) From 43f3c886a23de600b50813fbfc4fdebb58a7325f Mon Sep 17 00:00:00 2001 From: udiz Date: Wed, 13 Nov 2024 22:46:36 +0000 Subject: [PATCH 13/91] add test --- tests/queries/0_stateless/03216_arrayWithConstant_limits.sql | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/queries/0_stateless/03216_arrayWithConstant_limits.sql b/tests/queries/0_stateless/03216_arrayWithConstant_limits.sql index c46524c50e6..a8efaa81e54 100644 --- a/tests/queries/0_stateless/03216_arrayWithConstant_limits.sql +++ b/tests/queries/0_stateless/03216_arrayWithConstant_limits.sql @@ -1,3 +1,6 @@ SELECT arrayWithConstant(96142475, ['qMUF']); -- { serverError TOO_LARGE_ARRAY_SIZE } SELECT arrayWithConstant(100000000, materialize([[[[[[[[[['Hello, world!']]]]]]]]]])); -- { serverError TOO_LARGE_ARRAY_SIZE } SELECT length(arrayWithConstant(10000000, materialize([[[[[[[[[['Hello world']]]]]]]]]]))); + +CREATE TEMPORARY TABLE args (value Array(Int)) ENGINE=Memory AS SELECT [1, 1, 1, 1] as value FROM numbers(1, 10000); +CREATE TEMPORARY TABLE results (value Array(Array(Int))) ENGINE=Memory AS SELECT arrayWithConstant(10000, value) FROM args; \ No newline at end of file From 6879aa130a8e07023e26ffd53a872afbbf737f79 Mon Sep 17 00:00:00 2001 From: udiz Date: Wed, 13 Nov 2024 22:47:54 +0000 Subject: [PATCH 14/91] newline --- tests/queries/0_stateless/03216_arrayWithConstant_limits.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/queries/0_stateless/03216_arrayWithConstant_limits.sql b/tests/queries/0_stateless/03216_arrayWithConstant_limits.sql index a8efaa81e54..4c0e0b08b24 100644 --- a/tests/queries/0_stateless/03216_arrayWithConstant_limits.sql +++ b/tests/queries/0_stateless/03216_arrayWithConstant_limits.sql @@ -3,4 +3,4 @@ SELECT arrayWithConstant(100000000, materialize([[[[[[[[[['Hello, world!']]]]]]] SELECT length(arrayWithConstant(10000000, materialize([[[[[[[[[['Hello world']]]]]]]]]]))); CREATE TEMPORARY TABLE args (value Array(Int)) ENGINE=Memory AS SELECT [1, 1, 1, 1] as value FROM numbers(1, 10000); -CREATE TEMPORARY TABLE results (value Array(Array(Int))) ENGINE=Memory AS SELECT arrayWithConstant(10000, value) FROM args; \ No newline at end of file +CREATE TEMPORARY TABLE results (value Array(Array(Int))) ENGINE=Memory AS SELECT arrayWithConstant(10000, value) FROM args; From 57ddde47ea9185e47da3893e22ea8438d1469d1e Mon Sep 17 00:00:00 2001 From: Anton Ivashkin Date: Thu, 14 Nov 2024 11:16:48 +0100 Subject: [PATCH 15/91] Use auxiliary keeper for cluster discovery --- src/Interpreters/ClusterDiscovery.cpp | 11 +++++++---- src/Interpreters/ClusterDiscovery.h | 3 +++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/Interpreters/ClusterDiscovery.cpp b/src/Interpreters/ClusterDiscovery.cpp index 6f9c375c2f5..ab2ec886e76 100644 --- a/src/Interpreters/ClusterDiscovery.cpp +++ b/src/Interpreters/ClusterDiscovery.cpp @@ -129,9 +129,11 @@ ClusterDiscovery::ClusterDiscovery( if (!config.has(cluster_config_prefix)) continue; - String zk_root = config.getString(cluster_config_prefix + ".path"); - if (zk_root.empty()) + String zk_name_and_root = config.getString(cluster_config_prefix + ".path"); + if (zk_name_and_root.empty()) throw Exception(ErrorCodes::NO_ELEMENTS_IN_CONFIG, "ZooKeeper path for cluster '{}' is empty", key); + String zk_root = zkutil::extractZooKeeperPath(zk_name_and_root, true); + String zk_name = zkutil::extractZooKeeperName(zk_name_and_root); const auto & password = config.getString(cluster_config_prefix + ".password", ""); const auto & cluster_secret = config.getString(cluster_config_prefix + ".secret", ""); @@ -142,6 +144,7 @@ ClusterDiscovery::ClusterDiscovery( key, ClusterInfo( /* name_= */ key, + /* zk_name_= */ zk_name, /* zk_root_= */ zk_root, /* host_name= */ config.getString(cluster_config_prefix + ".my_hostname", getFQDNOrHostName()), /* username= */ config.getString(cluster_config_prefix + ".user", context->getUserName()), @@ -288,7 +291,7 @@ bool ClusterDiscovery::updateCluster(ClusterInfo & cluster_info) { LOG_DEBUG(log, "Updating cluster '{}'", cluster_info.name); - auto zk = context->getZooKeeper(); + auto zk = context->getDefaultOrAuxiliaryZooKeeper(cluster_info.zk_name); int start_version; Strings node_uuids = getNodeNames(zk, cluster_info.zk_root, cluster_info.name, &start_version, false); @@ -381,9 +384,9 @@ void ClusterDiscovery::initialUpdate() throw Exception(ErrorCodes::KEEPER_EXCEPTION, "Failpoint cluster_discovery_faults is triggered"); }); - auto zk = context->getZooKeeper(); for (auto & [_, info] : clusters_info) { + auto zk = context->getDefaultOrAuxiliaryZooKeeper(info.zk_name); registerInZk(zk, info); if (!updateCluster(info)) { diff --git a/src/Interpreters/ClusterDiscovery.h b/src/Interpreters/ClusterDiscovery.h index 756ed3d8d9b..b253473ce3e 100644 --- a/src/Interpreters/ClusterDiscovery.h +++ b/src/Interpreters/ClusterDiscovery.h @@ -67,6 +67,7 @@ private: struct ClusterInfo { const String name; + const String zk_name; const String zk_root; NodesInfo nodes_info; @@ -88,6 +89,7 @@ private: String cluster_secret; ClusterInfo(const String & name_, + const String & zk_name_, const String & zk_root_, const String & host_name, const String & username_, @@ -99,6 +101,7 @@ private: bool observer_mode, bool invisible) : name(name_) + , zk_name(zk_name_) , zk_root(zk_root_) , current_node(host_name + ":" + toString(port), secure, shard_id) , current_node_is_observer(observer_mode) From 1c682f13163522c2a38634f7566e55d7bebc7b4a Mon Sep 17 00:00:00 2001 From: Nikita Taranov Date: Thu, 14 Nov 2024 22:38:05 +0100 Subject: [PATCH 16/91] impl --- .../ObjectStorage/StorageObjectStorageSource.cpp | 2 +- .../03271_s3_table_function_asterisk_glob.reference | 4 ++++ .../03271_s3_table_function_asterisk_glob.sql | 12 ++++++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.reference create mode 100644 tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.sql diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp index 1ccf23ade90..105bea87dc4 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp @@ -305,7 +305,7 @@ StorageObjectStorageSource::ReaderHolder StorageObjectStorageSource::createReade { object_info = file_iterator->next(processor); - if (!object_info || object_info->getFileName().empty()) + if (!object_info) return {}; if (!object_info->metadata) diff --git a/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.reference b/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.reference new file mode 100644 index 00000000000..bc856dafab0 --- /dev/null +++ b/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.reference @@ -0,0 +1,4 @@ +0 +1 +2 +3 diff --git a/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.sql b/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.sql new file mode 100644 index 00000000000..0dabd3de7ba --- /dev/null +++ b/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.sql @@ -0,0 +1,12 @@ +-- Tags: no-parallel, no-fasttest +-- Tag no-fasttest: Depends on AWS + +SET max_threads = 1; +SET s3_truncate_on_insert = 1; + +INSERT INTO FUNCTION s3(s3_conn, filename='dir1/03271_s3_table_function_asterisk_glob/', format=Parquet) SELECT 0 as num; +INSERT INTO FUNCTION s3(s3_conn, filename='dir1/03271_s3_table_function_asterisk_glob/file1', format=Parquet) SELECT 1 as num; +INSERT INTO FUNCTION s3(s3_conn, filename='dir1/03271_s3_table_function_asterisk_glob/file2', format=Parquet) SELECT 2 as num; +INSERT INTO FUNCTION s3(s3_conn, filename='dir1/03271_s3_table_function_asterisk_glob/file3', format=Parquet) SELECT 3 as num; + +SELECT * FROM s3(s3_conn, filename='dir1/03271_s3_table_function_asterisk_glob/*') ORDER BY ALL; From bff84d44e82341887c477fca94a0bc978ea84ac6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Fri, 15 Nov 2024 17:13:14 +0100 Subject: [PATCH 17/91] Analyzer is not beta --- src/Core/Settings.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Core/Settings.cpp b/src/Core/Settings.cpp index dd04d0f4b26..0fea7d047ad 100644 --- a/src/Core/Settings.cpp +++ b/src/Core/Settings.cpp @@ -5638,10 +5638,10 @@ Build local plan for local replica \ DECLARE(Bool, allow_experimental_analyzer, true, R"( Allow new query analyzer. -)", IMPORTANT | BETA) ALIAS(enable_analyzer) \ +)", IMPORTANT) ALIAS(enable_analyzer) \ DECLARE(Bool, analyzer_compatibility_join_using_top_level_identifier, false, R"( Force to resolve identifier in JOIN USING from projection (for example, in `SELECT a + 1 AS b FROM t1 JOIN t2 USING (b)` join will be performed by `t1.a + 1 = t2.b`, rather then `t1.b = t2.b`). -)", BETA) \ +)", 0) \ \ DECLARE(Timezone, session_timezone, "", R"( Sets the implicit time zone of the current session or query. From 764dd82345f15f8b14042aa805ebae8c5a1086a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Fri, 15 Nov 2024 18:21:53 +0100 Subject: [PATCH 18/91] Support MergeTree settings and clickhouse local --- programs/local/LocalServer.cpp | 4 +++ programs/server/Server.cpp | 7 ++-- src/Storages/MergeTree/MergeTreeData.cpp | 13 +++++-- src/Storages/MergeTree/MergeTreeSettings.cpp | 36 +++++++++++++++++--- src/Storages/MergeTree/MergeTreeSettings.h | 2 +- 5 files changed, 53 insertions(+), 9 deletions(-) diff --git a/programs/local/LocalServer.cpp b/programs/local/LocalServer.cpp index e6f8ecef097..3ecc6ecf24d 100644 --- a/programs/local/LocalServer.cpp +++ b/programs/local/LocalServer.cpp @@ -79,6 +79,7 @@ namespace Setting namespace ServerSetting { + extern const ServerSettingsUInt32 allowed_feature_tier; extern const ServerSettingsDouble cache_size_to_ram_max_ratio; extern const ServerSettingsUInt64 compiled_expression_cache_elements_size; extern const ServerSettingsUInt64 compiled_expression_cache_size; @@ -789,6 +790,9 @@ void LocalServer::processConfig() /// Initialize a dummy query cache. global_context->setQueryCache(0, 0, 0, 0); + /// Initialize allowed tiers + global_context->getAccessControl().setAllowTierSettings(server_settings[ServerSetting::allowed_feature_tier]); + #if USE_EMBEDDED_COMPILER size_t compiled_expression_cache_max_size_in_bytes = server_settings[ServerSetting::compiled_expression_cache_size]; size_t compiled_expression_cache_max_elements = server_settings[ServerSetting::compiled_expression_cache_elements_size]; diff --git a/programs/server/Server.cpp b/programs/server/Server.cpp index 9c25d50c3c4..07821f2b6dd 100644 --- a/programs/server/Server.cpp +++ b/programs/server/Server.cpp @@ -2163,9 +2163,12 @@ try /// Check sanity of MergeTreeSettings on server startup { + /// All settings can be changed in the global config + bool allowed_experimental = 1; + bool allowed_beta = 1; size_t background_pool_tasks = global_context->getMergeMutateExecutor()->getMaxTasksCount(); - global_context->getMergeTreeSettings().sanityCheck(background_pool_tasks); - global_context->getReplicatedMergeTreeSettings().sanityCheck(background_pool_tasks); + global_context->getMergeTreeSettings().sanityCheck(background_pool_tasks, allowed_experimental, allowed_beta); + global_context->getReplicatedMergeTreeSettings().sanityCheck(background_pool_tasks, allowed_experimental, allowed_beta); } /// try set up encryption. There are some errors in config, error will be printed and server wouldn't start. CompressionCodecEncrypted::Configuration::instance().load(config(), "encryption_codecs"); diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp index b2f35d0a309..03da2826790 100644 --- a/src/Storages/MergeTree/MergeTreeData.cpp +++ b/src/Storages/MergeTree/MergeTreeData.cpp @@ -1,5 +1,6 @@ #include +#include #include #include #include @@ -461,7 +462,12 @@ MergeTreeData::MergeTreeData( /// Check sanity of MergeTreeSettings. Only when table is created. if (sanity_checks) - settings->sanityCheck(getContext()->getMergeMutateExecutor()->getMaxTasksCount()); + { + const auto & ac = getContext()->getAccessControl(); + bool allow_experimental = ac.getAllowExperimentalTierSettings(); + bool allow_beta = ac.getAllowBetaTierSettings(); + settings->sanityCheck(getContext()->getMergeMutateExecutor()->getMaxTasksCount(), allow_experimental, allow_beta); + } if (!date_column_name.empty()) { @@ -3895,7 +3901,10 @@ void MergeTreeData::changeSettings( /// Reset to default settings before applying existing. auto copy = getDefaultSettings(); copy->applyChanges(new_changes); - copy->sanityCheck(getContext()->getMergeMutateExecutor()->getMaxTasksCount()); + const auto & ac = getContext()->getAccessControl(); + bool allow_experimental = ac.getAllowExperimentalTierSettings(); + bool allow_beta = ac.getAllowBetaTierSettings(); + copy->sanityCheck(getContext()->getMergeMutateExecutor()->getMaxTasksCount(), allow_experimental, allow_beta); storage_settings.set(std::move(copy)); StorageInMemoryMetadata new_metadata = getInMemoryMetadata(); diff --git a/src/Storages/MergeTree/MergeTreeSettings.cpp b/src/Storages/MergeTree/MergeTreeSettings.cpp index fcd4e05cf00..37ca4672652 100644 --- a/src/Storages/MergeTree/MergeTreeSettings.cpp +++ b/src/Storages/MergeTree/MergeTreeSettings.cpp @@ -28,6 +28,7 @@ namespace ErrorCodes { extern const int UNKNOWN_SETTING; extern const int BAD_ARGUMENTS; + extern const int READONLY; } // clang-format off @@ -295,7 +296,7 @@ struct MergeTreeSettingsImpl : public BaseSettings void loadFromQuery(ASTStorage & storage_def, ContextPtr context, bool is_attach); /// Check that the values are sane taking also query-level settings into account. - void sanityCheck(size_t background_pool_tasks) const; + void sanityCheck(size_t background_pool_tasks, bool allow_experimental, bool allow_beta) const; }; IMPLEMENT_SETTINGS_TRAITS(MergeTreeSettingsTraits, LIST_OF_MERGE_TREE_SETTINGS) @@ -375,8 +376,35 @@ void MergeTreeSettingsImpl::loadFromQuery(ASTStorage & storage_def, ContextPtr c #undef ADD_IF_ABSENT } -void MergeTreeSettingsImpl::sanityCheck(size_t background_pool_tasks) const +void MergeTreeSettingsImpl::sanityCheck(size_t background_pool_tasks, bool allowed_experimental, bool allowed_beta) const { + if (!allowed_experimental || !allowed_beta) + { + for (const auto & setting : all()) + { + if (!setting.isValueChanged()) + continue; + + auto tier = setting.getTier(); + if (!allowed_experimental && tier == EXPERIMENTAL) + { + throw Exception( + ErrorCodes::READONLY, + "Cannot modify setting '{}'. Changes to EXPERIMENTAL settings are disabled in the server config " + "('allowed_feature_tier')", + setting.getName()); + } + if (!allowed_beta && tier == BETA) + { + throw Exception( + ErrorCodes::READONLY, + "Cannot modify setting '{}'. Changes to BETA settings are disabled in the server config ('allowed_feature_tier')", + setting.getName()); + } + } + } + + if (number_of_free_entries_in_pool_to_execute_mutation > background_pool_tasks) { throw Exception(ErrorCodes::BAD_ARGUMENTS, "The value of 'number_of_free_entries_in_pool_to_execute_mutation' setting" @@ -623,9 +651,9 @@ bool MergeTreeSettings::needSyncPart(size_t input_rows, size_t input_bytes) cons || (impl->min_compressed_bytes_to_fsync_after_merge && input_bytes >= impl->min_compressed_bytes_to_fsync_after_merge)); } -void MergeTreeSettings::sanityCheck(size_t background_pool_tasks) const +void MergeTreeSettings::sanityCheck(size_t background_pool_tasks, bool allow_experimental, bool allow_beta) const { - impl->sanityCheck(background_pool_tasks); + impl->sanityCheck(background_pool_tasks, allow_experimental, allow_beta); } void MergeTreeSettings::dumpToSystemMergeTreeSettingsColumns(MutableColumnsAndConstraints & params) const diff --git a/src/Storages/MergeTree/MergeTreeSettings.h b/src/Storages/MergeTree/MergeTreeSettings.h index f0712dcf27a..40d4f775848 100644 --- a/src/Storages/MergeTree/MergeTreeSettings.h +++ b/src/Storages/MergeTree/MergeTreeSettings.h @@ -77,7 +77,7 @@ struct MergeTreeSettings void loadFromConfig(const String & config_elem, const Poco::Util::AbstractConfiguration & config); bool needSyncPart(size_t input_rows, size_t input_bytes) const; - void sanityCheck(size_t background_pool_tasks) const; + void sanityCheck(size_t background_pool_tasks, bool allow_experimental, bool allow_beta) const; void dumpToSystemMergeTreeSettingsColumns(MutableColumnsAndConstraints & params) const; From f73d48c61b585621f2051632473fa24d64d90f58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Fri, 15 Nov 2024 18:23:14 +0100 Subject: [PATCH 19/91] Some integration tests for allowed_feature_tier --- .../test_allowed_feature_tier/__init__.py | 0 .../configs/allowed_feature_tier.xml | 3 + .../test_allowed_feature_tier/test.py | 171 ++++++++++++++++++ 3 files changed, 174 insertions(+) create mode 100644 tests/integration/test_allowed_feature_tier/__init__.py create mode 100644 tests/integration/test_allowed_feature_tier/configs/allowed_feature_tier.xml create mode 100644 tests/integration/test_allowed_feature_tier/test.py diff --git a/tests/integration/test_allowed_feature_tier/__init__.py b/tests/integration/test_allowed_feature_tier/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/integration/test_allowed_feature_tier/configs/allowed_feature_tier.xml b/tests/integration/test_allowed_feature_tier/configs/allowed_feature_tier.xml new file mode 100644 index 00000000000..f24c54711f4 --- /dev/null +++ b/tests/integration/test_allowed_feature_tier/configs/allowed_feature_tier.xml @@ -0,0 +1,3 @@ + + 0 + diff --git a/tests/integration/test_allowed_feature_tier/test.py b/tests/integration/test_allowed_feature_tier/test.py new file mode 100644 index 00000000000..42b22906224 --- /dev/null +++ b/tests/integration/test_allowed_feature_tier/test.py @@ -0,0 +1,171 @@ +import pytest + +from helpers.cluster import ClickHouseCluster + +cluster = ClickHouseCluster(__file__) +instance = cluster.add_instance( + "instance", + main_configs=["configs/allowed_feature_tier.xml"], + stay_alive=True, +) + +config_path = "/etc/clickhouse-server/config.d/allowed_feature_tier.xml" + + +@pytest.fixture(scope="module") +def start_cluster(): + try: + cluster.start() + yield cluster + finally: + cluster.shutdown() + + +def get_current_tier_value(instance): + query_with_current_tier_value = ( + "SELECT value FROM system.server_settings where name = 'allowed_feature_tier'" + ) + return instance.query(query_with_current_tier_value).strip() + + +def test_allowed_feature_tier_in_general_settings(start_cluster): + # We use these settings as an example. If it fails in the future because you've changed the tier of the setting + # please change it to another setting in the same tier. If there is none, feel free to comment the test for that tier + query_with_experimental_setting = ( + "SELECT 1 SETTINGS allow_experimental_time_series_table=1" + ) + query_with_beta_setting = "SELECT 1 SETTINGS enable_parallel_replicas=1" + + assert "0" == get_current_tier_value(instance) + output, error = instance.query_and_get_answer_with_error( + query_with_experimental_setting + ) + assert error == "" + assert "1" == output.strip() + + # Disable experimental settings + instance.replace_in_config(config_path, "0", "1") + instance.query("SYSTEM RELOAD CONFIG") + assert "1" == get_current_tier_value(instance) + + output, error = instance.query_and_get_answer_with_error( + query_with_experimental_setting + ) + assert output == "" + assert "Changes to EXPERIMENTAL settings are disabled" in error + + output, error = instance.query_and_get_answer_with_error(query_with_beta_setting) + assert error == "" + assert "1" == output.strip() + + # Disable experimental and beta settings + instance.replace_in_config(config_path, "1", "2") + instance.query("SYSTEM RELOAD CONFIG") + assert "2" == get_current_tier_value(instance) + + output, error = instance.query_and_get_answer_with_error( + query_with_experimental_setting + ) + assert output == "" + assert "Changes to EXPERIMENTAL settings are disabled" in error + + output, error = instance.query_and_get_answer_with_error(query_with_beta_setting) + assert output == "" + assert "Changes to BETA settings are disabled" in error + + # Leave the server as it was + instance.replace_in_config(config_path, "2", "0") + instance.query("SYSTEM RELOAD CONFIG") + assert "0" == get_current_tier_value(instance) + + +def test_allowed_feature_tier_in_mergetree_settings(start_cluster): + assert "0" == get_current_tier_value(instance) + instance.query("DROP TABLE IF EXISTS test_experimental") + + # Disable experimental settings + instance.replace_in_config(config_path, "0", "1") + instance.query("SYSTEM RELOAD CONFIG") + assert "1" == get_current_tier_value(instance) + + query_with_experimental_mergetree_setting = """ + CREATE TABLE test_experimental (uid String, version UInt32, is_deleted UInt8) + ENGINE = ReplacingMergeTree(version, is_deleted) + ORDER by (uid) + SETTINGS allow_experimental_replacing_merge_with_cleanup=1; + """ + + output, error = instance.query_and_get_answer_with_error( + query_with_experimental_mergetree_setting + ) + assert output == "" + assert "Changes to EXPERIMENTAL settings are disabled" in error + + # Go back + instance.replace_in_config(config_path, "1", "0") + instance.query("SYSTEM RELOAD CONFIG") + assert "0" == get_current_tier_value(instance) + + output, error = instance.query_and_get_answer_with_error( + query_with_experimental_mergetree_setting + ) + assert output == "" + assert error == "" + + output = instance.query( + "SELECT engine_full FROM system.tables WHERE name = 'test_experimental'" + ) + assert "allow_experimental_replacing_merge_with_cleanup" in output + + # We now disable experimental settings and restart the server to confirm it boots correctly + instance.replace_in_config(config_path, "0", "1") + instance.query("SYSTEM RELOAD CONFIG") + assert "1" == get_current_tier_value(instance) + + instance.restart_clickhouse() + + # After the reboot the table will be there + output = instance.query( + "SELECT engine_full FROM system.tables WHERE name = 'test_experimental'" + ) + assert "allow_experimental_replacing_merge_with_cleanup" in output + + # Creating a different table should not be possible + output, error = instance.query_and_get_answer_with_error( + """ + CREATE TABLE test_experimental_new (uid String, version UInt32, is_deleted UInt8) + ENGINE = ReplacingMergeTree(version, is_deleted) + ORDER by (uid) + SETTINGS allow_experimental_replacing_merge_with_cleanup=1; + """ + ) + assert output == "" + assert "Changes to EXPERIMENTAL settings are disabled" in error + + # Creating a different table and altering its settings to enable experimental should not be possible either + output, error = instance.query_and_get_answer_with_error( + """ + CREATE TABLE test_experimental_new (uid String, version UInt32, is_deleted UInt8) + ENGINE = ReplacingMergeTree(version, is_deleted) + ORDER by (uid); + """ + ) + assert output == "" + + output, error = instance.query_and_get_answer_with_error( + """ + ALTER TABLE test_experimental_new MODIFY setting allow_experimental_replacing_merge_with_cleanup=1 + """ + ) + assert output == "" + assert "Changes to EXPERIMENTAL settings are disabled" in error + instance.query("DROP TABLE IF EXISTS test_experimental_new") + + instance.replace_in_config(config_path, "1", "0") + instance.query("SYSTEM RELOAD CONFIG") + assert "0" == get_current_tier_value(instance) + instance.query("DROP TABLE IF EXISTS test_experimental") + + +def test_allowed_feature_tier_in_user(start_cluster): + pass From 268b823010587b87f65825c1f6876e2c9a755d5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Fri, 15 Nov 2024 18:37:21 +0100 Subject: [PATCH 20/91] Test new users --- .../test_allowed_feature_tier/test.py | 85 ++++++++++++++++++- 1 file changed, 84 insertions(+), 1 deletion(-) diff --git a/tests/integration/test_allowed_feature_tier/test.py b/tests/integration/test_allowed_feature_tier/test.py index 42b22906224..2bab3278ec6 100644 --- a/tests/integration/test_allowed_feature_tier/test.py +++ b/tests/integration/test_allowed_feature_tier/test.py @@ -168,4 +168,87 @@ def test_allowed_feature_tier_in_mergetree_settings(start_cluster): def test_allowed_feature_tier_in_user(start_cluster): - pass + instance.query("DROP USER IF EXISTS user_experimental") + + assert "0" == get_current_tier_value(instance) + instance.query("DROP TABLE IF EXISTS test_experimental") + + # Disable experimental settings + instance.replace_in_config(config_path, "0", "1") + instance.query("SYSTEM RELOAD CONFIG") + assert "1" == get_current_tier_value(instance) + + output, error = instance.query_and_get_answer_with_error( + "CREATE USER user_experimental IDENTIFIED WITH no_password SETTINGS allow_experimental_time_series_table = 1" + ) + assert output == "" + assert "Changes to EXPERIMENTAL settings are disabled" in error + + # Go back to normal and create the user to restart the server and verify it works + instance.replace_in_config(config_path, "1", "0") + instance.query("SYSTEM RELOAD CONFIG") + assert "0" == get_current_tier_value(instance) + + output, error = instance.query_and_get_answer_with_error( + "CREATE USER user_experimental IDENTIFIED WITH no_password SETTINGS allow_experimental_time_series_table = 1" + ) + assert output == "" + assert error == "" + + # Default user = 0 + output, error = instance.query_and_get_answer_with_error( + "SELECT value FROM system.settings WHERE name = 'allow_experimental_time_series_table'" + ) + assert output.strip() == "0" + assert error == "" + + # New user = 1 + output, error = instance.query_and_get_answer_with_error( + "SELECT value FROM system.settings WHERE name = 'allow_experimental_time_series_table'", + user="user_experimental", + ) + assert output.strip() == "1" + assert error == "" + + # Change back to block experimental features and restart to confirm everything is working as expected (only new changes are blocked) + instance.replace_in_config(config_path, "0", "1") + instance.query("SYSTEM RELOAD CONFIG") + assert "1" == get_current_tier_value(instance) + + instance.restart_clickhouse() + + # Default user = 0 + output, error = instance.query_and_get_answer_with_error( + "SELECT value FROM system.settings WHERE name = 'allow_experimental_time_series_table'" + ) + assert output.strip() == "0" + assert error == "" + + # New user = 1 + output, error = instance.query_and_get_answer_with_error( + "SELECT value FROM system.settings WHERE name = 'allow_experimental_time_series_table'", + user="user_experimental", + ) + assert output.strip() == "1" + assert error == "" + + # But note that they can't change the value either + # 1 - 1 => OK + output, error = instance.query_and_get_answer_with_error( + "SELECT 1 SETTINGS allow_experimental_time_series_table=1", + user="user_experimental", + ) + assert output.strip() == "1" + assert error == "" + # 1 - 0 => KO + output, error = instance.query_and_get_answer_with_error( + "SELECT 1 SETTINGS allow_experimental_time_series_table=0", + user="user_experimental", + ) + assert output == "" + assert "Changes to EXPERIMENTAL settings are disabled" in error + + instance.replace_in_config(config_path, "1", "0") + instance.query("SYSTEM RELOAD CONFIG") + assert "0" == get_current_tier_value(instance) + instance.query("DROP USER IF EXISTS user_experimental") From ec0bdcc1cb84dc9d542a84b56b03bbd98ed5d8b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Fri, 15 Nov 2024 19:11:18 +0100 Subject: [PATCH 21/91] Test and fix reload of default profile with allowed experimental settings from config --- src/Interpreters/InterpreterSystemQuery.cpp | 5 ++- .../configs/users.d/users.xml | 17 ++++++++ .../test_allowed_feature_tier/test.py | 43 +++++++++++++++++++ 3 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 tests/integration/test_allowed_feature_tier/configs/users.d/users.xml diff --git a/src/Interpreters/InterpreterSystemQuery.cpp b/src/Interpreters/InterpreterSystemQuery.cpp index b651bfb245e..21b8be93f92 100644 --- a/src/Interpreters/InterpreterSystemQuery.cpp +++ b/src/Interpreters/InterpreterSystemQuery.cpp @@ -289,7 +289,10 @@ BlockIO InterpreterSystemQuery::execute() /// Use global context with fresh system profile settings auto system_context = Context::createCopy(getContext()->getGlobalContext()); - system_context->setSetting("profile", getContext()->getSystemProfileName()); + /// Don't check for constraints when changing profile. It was accepted before (for example it might include + /// some experimental settings) + bool check_constraints = false; + system_context->setCurrentProfile(getContext()->getSystemProfileName(), check_constraints); /// Make canonical query for simpler processing if (query.type == Type::RELOAD_DICTIONARY) diff --git a/tests/integration/test_allowed_feature_tier/configs/users.d/users.xml b/tests/integration/test_allowed_feature_tier/configs/users.d/users.xml new file mode 100644 index 00000000000..aae78773f35 --- /dev/null +++ b/tests/integration/test_allowed_feature_tier/configs/users.d/users.xml @@ -0,0 +1,17 @@ + + + + 0 + + + + + + default + default + 1 + 1 + 1 + + + diff --git a/tests/integration/test_allowed_feature_tier/test.py b/tests/integration/test_allowed_feature_tier/test.py index 2bab3278ec6..81767a3b1f0 100644 --- a/tests/integration/test_allowed_feature_tier/test.py +++ b/tests/integration/test_allowed_feature_tier/test.py @@ -6,6 +6,9 @@ cluster = ClickHouseCluster(__file__) instance = cluster.add_instance( "instance", main_configs=["configs/allowed_feature_tier.xml"], + user_configs=[ + "configs/users.d/users.xml", + ], stay_alive=True, ) @@ -252,3 +255,43 @@ def test_allowed_feature_tier_in_user(start_cluster): instance.query("SYSTEM RELOAD CONFIG") assert "0" == get_current_tier_value(instance) instance.query("DROP USER IF EXISTS user_experimental") + + +def test_it_is_possible_to_enable_experimental_settings_in_default_profile( + start_cluster, +): + # You can disable changing experimental settings but changing the default value via global config file is ok + # It will just make the default value different and block changes + instance.replace_in_config(config_path, "0", "2") + + # Change default user config + instance.replace_in_config( + "/etc/clickhouse-server/users.d/users.xml", + "allow_experimental_time_series_table>.", + "allow_experimental_time_series_table>1", + ) + + instance.query("SYSTEM RELOAD CONFIG") + assert "2" == get_current_tier_value(instance) + output, error = instance.query_and_get_answer_with_error( + "SELECT value FROM system.settings WHERE name = 'allow_experimental_time_series_table'" + ) + assert output.strip() == "1" + assert error == "" + + # But it won't be possible to change it + output, error = instance.query_and_get_answer_with_error( + "SELECT 1 SETTINGS allow_experimental_time_series_table=0" + ) + assert output == "" + assert "Changes to EXPERIMENTAL settings are disabled" in error + + instance.replace_in_config(config_path, "2", "0") + instance.replace_in_config( + "/etc/clickhouse-server/users.d/users.xml", + "allow_experimental_time_series_table>.", + "allow_experimental_time_series_table>0", + ) + + instance.query("SYSTEM RELOAD CONFIG") + assert "0" == get_current_tier_value(instance) From a82ab36c08b02918779afed9dd27ac237cb3416e Mon Sep 17 00:00:00 2001 From: Nikita Taranov Date: Fri, 15 Nov 2024 12:12:43 +0100 Subject: [PATCH 22/91] try fix --- src/Storages/ObjectStorage/ReadBufferIterator.cpp | 1 - src/Storages/ObjectStorage/StorageObjectStorageSource.cpp | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Storages/ObjectStorage/ReadBufferIterator.cpp b/src/Storages/ObjectStorage/ReadBufferIterator.cpp index 7dd3ad0d79f..7c184f52872 100644 --- a/src/Storages/ObjectStorage/ReadBufferIterator.cpp +++ b/src/Storages/ObjectStorage/ReadBufferIterator.cpp @@ -218,7 +218,6 @@ ReadBufferIterator::Data ReadBufferIterator::next() } const auto filename = current_object_info->getFileName(); - chassert(!filename.empty()); /// file iterator could get new keys after new iteration if (read_keys.size() > prev_read_keys_size) diff --git a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp index 105bea87dc4..fd99b832973 100644 --- a/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp +++ b/src/Storages/ObjectStorage/StorageObjectStorageSource.cpp @@ -305,7 +305,7 @@ StorageObjectStorageSource::ReaderHolder StorageObjectStorageSource::createReade { object_info = file_iterator->next(processor); - if (!object_info) + if (!object_info || object_info->getPath().empty()) return {}; if (!object_info->metadata) From 2d48406a82a37fd7e076e6a6728a54e5fb60ef64 Mon Sep 17 00:00:00 2001 From: Anton Ivashkin Date: Mon, 18 Nov 2024 11:21:32 +0100 Subject: [PATCH 23/91] Fix test style --- .../test_cluster_discovery/test_auxiliary_keeper.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py b/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py index cfc94a05b58..31f32de8b4f 100644 --- a/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py +++ b/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py @@ -20,10 +20,13 @@ shard_configs = { nodes = { node_name: cluster.add_instance( node_name, - main_configs=shard_config + ["config/config_discovery_path_auxiliary_keeper.xml", "config/config_keepers.xml"], + main_configs=shard_config + + [ + "config/config_discovery_path_auxiliary_keeper.xml", + "config/config_keepers.xml", + ], stay_alive=True, with_zookeeper=True, - #use_keeper=False, ) for node_name, shard_config in shard_configs.items() } From e5b7ba77176d54d40813f51fba3d6ced8ad1ee3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Mon, 18 Nov 2024 12:07:39 +0100 Subject: [PATCH 24/91] Tidy --- src/Storages/MergeTree/MergeTreeSettings.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Storages/MergeTree/MergeTreeSettings.cpp b/src/Storages/MergeTree/MergeTreeSettings.cpp index 37ca4672652..3b0cd187f65 100644 --- a/src/Storages/MergeTree/MergeTreeSettings.cpp +++ b/src/Storages/MergeTree/MergeTreeSettings.cpp @@ -376,9 +376,9 @@ void MergeTreeSettingsImpl::loadFromQuery(ASTStorage & storage_def, ContextPtr c #undef ADD_IF_ABSENT } -void MergeTreeSettingsImpl::sanityCheck(size_t background_pool_tasks, bool allowed_experimental, bool allowed_beta) const +void MergeTreeSettingsImpl::sanityCheck(size_t background_pool_tasks, bool allow_experimental, bool allow_beta) const { - if (!allowed_experimental || !allowed_beta) + if (!allow_experimental || !allow_beta) { for (const auto & setting : all()) { @@ -386,7 +386,7 @@ void MergeTreeSettingsImpl::sanityCheck(size_t background_pool_tasks, bool allow continue; auto tier = setting.getTier(); - if (!allowed_experimental && tier == EXPERIMENTAL) + if (!allow_experimental && tier == EXPERIMENTAL) { throw Exception( ErrorCodes::READONLY, @@ -394,7 +394,7 @@ void MergeTreeSettingsImpl::sanityCheck(size_t background_pool_tasks, bool allow "('allowed_feature_tier')", setting.getName()); } - if (!allowed_beta && tier == BETA) + if (!allow_beta && tier == BETA) { throw Exception( ErrorCodes::READONLY, From 926f7ca7a2740b48ff65cd5e8ae35fd67316d127 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Mon, 18 Nov 2024 12:14:18 +0100 Subject: [PATCH 25/91] Style changes --- .../test_allowed_feature_tier/test.py | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/tests/integration/test_allowed_feature_tier/test.py b/tests/integration/test_allowed_feature_tier/test.py index 81767a3b1f0..fff1ff76906 100644 --- a/tests/integration/test_allowed_feature_tier/test.py +++ b/tests/integration/test_allowed_feature_tier/test.py @@ -12,7 +12,7 @@ instance = cluster.add_instance( stay_alive=True, ) -config_path = "/etc/clickhouse-server/config.d/allowed_feature_tier.xml" +feature_tier_path = "/etc/clickhouse-server/config.d/allowed_feature_tier.xml" @pytest.fixture(scope="module") @@ -33,7 +33,7 @@ def get_current_tier_value(instance): def test_allowed_feature_tier_in_general_settings(start_cluster): # We use these settings as an example. If it fails in the future because you've changed the tier of the setting - # please change it to another setting in the same tier. If there is none, feel free to comment the test for that tier + # please change it to another setting in the same tier. If there is none, feel free to comment out the test for that tier query_with_experimental_setting = ( "SELECT 1 SETTINGS allow_experimental_time_series_table=1" ) @@ -47,7 +47,7 @@ def test_allowed_feature_tier_in_general_settings(start_cluster): assert "1" == output.strip() # Disable experimental settings - instance.replace_in_config(config_path, "0", "1") + instance.replace_in_config(feature_tier_path, "0", "1") instance.query("SYSTEM RELOAD CONFIG") assert "1" == get_current_tier_value(instance) @@ -62,7 +62,7 @@ def test_allowed_feature_tier_in_general_settings(start_cluster): assert "1" == output.strip() # Disable experimental and beta settings - instance.replace_in_config(config_path, "1", "2") + instance.replace_in_config(feature_tier_path, "1", "2") instance.query("SYSTEM RELOAD CONFIG") assert "2" == get_current_tier_value(instance) @@ -77,7 +77,7 @@ def test_allowed_feature_tier_in_general_settings(start_cluster): assert "Changes to BETA settings are disabled" in error # Leave the server as it was - instance.replace_in_config(config_path, "2", "0") + instance.replace_in_config(feature_tier_path, "2", "0") instance.query("SYSTEM RELOAD CONFIG") assert "0" == get_current_tier_value(instance) @@ -87,7 +87,7 @@ def test_allowed_feature_tier_in_mergetree_settings(start_cluster): instance.query("DROP TABLE IF EXISTS test_experimental") # Disable experimental settings - instance.replace_in_config(config_path, "0", "1") + instance.replace_in_config(feature_tier_path, "0", "1") instance.query("SYSTEM RELOAD CONFIG") assert "1" == get_current_tier_value(instance) @@ -105,7 +105,7 @@ def test_allowed_feature_tier_in_mergetree_settings(start_cluster): assert "Changes to EXPERIMENTAL settings are disabled" in error # Go back - instance.replace_in_config(config_path, "1", "0") + instance.replace_in_config(feature_tier_path, "1", "0") instance.query("SYSTEM RELOAD CONFIG") assert "0" == get_current_tier_value(instance) @@ -121,7 +121,7 @@ def test_allowed_feature_tier_in_mergetree_settings(start_cluster): assert "allow_experimental_replacing_merge_with_cleanup" in output # We now disable experimental settings and restart the server to confirm it boots correctly - instance.replace_in_config(config_path, "0", "1") + instance.replace_in_config(feature_tier_path, "0", "1") instance.query("SYSTEM RELOAD CONFIG") assert "1" == get_current_tier_value(instance) @@ -164,7 +164,7 @@ def test_allowed_feature_tier_in_mergetree_settings(start_cluster): assert "Changes to EXPERIMENTAL settings are disabled" in error instance.query("DROP TABLE IF EXISTS test_experimental_new") - instance.replace_in_config(config_path, "1", "0") + instance.replace_in_config(feature_tier_path, "1", "0") instance.query("SYSTEM RELOAD CONFIG") assert "0" == get_current_tier_value(instance) instance.query("DROP TABLE IF EXISTS test_experimental") @@ -172,12 +172,10 @@ def test_allowed_feature_tier_in_mergetree_settings(start_cluster): def test_allowed_feature_tier_in_user(start_cluster): instance.query("DROP USER IF EXISTS user_experimental") - assert "0" == get_current_tier_value(instance) - instance.query("DROP TABLE IF EXISTS test_experimental") # Disable experimental settings - instance.replace_in_config(config_path, "0", "1") + instance.replace_in_config(feature_tier_path, "0", "1") instance.query("SYSTEM RELOAD CONFIG") assert "1" == get_current_tier_value(instance) @@ -188,7 +186,7 @@ def test_allowed_feature_tier_in_user(start_cluster): assert "Changes to EXPERIMENTAL settings are disabled" in error # Go back to normal and create the user to restart the server and verify it works - instance.replace_in_config(config_path, "1", "0") + instance.replace_in_config(feature_tier_path, "1", "0") instance.query("SYSTEM RELOAD CONFIG") assert "0" == get_current_tier_value(instance) @@ -214,7 +212,7 @@ def test_allowed_feature_tier_in_user(start_cluster): assert error == "" # Change back to block experimental features and restart to confirm everything is working as expected (only new changes are blocked) - instance.replace_in_config(config_path, "0", "1") + instance.replace_in_config(feature_tier_path, "0", "1") instance.query("SYSTEM RELOAD CONFIG") assert "1" == get_current_tier_value(instance) @@ -251,7 +249,7 @@ def test_allowed_feature_tier_in_user(start_cluster): assert output == "" assert "Changes to EXPERIMENTAL settings are disabled" in error - instance.replace_in_config(config_path, "1", "0") + instance.replace_in_config(feature_tier_path, "1", "0") instance.query("SYSTEM RELOAD CONFIG") assert "0" == get_current_tier_value(instance) instance.query("DROP USER IF EXISTS user_experimental") @@ -262,7 +260,7 @@ def test_it_is_possible_to_enable_experimental_settings_in_default_profile( ): # You can disable changing experimental settings but changing the default value via global config file is ok # It will just make the default value different and block changes - instance.replace_in_config(config_path, "0", "2") + instance.replace_in_config(feature_tier_path, "0", "2") # Change default user config instance.replace_in_config( @@ -286,7 +284,7 @@ def test_it_is_possible_to_enable_experimental_settings_in_default_profile( assert output == "" assert "Changes to EXPERIMENTAL settings are disabled" in error - instance.replace_in_config(config_path, "2", "0") + instance.replace_in_config(feature_tier_path, "2", "0") instance.replace_in_config( "/etc/clickhouse-server/users.d/users.xml", "allow_experimental_time_series_table>.", From e45dd36343915d3504067126b913e60faae7be0b Mon Sep 17 00:00:00 2001 From: Nikita Taranov Date: Mon, 18 Nov 2024 13:38:26 +0100 Subject: [PATCH 26/91] add test from Christoph --- .../03271_s3_table_function_asterisk_glob.reference | 4 ++++ .../03271_s3_table_function_asterisk_glob.sql | 13 +++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.reference b/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.reference index bc856dafab0..d4dc73eff36 100644 --- a/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.reference +++ b/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.reference @@ -2,3 +2,7 @@ 1 2 3 +0 +1 +2 +3 diff --git a/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.sql b/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.sql index 0dabd3de7ba..b0937a073c1 100644 --- a/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.sql +++ b/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.sql @@ -1,12 +1,21 @@ -- Tags: no-parallel, no-fasttest -- Tag no-fasttest: Depends on AWS -SET max_threads = 1; SET s3_truncate_on_insert = 1; +SET s3_skip_empty_files = 0; INSERT INTO FUNCTION s3(s3_conn, filename='dir1/03271_s3_table_function_asterisk_glob/', format=Parquet) SELECT 0 as num; INSERT INTO FUNCTION s3(s3_conn, filename='dir1/03271_s3_table_function_asterisk_glob/file1', format=Parquet) SELECT 1 as num; INSERT INTO FUNCTION s3(s3_conn, filename='dir1/03271_s3_table_function_asterisk_glob/file2', format=Parquet) SELECT 2 as num; INSERT INTO FUNCTION s3(s3_conn, filename='dir1/03271_s3_table_function_asterisk_glob/file3', format=Parquet) SELECT 3 as num; -SELECT * FROM s3(s3_conn, filename='dir1/03271_s3_table_function_asterisk_glob/*') ORDER BY ALL; +SELECT * FROM s3(s3_conn, filename='dir1/03271_s3_table_function_asterisk_glob/*') ORDER BY ALL SETTINGS max_threads = 1; +SELECT * FROM s3(s3_conn, filename='dir1/03271_s3_table_function_asterisk_glob/*') ORDER BY ALL SETTINGS max_threads = 4; + +-- Empty "directory" files created implicitly by S3 console: +-- https://docs.aws.amazon.com/AmazonS3/latest/userguide/using-folders.html +SELECT * +FROM s3('https://datasets-documentation.s3.eu-west-3.amazonaws.com/amazon_reviews/*', NOSIGN) +LIMIT 1 +FORMAT Null +SETTINGS s3_skip_empty_files = 1; From fb552dd2c054f629ac12e32b9eef49109aba6765 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Mon, 18 Nov 2024 13:49:11 +0100 Subject: [PATCH 27/91] Remove unused trash --- src/Columns/ColumnDecimal.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Columns/ColumnDecimal.h b/src/Columns/ColumnDecimal.h index 6f8360a54dd..924a5bdf0b2 100644 --- a/src/Columns/ColumnDecimal.h +++ b/src/Columns/ColumnDecimal.h @@ -3,7 +3,6 @@ #include #include #include -#include #include #include #include @@ -98,8 +97,6 @@ public: return StringRef(reinterpret_cast(&data[n]), sizeof(data[n])); } - Float64 getFloat64(size_t n) const final { return DecimalUtils::convertTo(data[n], scale); } - const char * deserializeAndInsertFromArena(const char * pos) override; const char * skipSerializedInArena(const char * pos) const override; void updateHashWithValue(size_t n, SipHash & hash) const override; From 3a012e5a967af5949d9ce228a8b765146aa7021e Mon Sep 17 00:00:00 2001 From: Nikita Taranov Date: Mon, 18 Nov 2024 13:55:18 +0100 Subject: [PATCH 28/91] enable s3_skip_empty_files by default --- docs/en/engines/table-engines/integrations/s3.md | 2 +- docs/en/sql-reference/table-functions/s3.md | 2 +- src/Core/Settings.cpp | 2 +- src/Core/SettingsChangesHistory.cpp | 1 + 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/en/engines/table-engines/integrations/s3.md b/docs/en/engines/table-engines/integrations/s3.md index fd27d4b6ed9..9868b2a05a8 100644 --- a/docs/en/engines/table-engines/integrations/s3.md +++ b/docs/en/engines/table-engines/integrations/s3.md @@ -258,7 +258,7 @@ CREATE TABLE table_with_asterisk (name String, value UInt32) - [s3_truncate_on_insert](/docs/en/operations/settings/settings.md#s3_truncate_on_insert) - allows to truncate file before insert into it. Disabled by default. - [s3_create_new_file_on_insert](/docs/en/operations/settings/settings.md#s3_create_new_file_on_insert) - allows to create a new file on each insert if format has suffix. Disabled by default. -- [s3_skip_empty_files](/docs/en/operations/settings/settings.md#s3_skip_empty_files) - allows to skip empty files while reading. Disabled by default. +- [s3_skip_empty_files](/docs/en/operations/settings/settings.md#s3_skip_empty_files) - allows to skip empty files while reading. Enabled by default. ## S3-related Settings {#settings} diff --git a/docs/en/sql-reference/table-functions/s3.md b/docs/en/sql-reference/table-functions/s3.md index b14eb84392f..ea7820c1aec 100644 --- a/docs/en/sql-reference/table-functions/s3.md +++ b/docs/en/sql-reference/table-functions/s3.md @@ -317,7 +317,7 @@ SELECT * from s3('s3://data/path/date=*/country=*/code=*/*.parquet') where _date - [s3_truncate_on_insert](/docs/en/operations/settings/settings.md#s3_truncate_on_insert) - allows to truncate file before insert into it. Disabled by default. - [s3_create_new_file_on_insert](/docs/en/operations/settings/settings.md#s3_create_new_file_on_insert) - allows to create a new file on each insert if format has suffix. Disabled by default. -- [s3_skip_empty_files](/docs/en/operations/settings/settings.md#s3_skip_empty_files) - allows to skip empty files while reading. Disabled by default. +- [s3_skip_empty_files](/docs/en/operations/settings/settings.md#s3_skip_empty_files) - allows to skip empty files while reading. Enabled by default. **See Also** diff --git a/src/Core/Settings.cpp b/src/Core/Settings.cpp index 0e2e1f8a3f0..2fee9b4a718 100644 --- a/src/Core/Settings.cpp +++ b/src/Core/Settings.cpp @@ -430,7 +430,7 @@ Possible values: - 0 — `INSERT` query appends new data to the end of the file. - 1 — `INSERT` query creates a new file. )", 0) \ - DECLARE(Bool, s3_skip_empty_files, false, R"( + DECLARE(Bool, s3_skip_empty_files, true, R"( Enables or disables skipping empty files in [S3](../../engines/table-engines/integrations/s3.md) engine tables. Possible values: diff --git a/src/Core/SettingsChangesHistory.cpp b/src/Core/SettingsChangesHistory.cpp index dc95054f722..b4e0a29ec25 100644 --- a/src/Core/SettingsChangesHistory.cpp +++ b/src/Core/SettingsChangesHistory.cpp @@ -84,6 +84,7 @@ static std::initializer_list Date: Mon, 18 Nov 2024 14:04:02 +0100 Subject: [PATCH 29/91] upd test --- .../03271_s3_table_function_asterisk_glob.reference | 8 ++++++++ .../03271_s3_table_function_asterisk_glob.sql | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.reference b/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.reference index d4dc73eff36..373831be4eb 100644 --- a/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.reference +++ b/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.reference @@ -6,3 +6,11 @@ 1 2 3 +0 +1 +2 +3 +0 +1 +2 +3 diff --git a/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.sql b/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.sql index b0937a073c1..37d974c5503 100644 --- a/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.sql +++ b/tests/queries/0_stateless/03271_s3_table_function_asterisk_glob.sql @@ -12,6 +12,9 @@ INSERT INTO FUNCTION s3(s3_conn, filename='dir1/03271_s3_table_function_asterisk SELECT * FROM s3(s3_conn, filename='dir1/03271_s3_table_function_asterisk_glob/*') ORDER BY ALL SETTINGS max_threads = 1; SELECT * FROM s3(s3_conn, filename='dir1/03271_s3_table_function_asterisk_glob/*') ORDER BY ALL SETTINGS max_threads = 4; +SELECT * FROM s3Cluster('test_cluster_two_shards_localhost', s3_conn, filename='dir1/03271_s3_table_function_asterisk_glob/*') ORDER BY ALL SETTINGS max_threads = 1; +SELECT * FROM s3Cluster('test_cluster_two_shards_localhost', s3_conn, filename='dir1/03271_s3_table_function_asterisk_glob/*') ORDER BY ALL SETTINGS max_threads = 4; + -- Empty "directory" files created implicitly by S3 console: -- https://docs.aws.amazon.com/AmazonS3/latest/userguide/using-folders.html SELECT * @@ -19,3 +22,10 @@ FROM s3('https://datasets-documentation.s3.eu-west-3.amazonaws.com/amazon_review LIMIT 1 FORMAT Null SETTINGS s3_skip_empty_files = 1; + +-- TODO: Still doesn't work +-- SELECT * +-- FROM s3Cluster('test_cluster_two_shards_localhost', 'https://datasets-documentation.s3.eu-west-3.amazonaws.com/amazon_reviews/*', NOSIGN) +-- LIMIT 1 +-- FORMAT Null +-- SETTINGS s3_skip_empty_files = 1; From fb4e1feb16c4de9082c2f6eb066a79477bdbdef5 Mon Sep 17 00:00:00 2001 From: Anton Ivashkin Date: Mon, 18 Nov 2024 14:38:46 +0100 Subject: [PATCH 30/91] Add cleanup after test --- tests/integration/test_cluster_discovery/test.py | 5 +++++ .../test_cluster_discovery/test_auxiliary_keeper.py | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/tests/integration/test_cluster_discovery/test.py b/tests/integration/test_cluster_discovery/test.py index 59317d2be48..4c447ac06a2 100644 --- a/tests/integration/test_cluster_discovery/test.py +++ b/tests/integration/test_cluster_discovery/test.py @@ -119,3 +119,8 @@ def test_cluster_discovery_startup_and_stop(start_cluster): check_nodes_count( [nodes["node1"], nodes["node2"]], 2, cluster_name="two_shards", retries=1 ) + + # cleanup + nodes["node0"].query( + "DROP TABLE tbl ON CLUSTER 'test_auto_cluster' SYNC" + ) diff --git a/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py b/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py index 31f32de8b4f..fe2e885b3c9 100644 --- a/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py +++ b/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py @@ -122,3 +122,8 @@ def test_cluster_discovery_with_auxiliary_keeper_startup_and_stop(start_cluster) check_nodes_count( [nodes["node1"], nodes["node2"]], 2, cluster_name="two_shards", retries=1 ) + + # cleanup + nodes["node0"].query( + "DROP TABLE tbl ON CLUSTER 'test_auto_cluster' SYNC" + ) From 9917dc66d4d83e7c7a8dfb96dc236790ace9ed72 Mon Sep 17 00:00:00 2001 From: Anton Ivashkin Date: Mon, 18 Nov 2024 16:18:36 +0100 Subject: [PATCH 31/91] Antother style fix --- tests/integration/test_cluster_discovery/test.py | 4 +--- .../test_cluster_discovery/test_auxiliary_keeper.py | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/integration/test_cluster_discovery/test.py b/tests/integration/test_cluster_discovery/test.py index 4c447ac06a2..7eb1000b23a 100644 --- a/tests/integration/test_cluster_discovery/test.py +++ b/tests/integration/test_cluster_discovery/test.py @@ -121,6 +121,4 @@ def test_cluster_discovery_startup_and_stop(start_cluster): ) # cleanup - nodes["node0"].query( - "DROP TABLE tbl ON CLUSTER 'test_auto_cluster' SYNC" - ) + nodes["node0"].query("DROP TABLE tbl ON CLUSTER 'test_auto_cluster' SYNC") diff --git a/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py b/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py index fe2e885b3c9..d35a22ab09a 100644 --- a/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py +++ b/tests/integration/test_cluster_discovery/test_auxiliary_keeper.py @@ -124,6 +124,4 @@ def test_cluster_discovery_with_auxiliary_keeper_startup_and_stop(start_cluster) ) # cleanup - nodes["node0"].query( - "DROP TABLE tbl ON CLUSTER 'test_auto_cluster' SYNC" - ) + nodes["node0"].query("DROP TABLE tbl ON CLUSTER 'test_auto_cluster' SYNC") From ec776fe8db847b40e7ae079f6e2a51e954be917d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Mon, 18 Nov 2024 17:08:08 +0100 Subject: [PATCH 32/91] Remove wasteful template instatiations --- src/Core/callOnTypeIndex.h | 71 +++++++++++++++++++++++++++++ src/Functions/FunctionsComparison.h | 40 +++++++++++++++- 2 files changed, 110 insertions(+), 1 deletion(-) diff --git a/src/Core/callOnTypeIndex.h b/src/Core/callOnTypeIndex.h index 0c8f2201b0d..09fbc7f1f10 100644 --- a/src/Core/callOnTypeIndex.h +++ b/src/Core/callOnTypeIndex.h @@ -87,6 +87,77 @@ static bool callOnBasicType(TypeIndex number, F && f) return false; } + +template +static bool callOnBasicTypeSecondArg(TypeIndex number, F && f) +{ + if constexpr (_int) + { + switch (number) + { + case TypeIndex::UInt8: return f(TypePair()); + case TypeIndex::UInt16: return f(TypePair()); + case TypeIndex::UInt32: return f(TypePair()); + case TypeIndex::UInt64: return f(TypePair()); + case TypeIndex::UInt128: return f(TypePair()); + case TypeIndex::UInt256: return f(TypePair()); + + case TypeIndex::Int8: return f(TypePair()); + case TypeIndex::Int16: return f(TypePair()); + case TypeIndex::Int32: return f(TypePair()); + case TypeIndex::Int64: return f(TypePair()); + case TypeIndex::Int128: return f(TypePair()); + case TypeIndex::Int256: return f(TypePair()); + + case TypeIndex::Enum8: return f(TypePair()); + case TypeIndex::Enum16: return f(TypePair()); + + default: + break; + } + } + + if constexpr (_decimal) + { + switch (number) + { + case TypeIndex::Decimal32: return f(TypePair()); + case TypeIndex::Decimal64: return f(TypePair()); + case TypeIndex::Decimal128: return f(TypePair()); + case TypeIndex::Decimal256: return f(TypePair()); + default: + break; + } + } + + if constexpr (_float) + { + switch (number) + { + case TypeIndex::BFloat16: return f(TypePair()); + case TypeIndex::Float32: return f(TypePair()); + case TypeIndex::Float64: return f(TypePair()); + default: + break; + } + } + + if constexpr (_datetime) + { + switch (number) + { + case TypeIndex::Date: return f(TypePair()); + case TypeIndex::Date32: return f(TypePair()); + case TypeIndex::DateTime: return f(TypePair()); + case TypeIndex::DateTime64: return f(TypePair()); + default: + break; + } + } + + return false; +} + /// Unroll template using TypeIndex template static inline bool callOnBasicTypes(TypeIndex type_num1, TypeIndex type_num2, F && f) diff --git a/src/Functions/FunctionsComparison.h b/src/Functions/FunctionsComparison.h index c9d3ccb4445..e6133bcd773 100644 --- a/src/Functions/FunctionsComparison.h +++ b/src/Functions/FunctionsComparison.h @@ -50,6 +50,44 @@ namespace DB { +namespace +{ +template +static inline bool callOnAtLeastOneDecimalType(TypeIndex type_num1, TypeIndex type_num2, F && f) +{ + switch (type_num1) + { + case TypeIndex::Decimal32: + return callOnBasicType(type_num2, std::forward(f)); + case TypeIndex::Decimal64: + return callOnBasicType(type_num2, std::forward(f)); + case TypeIndex::Decimal128: + return callOnBasicType(type_num2, std::forward(f)); + case TypeIndex::Decimal256: + return callOnBasicType(type_num2, std::forward(f)); + default: + break; + } + + switch (type_num2) + { + case TypeIndex::Decimal32: + return callOnBasicTypeSecondArg(type_num1, std::forward(f)); + case TypeIndex::Decimal64: + return callOnBasicTypeSecondArg(type_num1, std::forward(f)); + case TypeIndex::Decimal128: + return callOnBasicTypeSecondArg(type_num1, std::forward(f)); + case TypeIndex::Decimal256: + return callOnBasicTypeSecondArg(type_num1, std::forward(f)); + default: + break; + } + + return false; +} +} + + namespace ErrorCodes { extern const int ILLEGAL_COLUMN; @@ -770,7 +808,7 @@ private: return (res = DecimalComparison::apply(col_left, col_right)) != nullptr; }; - if (!callOnBasicTypes(left_number, right_number, call)) + if (!callOnAtLeastOneDecimalType(left_number, right_number, call)) throw Exception(ErrorCodes::LOGICAL_ERROR, "Wrong call for {} with {} and {}", getName(), col_left.type->getName(), col_right.type->getName()); From 1c308f970b92d9c9c48404364cce713353aca916 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Mon, 18 Nov 2024 17:21:06 +0100 Subject: [PATCH 33/91] Try to remove more decimal instantiations --- src/Core/DecimalComparison.h | 133 +++++++++++++--------------- src/Core/Field.cpp | 9 +- src/Formats/ProtobufSerializer.cpp | 3 +- src/Functions/FunctionsComparison.h | 5 +- 4 files changed, 74 insertions(+), 76 deletions(-) diff --git a/src/Core/DecimalComparison.h b/src/Core/DecimalComparison.h index 77402adf164..7311105f6fe 100644 --- a/src/Core/DecimalComparison.h +++ b/src/Core/DecimalComparison.h @@ -52,8 +52,8 @@ struct DecCompareInt using TypeB = Type; }; -template typename Operation, bool _check_overflow = true, - bool _actual = is_decimal || is_decimal> +template typename Operation> +requires is_decimal || is_decimal class DecimalComparison { public: @@ -65,20 +65,17 @@ public: using ArrayA = typename ColVecA::Container; using ArrayB = typename ColVecB::Container; - static ColumnPtr apply(const ColumnWithTypeAndName & col_left, const ColumnWithTypeAndName & col_right) + static ColumnPtr apply(const ColumnWithTypeAndName & col_left, const ColumnWithTypeAndName & col_right, bool check_overflow) { - if constexpr (_actual) - { - ColumnPtr c_res; - Shift shift = getScales(col_left.type, col_right.type); + ColumnPtr c_res; + Shift shift = getScales(col_left.type, col_right.type); - return applyWithScale(col_left.column, col_right.column, shift); - } - else - return nullptr; + if (check_overflow) + return applyWithScale(col_left.column, col_right.column, shift); + return applyWithScale(col_left.column, col_right.column, shift); } - static bool compare(A a, B b, UInt32 scale_a, UInt32 scale_b) + static bool compare(A a, B b, UInt32 scale_a, UInt32 scale_b, bool check_overflow) { static const UInt32 max_scale = DecimalUtils::max_precision; if (scale_a > max_scale || scale_b > max_scale) @@ -90,7 +87,9 @@ public: if (scale_a > scale_b) shift.b = static_cast(DecimalUtils::scaleMultiplier(scale_a - scale_b)); - return applyWithScale(a, b, shift); + if (check_overflow) + return applyWithScale(a, b, shift); + return applyWithScale(a, b, shift); } private: @@ -104,14 +103,14 @@ private: bool right() const { return b != 1; } }; - template + template static auto applyWithScale(T a, U b, const Shift & shift) { if (shift.left()) - return apply(a, b, shift.a); + return apply(a, b, shift.a); if (shift.right()) - return apply(a, b, shift.b); - return apply(a, b, 1); + return apply(a, b, shift.b); + return apply(a, b, 1); } template @@ -158,66 +157,63 @@ private: return shift; } - template + template static ColumnPtr apply(const ColumnPtr & c0, const ColumnPtr & c1, CompareInt scale) { auto c_res = ColumnUInt8::create(); - if constexpr (_actual) + bool c0_is_const = isColumnConst(*c0); + bool c1_is_const = isColumnConst(*c1); + + if (c0_is_const && c1_is_const) { - bool c0_is_const = isColumnConst(*c0); - bool c1_is_const = isColumnConst(*c1); + const ColumnConst & c0_const = checkAndGetColumnConst(*c0); + const ColumnConst & c1_const = checkAndGetColumnConst(*c1); - if (c0_is_const && c1_is_const) + A a = c0_const.template getValue(); + B b = c1_const.template getValue(); + UInt8 res = apply(a, b, scale); + return DataTypeUInt8().createColumnConst(c0->size(), toField(res)); + } + + ColumnUInt8::Container & vec_res = c_res->getData(); + vec_res.resize(c0->size()); + + if (c0_is_const) + { + const ColumnConst & c0_const = checkAndGetColumnConst(*c0); + A a = c0_const.template getValue(); + if (const ColVecB * c1_vec = checkAndGetColumn(c1.get())) + constantVector(a, c1_vec->getData(), vec_res, scale); + else + throw Exception(ErrorCodes::LOGICAL_ERROR, "Wrong column in Decimal comparison"); + } + else if (c1_is_const) + { + const ColumnConst & c1_const = checkAndGetColumnConst(*c1); + B b = c1_const.template getValue(); + if (const ColVecA * c0_vec = checkAndGetColumn(c0.get())) + vectorConstant(c0_vec->getData(), b, vec_res, scale); + else + throw Exception(ErrorCodes::LOGICAL_ERROR, "Wrong column in Decimal comparison"); + } + else + { + if (const ColVecA * c0_vec = checkAndGetColumn(c0.get())) { - const ColumnConst & c0_const = checkAndGetColumnConst(*c0); - const ColumnConst & c1_const = checkAndGetColumnConst(*c1); - - A a = c0_const.template getValue(); - B b = c1_const.template getValue(); - UInt8 res = apply(a, b, scale); - return DataTypeUInt8().createColumnConst(c0->size(), toField(res)); - } - - ColumnUInt8::Container & vec_res = c_res->getData(); - vec_res.resize(c0->size()); - - if (c0_is_const) - { - const ColumnConst & c0_const = checkAndGetColumnConst(*c0); - A a = c0_const.template getValue(); if (const ColVecB * c1_vec = checkAndGetColumn(c1.get())) - constantVector(a, c1_vec->getData(), vec_res, scale); - else - throw Exception(ErrorCodes::LOGICAL_ERROR, "Wrong column in Decimal comparison"); - } - else if (c1_is_const) - { - const ColumnConst & c1_const = checkAndGetColumnConst(*c1); - B b = c1_const.template getValue(); - if (const ColVecA * c0_vec = checkAndGetColumn(c0.get())) - vectorConstant(c0_vec->getData(), b, vec_res, scale); + vectorVector(c0_vec->getData(), c1_vec->getData(), vec_res, scale); else throw Exception(ErrorCodes::LOGICAL_ERROR, "Wrong column in Decimal comparison"); } else - { - if (const ColVecA * c0_vec = checkAndGetColumn(c0.get())) - { - if (const ColVecB * c1_vec = checkAndGetColumn(c1.get())) - vectorVector(c0_vec->getData(), c1_vec->getData(), vec_res, scale); - else - throw Exception(ErrorCodes::LOGICAL_ERROR, "Wrong column in Decimal comparison"); - } - else - throw Exception(ErrorCodes::LOGICAL_ERROR, "Wrong column in Decimal comparison"); - } + throw Exception(ErrorCodes::LOGICAL_ERROR, "Wrong column in Decimal comparison"); } return c_res; } - template + template static NO_INLINE UInt8 apply(A a, B b, CompareInt scale [[maybe_unused]]) { CompareInt x; @@ -232,7 +228,7 @@ private: else y = static_cast(b); - if constexpr (_check_overflow) + if constexpr (check_overflow) { bool overflow = false; @@ -264,9 +260,8 @@ private: return Op::apply(x, y); } - template - static void NO_INLINE vectorVector(const ArrayA & a, const ArrayB & b, PaddedPODArray & c, - CompareInt scale) + template + static void NO_INLINE vectorVector(const ArrayA & a, const ArrayB & b, PaddedPODArray & c, CompareInt scale) { size_t size = a.size(); const A * a_pos = a.data(); @@ -276,14 +271,14 @@ private: while (a_pos < a_end) { - *c_pos = apply(*a_pos, *b_pos, scale); + *c_pos = apply(*a_pos, *b_pos, scale); ++a_pos; ++b_pos; ++c_pos; } } - template + template static void NO_INLINE vectorConstant(const ArrayA & a, B b, PaddedPODArray & c, CompareInt scale) { size_t size = a.size(); @@ -293,13 +288,13 @@ private: while (a_pos < a_end) { - *c_pos = apply(*a_pos, b, scale); + *c_pos = apply(*a_pos, b, scale); ++a_pos; ++c_pos; } } - template + template static void NO_INLINE constantVector(A a, const ArrayB & b, PaddedPODArray & c, CompareInt scale) { size_t size = b.size(); @@ -309,7 +304,7 @@ private: while (b_pos < b_end) { - *c_pos = apply(a, *b_pos, scale); + *c_pos = apply(a, *b_pos, scale); ++b_pos; ++c_pos; } diff --git a/src/Core/Field.cpp b/src/Core/Field.cpp index e774a95e19f..90f30b52c0c 100644 --- a/src/Core/Field.cpp +++ b/src/Core/Field.cpp @@ -529,22 +529,25 @@ Field Field::restoreFromDump(std::string_view dump_) template bool decimalEqual(T x, T y, UInt32 x_scale, UInt32 y_scale) { + bool check_overflow = true; using Comparator = DecimalComparison; - return Comparator::compare(x, y, x_scale, y_scale); + return Comparator::compare(x, y, x_scale, y_scale, check_overflow); } template bool decimalLess(T x, T y, UInt32 x_scale, UInt32 y_scale) { + bool check_overflow = true; using Comparator = DecimalComparison; - return Comparator::compare(x, y, x_scale, y_scale); + return Comparator::compare(x, y, x_scale, y_scale, check_overflow); } template bool decimalLessOrEqual(T x, T y, UInt32 x_scale, UInt32 y_scale) { + bool check_overflow = true; using Comparator = DecimalComparison; - return Comparator::compare(x, y, x_scale, y_scale); + return Comparator::compare(x, y, x_scale, y_scale, check_overflow); } diff --git a/src/Formats/ProtobufSerializer.cpp b/src/Formats/ProtobufSerializer.cpp index bd8aeff5a5d..5ea645c9c3c 100644 --- a/src/Formats/ProtobufSerializer.cpp +++ b/src/Formats/ProtobufSerializer.cpp @@ -1307,7 +1307,8 @@ namespace { if (decimal.value == 0) writeInt(0); - else if (DecimalComparison::compare(decimal, 1, scale, 0)) + else if (DecimalComparison::compare( + decimal, 1, scale, 0, /* check overflow */ true)) writeInt(1); else { diff --git a/src/Functions/FunctionsComparison.h b/src/Functions/FunctionsComparison.h index e6133bcd773..054d2fe7079 100644 --- a/src/Functions/FunctionsComparison.h +++ b/src/Functions/FunctionsComparison.h @@ -803,9 +803,8 @@ private: using LeftDataType = typename Types::LeftType; using RightDataType = typename Types::RightType; - if (check_decimal_overflow) - return (res = DecimalComparison::apply(col_left, col_right)) != nullptr; - return (res = DecimalComparison::apply(col_left, col_right)) != nullptr; + return (res = DecimalComparison::apply(col_left, col_right, check_decimal_overflow)) + != nullptr; }; if (!callOnAtLeastOneDecimalType(left_number, right_number, call)) From a258b6d0f270fc08ec98cbac8ecce58aaf2081c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Mon, 18 Nov 2024 17:36:45 +0100 Subject: [PATCH 34/91] Prevent magic_enum in Field.h --- src/Core/Field.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Core/Field.h b/src/Core/Field.h index c08d5c9eb42..5a6ee9cdf29 100644 --- a/src/Core/Field.h +++ b/src/Core/Field.h @@ -863,6 +863,9 @@ template <> struct Field::EnumToType { usi template <> struct Field::EnumToType { using Type = CustomType; }; template <> struct Field::EnumToType { using Type = UInt64; }; +/// Use it to prevent inclusion of magic_enum in headers, which is very expensive for the compiler +std::string_view fieldTypeToString(Field::Types::Which type); + constexpr bool isInt64OrUInt64FieldType(Field::Types::Which t) { return t == Field::Types::Int64 @@ -886,7 +889,7 @@ auto & Field::safeGet() if (target != which && !(which == Field::Types::Bool && (target == Field::Types::UInt64 || target == Field::Types::Int64)) && !(isInt64OrUInt64FieldType(which) && isInt64OrUInt64FieldType(target))) - throw Exception(ErrorCodes::BAD_GET, "Bad get: has {}, requested {}", getTypeName(), target); + throw Exception(ErrorCodes::BAD_GET, "Bad get: has {}, requested {}", getTypeName(), fieldTypeToString(target)); return get(); } @@ -1002,8 +1005,6 @@ void readQuoted(DecimalField & x, ReadBuffer & buf); void writeFieldText(const Field & x, WriteBuffer & buf); String toString(const Field & x); - -std::string_view fieldTypeToString(Field::Types::Which type); } template <> From e33f5bb4e943bda039b442e8999f5c28ba84ebca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ra=C3=BAl=20Mar=C3=ADn?= Date: Mon, 18 Nov 2024 17:45:41 +0100 Subject: [PATCH 35/91] Remove unused leftovers Usage was removed in 23.6 https://github.com/ClickHouse/ClickHouse/pull/50531 --- src/Functions/FunctionsComparison.h | 62 ----------------------------- 1 file changed, 62 deletions(-) diff --git a/src/Functions/FunctionsComparison.h b/src/Functions/FunctionsComparison.h index 054d2fe7079..0add34939b2 100644 --- a/src/Functions/FunctionsComparison.h +++ b/src/Functions/FunctionsComparison.h @@ -41,12 +41,6 @@ #include #include -#if USE_EMBEDDED_COMPILER -# include -# include -#endif - - namespace DB { @@ -612,62 +606,6 @@ struct GenericComparisonImpl } }; - -#if USE_EMBEDDED_COMPILER - -template