From 7c85b62e4ed0e7a701d78a88024e12120699643d Mon Sep 17 00:00:00 2001 From: vdimir Date: Mon, 8 Aug 2022 09:10:01 +0000 Subject: [PATCH 1/6] Add setting type number with 'auto' --- src/Core/SettingsFields.cpp | 72 ++++++++++++++++++++++++++++++++----- src/Core/SettingsFields.h | 42 ++++++++++++++++++++-- 2 files changed, 103 insertions(+), 11 deletions(-) diff --git a/src/Core/SettingsFields.cpp b/src/Core/SettingsFields.cpp index d77a510d7f9..e3bf44d33a7 100644 --- a/src/Core/SettingsFields.cpp +++ b/src/Core/SettingsFields.cpp @@ -156,29 +156,85 @@ template struct SettingFieldNumber; namespace { - UInt64 stringToMaxThreads(const String & str) + bool isAutoSetting(const String & str) + { + return startsWith(str, "auto"); + } + + bool isAutoSetting(const Field & f) + { + return f.getType() == Field::Types::String && isAutoSetting(f.get()); + } + + template + T stringToNumberWithAuto(const String & str) { if (startsWith(str, "auto")) return 0; - return parseFromString(str); + return parseFromString(str); } - UInt64 fieldToMaxThreads(const Field & f) + template + T fieldToNumberWithAuto(const Field & f) { if (f.getType() == Field::Types::String) - return stringToMaxThreads(f.get()); + return stringToNumberWithAuto(f.get()); else - return applyVisitor(FieldVisitorConvertToNumber(), f); + return applyVisitor(FieldVisitorConvertToNumber(), f); } } -SettingFieldMaxThreads::SettingFieldMaxThreads(const Field & f) : SettingFieldMaxThreads(fieldToMaxThreads(f)) +template +SettingFieldNumberWithAuto::SettingFieldNumberWithAuto(const Field & f) + : is_auto(isAutoSetting(f)), value(fieldToNumberWithAuto(f)) +{ +} + +template +SettingFieldNumberWithAuto & SettingFieldNumberWithAuto::operator=(const Field & f) +{ + is_auto = isAutoSetting(f); + value = fieldToNumberWithAuto(f); + return *this; +} + +template +String SettingFieldNumberWithAuto::toString() const +{ + if (is_auto) + return "auto"; + else + return ::DB::toString(value); +} + +template +void SettingFieldNumberWithAuto::parseFromString(const String & str) +{ + is_auto = isAutoSetting(str); + value = stringToNumberWithAuto(str); +} + +template +void SettingFieldNumberWithAuto::writeBinary(WriteBuffer & out) const +{ + writeStringBinary(toString(), out); +} + +template +void SettingFieldNumberWithAuto::readBinary(ReadBuffer & in) +{ + String str; + readStringBinary(str, in); + parseFromString(str); +} + +SettingFieldMaxThreads::SettingFieldMaxThreads(const Field & f) : SettingFieldMaxThreads(fieldToNumberWithAuto(f)) { } SettingFieldMaxThreads & SettingFieldMaxThreads::operator=(const Field & f) { - *this = fieldToMaxThreads(f); + *this = fieldToNumberWithAuto(f); return *this; } @@ -192,7 +248,7 @@ String SettingFieldMaxThreads::toString() const void SettingFieldMaxThreads::parseFromString(const String & str) { - *this = stringToMaxThreads(str); + *this = stringToNumberWithAuto(str); } void SettingFieldMaxThreads::writeBinary(WriteBuffer & out) const diff --git a/src/Core/SettingsFields.h b/src/Core/SettingsFields.h index 20f2b34084e..f7946368fc5 100644 --- a/src/Core/SettingsFields.h +++ b/src/Core/SettingsFields.h @@ -59,10 +59,46 @@ using SettingFieldFloat = SettingFieldNumber; using SettingFieldBool = SettingFieldNumber; -/** Unlike SettingFieldUInt64, supports the value of 'auto' - the number of processor cores without taking into account SMT. - * A value of 0 is also treated as auto. - * When serializing, `auto` is written in the same way as 0. +/** Like SettingFieldNumber, but also supports the value of 'auto'. + * Note: 0 and 'auto' are not equal. By default when 'auto' is set the value is set to 0. + * But if you need to distinguish between 0 and 'auto', you can use the 'is_auto' flag. + * Serialized as string. */ +template +struct SettingFieldNumberWithAuto +{ + using Type = T; + + bool is_auto; + T value; + bool changed = false; + + explicit SettingFieldNumberWithAuto() : is_auto(true), value(0) {} + explicit SettingFieldNumberWithAuto(T x) : is_auto(false), value(x) {} + explicit SettingFieldNumberWithAuto(const Field & f); + + SettingFieldNumberWithAuto & operator=(T x) { is_auto = false; value = x; changed = true; return *this; } + SettingFieldNumberWithAuto & operator=(const Field & f); + + operator T() const { return value; } /// NOLINT + explicit operator Field() const { return is_auto ? Field("auto") : Field(value); } + + String toString() const; + void parseFromString(const String & str); + + void writeBinary(WriteBuffer & out) const; + void readBinary(ReadBuffer & in); + + T valueOr(T default_value) const { return is_auto ? default_value : value; } +}; + +using SettingFieldUInt64WithAuto = SettingFieldNumberWithAuto; +using SettingFieldInt64WithAuto = SettingFieldNumberWithAuto; + +/* Similar to SettingFieldNumberWithAuto with small differences to behave like regular UInt64, supported to compatibility. + * When setting to 'auto' it becames equal to the number of processor cores without taking into account SMT. + * A value of 0 is also treated as 'auto', so 'auto' is parsed and serialized in the same way as 0. + */ struct SettingFieldMaxThreads { bool is_auto; From 3189c40e6d80478a0a91431f7dba37bc85c20427 Mon Sep 17 00:00:00 2001 From: vdimir Date: Tue, 9 Aug 2022 16:15:28 +0000 Subject: [PATCH 2/6] Implement SettingField with auto is a wrapper --- src/Core/SettingsFields.cpp | 75 +++++----------------------------- src/Core/SettingsFields.h | 81 +++++++++++++++++++++++++------------ 2 files changed, 66 insertions(+), 90 deletions(-) diff --git a/src/Core/SettingsFields.cpp b/src/Core/SettingsFields.cpp index e3bf44d33a7..86b20da9e8c 100644 --- a/src/Core/SettingsFields.cpp +++ b/src/Core/SettingsFields.cpp @@ -153,88 +153,35 @@ template struct SettingFieldNumber; template struct SettingFieldNumber; template struct SettingFieldNumber; +template struct SettingWithAuto>; +template struct SettingWithAuto>; +template struct SettingWithAuto>; namespace { - bool isAutoSetting(const String & str) - { - return startsWith(str, "auto"); - } - - bool isAutoSetting(const Field & f) - { - return f.getType() == Field::Types::String && isAutoSetting(f.get()); - } - - template - T stringToNumberWithAuto(const String & str) + UInt64 stringToMaxThreads(const String & str) { if (startsWith(str, "auto")) return 0; - return parseFromString(str); + return parseFromString(str); } - template - T fieldToNumberWithAuto(const Field & f) + UInt64 fieldToMaxThreads(const Field & f) { if (f.getType() == Field::Types::String) - return stringToNumberWithAuto(f.get()); + return stringToMaxThreads(f.get()); else - return applyVisitor(FieldVisitorConvertToNumber(), f); + return applyVisitor(FieldVisitorConvertToNumber(), f); } } -template -SettingFieldNumberWithAuto::SettingFieldNumberWithAuto(const Field & f) - : is_auto(isAutoSetting(f)), value(fieldToNumberWithAuto(f)) -{ -} - -template -SettingFieldNumberWithAuto & SettingFieldNumberWithAuto::operator=(const Field & f) -{ - is_auto = isAutoSetting(f); - value = fieldToNumberWithAuto(f); - return *this; -} - -template -String SettingFieldNumberWithAuto::toString() const -{ - if (is_auto) - return "auto"; - else - return ::DB::toString(value); -} - -template -void SettingFieldNumberWithAuto::parseFromString(const String & str) -{ - is_auto = isAutoSetting(str); - value = stringToNumberWithAuto(str); -} - -template -void SettingFieldNumberWithAuto::writeBinary(WriteBuffer & out) const -{ - writeStringBinary(toString(), out); -} - -template -void SettingFieldNumberWithAuto::readBinary(ReadBuffer & in) -{ - String str; - readStringBinary(str, in); - parseFromString(str); -} - -SettingFieldMaxThreads::SettingFieldMaxThreads(const Field & f) : SettingFieldMaxThreads(fieldToNumberWithAuto(f)) +SettingFieldMaxThreads::SettingFieldMaxThreads(const Field & f) : SettingFieldMaxThreads(fieldToMaxThreads(f)) { } SettingFieldMaxThreads & SettingFieldMaxThreads::operator=(const Field & f) { - *this = fieldToNumberWithAuto(f); + *this = fieldToMaxThreads(f); return *this; } @@ -248,7 +195,7 @@ String SettingFieldMaxThreads::toString() const void SettingFieldMaxThreads::parseFromString(const String & str) { - *this = stringToNumberWithAuto(str); + *this = stringToMaxThreads(str); } void SettingFieldMaxThreads::writeBinary(WriteBuffer & out) const diff --git a/src/Core/SettingsFields.h b/src/Core/SettingsFields.h index f7946368fc5..0fcc1d6783f 100644 --- a/src/Core/SettingsFields.h +++ b/src/Core/SettingsFields.h @@ -58,45 +58,74 @@ using SettingFieldInt64 = SettingFieldNumber; using SettingFieldFloat = SettingFieldNumber; using SettingFieldBool = SettingFieldNumber; - -/** Like SettingFieldNumber, but also supports the value of 'auto'. - * Note: 0 and 'auto' are not equal. By default when 'auto' is set the value is set to 0. - * But if you need to distinguish between 0 and 'auto', you can use the 'is_auto' flag. - * Serialized as string. +/** Wraps any SettingField to support special value 'auto' that can be checked with `is_auto` flag. + * Note about serialization: + * The new versions with `SettingsWriteFormat::STRINGS_WITH_FLAGS` serialize values as a string. + * In legacy SettingsWriteFormat mode, functions `read/writeBinary` would serialize values as a binary, and 'is_auto' would be ignored. + * It's possible to upgrade settings from regular type to wrapped ones and keep compatibility with old versions, + * but when serializing 'auto' old version will see binary representation of the default value. */ -template -struct SettingFieldNumberWithAuto +template +struct SettingWithAuto { - using Type = T; + constexpr static auto keyword = "auto"; + static bool isAuto(const Field & f) { return f.getType() == Field::Types::String && f.safeGet() == keyword; } + static bool isAuto(const String & str) { return str == keyword; } - bool is_auto; - T value; + using Type = typename Base::Type; + + Base base; + bool is_auto = false; bool changed = false; - explicit SettingFieldNumberWithAuto() : is_auto(true), value(0) {} - explicit SettingFieldNumberWithAuto(T x) : is_auto(false), value(x) {} - explicit SettingFieldNumberWithAuto(const Field & f); + explicit SettingWithAuto() : is_auto(true) {} + explicit SettingWithAuto(Type val) : is_auto(false) { base = Base(val); } - SettingFieldNumberWithAuto & operator=(T x) { is_auto = false; value = x; changed = true; return *this; } - SettingFieldNumberWithAuto & operator=(const Field & f); + explicit SettingWithAuto(const Field & f) + : is_auto(isAuto(f)) + { + if (!is_auto) + base = Base(f); + } - operator T() const { return value; } /// NOLINT - explicit operator Field() const { return is_auto ? Field("auto") : Field(value); } + SettingWithAuto & operator=(const Field & f) + { + changed = true; + if (is_auto = isAuto(f); !is_auto) + base = f; + return *this; + } - String toString() const; - void parseFromString(const String & str); + explicit operator Field() const { return is_auto ? Field(keyword) : Field(base); } - void writeBinary(WriteBuffer & out) const; - void readBinary(ReadBuffer & in); + String toString() const { return is_auto ? keyword : base.toString(); } - T valueOr(T default_value) const { return is_auto ? default_value : value; } + void parseFromString(const String & str) + { + changed = true; + if (is_auto = isAuto(str); !is_auto) + base.parseFromString(str); + } + + void writeBinary(WriteBuffer & out) const + { + if (is_auto) + Base().writeBinary(out); /// serialize default value + else + base.writeBinary(out); + } + + void readBinary(ReadBuffer & in) { changed = true; is_auto = false; base.readBinary(in); } + + Type valueOr(Type default_value) const { return is_auto ? default_value : base.value; } }; -using SettingFieldUInt64WithAuto = SettingFieldNumberWithAuto; -using SettingFieldInt64WithAuto = SettingFieldNumberWithAuto; +using SettingFieldUInt64WithAuto = SettingWithAuto; +using SettingFieldInt64WithAuto = SettingWithAuto; +using SettingFieldFloatWithAuto = SettingWithAuto; -/* Similar to SettingFieldNumberWithAuto with small differences to behave like regular UInt64, supported to compatibility. - * When setting to 'auto' it becames equal to the number of processor cores without taking into account SMT. +/* Similar to SettingFieldUInt64WithAuto with small differences to behave like regular UInt64, supported to compatibility. + * When setting to 'auto' it becomes equal to the number of processor cores without taking into account SMT. * A value of 0 is also treated as 'auto', so 'auto' is parsed and serialized in the same way as 0. */ struct SettingFieldMaxThreads From ae1db8386bf28563f993d3927411968f7f4f9455 Mon Sep 17 00:00:00 2001 From: vdimir Date: Tue, 9 Aug 2022 16:16:42 +0000 Subject: [PATCH 3/6] Change type of insert_quorum to UInt64WithAuto --- src/Core/Settings.h | 2 +- src/Storages/StorageReplicatedMergeTree.cpp | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Core/Settings.h b/src/Core/Settings.h index 92af0576129..727e45e3e50 100644 --- a/src/Core/Settings.h +++ b/src/Core/Settings.h @@ -212,7 +212,7 @@ static constexpr UInt64 operator""_GiB(unsigned long long value) \ M(Bool, insert_deduplicate, true, "For INSERT queries in the replicated table, specifies that deduplication of insertings blocks should be performed", 0) \ \ - M(UInt64, insert_quorum, 0, "For INSERT queries in the replicated table, wait writing for the specified number of replicas and linearize the addition of the data. 0 - disabled.", 0) \ + M(UInt64WithAuto, insert_quorum, 0, "For INSERT queries in the replicated table, wait writing for the specified number of replicas and linearize the addition of the data. 0 - disabled.", 0) \ M(Milliseconds, insert_quorum_timeout, 600000, "", 0) \ M(Bool, insert_quorum_parallel, true, "For quorum INSERT queries - enable to make parallel inserts without linearizability", 0) \ M(UInt64, select_sequential_consistency, 0, "For SELECT queries from the replicated table, throw an exception if the replica does not have a chunk written with the quorum; do not read the parts that have not yet been written with the quorum.", 0) \ diff --git a/src/Storages/StorageReplicatedMergeTree.cpp b/src/Storages/StorageReplicatedMergeTree.cpp index b4fc6b34c9e..3e5c1ce6982 100644 --- a/src/Storages/StorageReplicatedMergeTree.cpp +++ b/src/Storages/StorageReplicatedMergeTree.cpp @@ -4443,8 +4443,9 @@ SinkToStoragePtr StorageReplicatedMergeTree::write(const ASTPtr & /*query*/, con bool deduplicate = storage_settings_ptr->replicated_deduplication_window != 0 && query_settings.insert_deduplicate; // TODO: should we also somehow pass list of columns to deduplicate on to the ReplicatedMergeTreeSink? + // TODO: insert_quorum = 'auto' would be supported in https://github.com/ClickHouse/ClickHouse/pull/39970, now it's same as 0. return std::make_shared( - *this, metadata_snapshot, query_settings.insert_quorum, + *this, metadata_snapshot, query_settings.insert_quorum.valueOr(0), query_settings.insert_quorum_timeout.totalMilliseconds(), query_settings.max_partitions_per_insert_block, query_settings.insert_quorum_parallel, From 21ace403ab98f4fc3b221de2376d09deb9c1ffde Mon Sep 17 00:00:00 2001 From: vdimir Date: Tue, 9 Aug 2022 17:04:15 +0000 Subject: [PATCH 4/6] Add test setting_value_auto --- .../0_stateless/02381_setting_value_auto.reference | 4 ++++ tests/queries/0_stateless/02381_setting_value_auto.sql | 10 ++++++++++ 2 files changed, 14 insertions(+) create mode 100644 tests/queries/0_stateless/02381_setting_value_auto.reference create mode 100644 tests/queries/0_stateless/02381_setting_value_auto.sql diff --git a/tests/queries/0_stateless/02381_setting_value_auto.reference b/tests/queries/0_stateless/02381_setting_value_auto.reference new file mode 100644 index 00000000000..72c87cf6f7d --- /dev/null +++ b/tests/queries/0_stateless/02381_setting_value_auto.reference @@ -0,0 +1,4 @@ +0 0 UInt64WithAuto +auto 1 UInt64WithAuto +0 1 UInt64WithAuto +1 1 UInt64WithAuto diff --git a/tests/queries/0_stateless/02381_setting_value_auto.sql b/tests/queries/0_stateless/02381_setting_value_auto.sql new file mode 100644 index 00000000000..5b536a9d749 --- /dev/null +++ b/tests/queries/0_stateless/02381_setting_value_auto.sql @@ -0,0 +1,10 @@ +SELECT value, changed, type FROM system.settings WHERE name = 'insert_quorum'; + +SET insert_quorum = 'auto'; +SELECT value, changed, type FROM system.settings WHERE name = 'insert_quorum'; + +SET insert_quorum = 0; +SELECT value, changed, type FROM system.settings WHERE name = 'insert_quorum'; + +SET insert_quorum = 1; +SELECT value, changed, type FROM system.settings WHERE name = 'insert_quorum'; From 261ccc35cf2004c2f0603717a42a95e0ecbe8b2c Mon Sep 17 00:00:00 2001 From: Vladimir C Date: Thu, 11 Aug 2022 12:18:44 +0200 Subject: [PATCH 5/6] Rename SettingAutoWrapper, add comment to readBinary Co-authored-by: Azat Khuzhin Co-authored-by: Maksim Kita --- src/Core/Settings.h | 2 +- src/Core/SettingsFields.cpp | 6 ++--- src/Core/SettingsFields.h | 24 ++++++++++++------- .../02381_setting_value_auto.reference | 8 +++---- 4 files changed, 23 insertions(+), 17 deletions(-) diff --git a/src/Core/Settings.h b/src/Core/Settings.h index 727e45e3e50..f5108031dfb 100644 --- a/src/Core/Settings.h +++ b/src/Core/Settings.h @@ -212,7 +212,7 @@ static constexpr UInt64 operator""_GiB(unsigned long long value) \ M(Bool, insert_deduplicate, true, "For INSERT queries in the replicated table, specifies that deduplication of insertings blocks should be performed", 0) \ \ - M(UInt64WithAuto, insert_quorum, 0, "For INSERT queries in the replicated table, wait writing for the specified number of replicas and linearize the addition of the data. 0 - disabled.", 0) \ + M(UInt64Auto, insert_quorum, 0, "For INSERT queries in the replicated table, wait writing for the specified number of replicas and linearize the addition of the data. 0 - disabled.", 0) \ M(Milliseconds, insert_quorum_timeout, 600000, "", 0) \ M(Bool, insert_quorum_parallel, true, "For quorum INSERT queries - enable to make parallel inserts without linearizability", 0) \ M(UInt64, select_sequential_consistency, 0, "For SELECT queries from the replicated table, throw an exception if the replica does not have a chunk written with the quorum; do not read the parts that have not yet been written with the quorum.", 0) \ diff --git a/src/Core/SettingsFields.cpp b/src/Core/SettingsFields.cpp index 86b20da9e8c..5b1b6b10cc2 100644 --- a/src/Core/SettingsFields.cpp +++ b/src/Core/SettingsFields.cpp @@ -153,9 +153,9 @@ template struct SettingFieldNumber; template struct SettingFieldNumber; template struct SettingFieldNumber; -template struct SettingWithAuto>; -template struct SettingWithAuto>; -template struct SettingWithAuto>; +template struct SettingAutoWrapper>; +template struct SettingAutoWrapper>; +template struct SettingAutoWrapper>; namespace { diff --git a/src/Core/SettingsFields.h b/src/Core/SettingsFields.h index 0fcc1d6783f..68c6e85796e 100644 --- a/src/Core/SettingsFields.h +++ b/src/Core/SettingsFields.h @@ -66,7 +66,7 @@ using SettingFieldBool = SettingFieldNumber; * but when serializing 'auto' old version will see binary representation of the default value. */ template -struct SettingWithAuto +struct SettingAutoWrapper { constexpr static auto keyword = "auto"; static bool isAuto(const Field & f) { return f.getType() == Field::Types::String && f.safeGet() == keyword; } @@ -78,17 +78,17 @@ struct SettingWithAuto bool is_auto = false; bool changed = false; - explicit SettingWithAuto() : is_auto(true) {} - explicit SettingWithAuto(Type val) : is_auto(false) { base = Base(val); } + explicit SettingAutoWrapper() : is_auto(true) {} + explicit SettingAutoWrapper(Type val) : is_auto(false) { base = Base(val); } - explicit SettingWithAuto(const Field & f) + explicit SettingAutoWrapper(const Field & f) : is_auto(isAuto(f)) { if (!is_auto) base = Base(f); } - SettingWithAuto & operator=(const Field & f) + SettingAutoWrapper & operator=(const Field & f) { changed = true; if (is_auto = isAuto(f); !is_auto) @@ -115,16 +115,22 @@ struct SettingWithAuto base.writeBinary(out); } + /* + * That it is fine to reset `is_auto` here and to use default value in case `is_auto` + * because settings will be serialized only if changed. + * If they were changed they were requested to use explicit value instead of `auto`. + * And so interactions between client-server, and server-server (distributed queries), should be OK. + */ void readBinary(ReadBuffer & in) { changed = true; is_auto = false; base.readBinary(in); } Type valueOr(Type default_value) const { return is_auto ? default_value : base.value; } }; -using SettingFieldUInt64WithAuto = SettingWithAuto; -using SettingFieldInt64WithAuto = SettingWithAuto; -using SettingFieldFloatWithAuto = SettingWithAuto; +using SettingFieldUInt64Auto = SettingAutoWrapper; +using SettingFieldInt64Auto = SettingAutoWrapper; +using SettingFieldFloatAuto = SettingAutoWrapper; -/* Similar to SettingFieldUInt64WithAuto with small differences to behave like regular UInt64, supported to compatibility. +/* Similar to SettingFieldUInt64Auto with small differences to behave like regular UInt64, supported to compatibility. * When setting to 'auto' it becomes equal to the number of processor cores without taking into account SMT. * A value of 0 is also treated as 'auto', so 'auto' is parsed and serialized in the same way as 0. */ diff --git a/tests/queries/0_stateless/02381_setting_value_auto.reference b/tests/queries/0_stateless/02381_setting_value_auto.reference index 72c87cf6f7d..acc5025da5e 100644 --- a/tests/queries/0_stateless/02381_setting_value_auto.reference +++ b/tests/queries/0_stateless/02381_setting_value_auto.reference @@ -1,4 +1,4 @@ -0 0 UInt64WithAuto -auto 1 UInt64WithAuto -0 1 UInt64WithAuto -1 1 UInt64WithAuto +0 0 UInt64Auto +auto 1 UInt64Auto +0 1 UInt64Auto +1 1 UInt64Auto From 96776d30287914a3bc0eba38dfdf2965c9bbec70 Mon Sep 17 00:00:00 2001 From: Vladimir C Date: Thu, 11 Aug 2022 13:15:28 +0200 Subject: [PATCH 6/6] Trim trailing whitespaces in SettingsFields.h --- src/Core/SettingsFields.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Core/SettingsFields.h b/src/Core/SettingsFields.h index 68c6e85796e..f01ac37d3cc 100644 --- a/src/Core/SettingsFields.h +++ b/src/Core/SettingsFields.h @@ -115,7 +115,7 @@ struct SettingAutoWrapper base.writeBinary(out); } - /* + /* * That it is fine to reset `is_auto` here and to use default value in case `is_auto` * because settings will be serialized only if changed. * If they were changed they were requested to use explicit value instead of `auto`.