From eb62030fa425af8ef176e7f7f333b8e34fa4dc4f Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Wed, 10 May 2023 05:02:52 +0200 Subject: [PATCH 1/2] Fix assigning a setting to NULL in settings profile's definition. --- src/Access/SettingsConstraints.cpp | 12 ++++---- src/Access/SettingsProfileElement.cpp | 30 +++++++++---------- src/Access/SettingsProfileElement.h | 6 ++-- .../Access/ASTSettingsProfileElement.cpp | 12 ++++---- .../Access/ASTSettingsProfileElement.h | 6 ++-- .../Access/ParserSettingsProfileElement.cpp | 24 +++++++-------- .../StorageSystemSettingsProfileElements.cpp | 12 ++++---- 7 files changed, 51 insertions(+), 51 deletions(-) diff --git a/src/Access/SettingsConstraints.cpp b/src/Access/SettingsConstraints.cpp index e83ab264f4f..12f584cab83 100644 --- a/src/Access/SettingsConstraints.cpp +++ b/src/Access/SettingsConstraints.cpp @@ -105,21 +105,21 @@ void SettingsConstraints::check(const Settings & current_settings, const Setting if (SettingsProfileElements::isAllowBackupSetting(element.setting_name)) continue; - if (!element.value.isNull()) + if (element.value) { - SettingChange value(element.setting_name, element.value); + SettingChange value(element.setting_name, *element.value); check(current_settings, value); } - if (!element.min_value.isNull()) + if (element.min_value) { - SettingChange value(element.setting_name, element.min_value); + SettingChange value(element.setting_name, *element.min_value); check(current_settings, value); } - if (!element.max_value.isNull()) + if (element.max_value) { - SettingChange value(element.setting_name, element.max_value); + SettingChange value(element.setting_name, *element.max_value); check(current_settings, value); } diff --git a/src/Access/SettingsProfileElement.cpp b/src/Access/SettingsProfileElement.cpp index ce56782d887..9358391cb93 100644 --- a/src/Access/SettingsProfileElement.cpp +++ b/src/Access/SettingsProfileElement.cpp @@ -63,18 +63,18 @@ void SettingsProfileElement::init(const ASTSettingsProfileElement & ast, const A max_value = ast.max_value; writability = ast.writability; - if (!value.isNull()) - value = Settings::castValueUtil(setting_name, value); - if (!min_value.isNull()) - min_value = Settings::castValueUtil(setting_name, min_value); - if (!max_value.isNull()) - max_value = Settings::castValueUtil(setting_name, max_value); + if (value) + value = Settings::castValueUtil(setting_name, *value); + if (min_value) + min_value = Settings::castValueUtil(setting_name, *min_value); + if (max_value) + max_value = Settings::castValueUtil(setting_name, *max_value); } } bool SettingsProfileElement::isConstraint() const { - return this->writability || !this->min_value.isNull() || !this->max_value.isNull(); + return this->writability || this->min_value || this->max_value; } std::shared_ptr SettingsProfileElement::toAST() const @@ -187,8 +187,8 @@ Settings SettingsProfileElements::toSettings() const Settings res; for (const auto & elem : *this) { - if (!elem.setting_name.empty() && !isAllowBackupSetting(elem.setting_name) && !elem.value.isNull()) - res.set(elem.setting_name, elem.value); + if (!elem.setting_name.empty() && !isAllowBackupSetting(elem.setting_name) && elem.value) + res.set(elem.setting_name, *elem.value); } return res; } @@ -200,8 +200,8 @@ SettingsChanges SettingsProfileElements::toSettingsChanges() const { if (!elem.setting_name.empty() && !isAllowBackupSetting(elem.setting_name)) { - if (!elem.value.isNull()) - res.push_back({elem.setting_name, elem.value}); + if (elem.value) + res.push_back({elem.setting_name, *elem.value}); } } return res; @@ -214,8 +214,8 @@ SettingsConstraints SettingsProfileElements::toSettingsConstraints(const AccessC if (!elem.setting_name.empty() && elem.isConstraint() && !isAllowBackupSetting(elem.setting_name)) res.set( elem.setting_name, - elem.min_value, - elem.max_value, + elem.min_value ? *elem.min_value : Field{}, + elem.max_value ? *elem.max_value : Field{}, elem.writability ? *elem.writability : SettingConstraintWritability::WRITABLE); return res; } @@ -240,8 +240,8 @@ bool SettingsProfileElements::isBackupAllowed() const { for (const auto & setting : *this) { - if (isAllowBackupSetting(setting.setting_name)) - return static_cast(SettingFieldBool{setting.value}); + if (isAllowBackupSetting(setting.setting_name) && setting.value) + return static_cast(SettingFieldBool{*setting.value}); } return true; } diff --git a/src/Access/SettingsProfileElement.h b/src/Access/SettingsProfileElement.h index 7f9379c1e47..7078f565295 100644 --- a/src/Access/SettingsProfileElement.h +++ b/src/Access/SettingsProfileElement.h @@ -23,9 +23,9 @@ struct SettingsProfileElement std::optional parent_profile; String setting_name; - Field value; - Field min_value; - Field max_value; + std::optional value; + std::optional min_value; + std::optional max_value; std::optional writability; auto toTuple() const { return std::tie(parent_profile, setting_name, value, min_value, max_value, writability); } diff --git a/src/Parsers/Access/ASTSettingsProfileElement.cpp b/src/Parsers/Access/ASTSettingsProfileElement.cpp index 76973c428b2..7b29b15cb29 100644 --- a/src/Parsers/Access/ASTSettingsProfileElement.cpp +++ b/src/Parsers/Access/ASTSettingsProfileElement.cpp @@ -35,21 +35,21 @@ void ASTSettingsProfileElement::formatImpl(const FormatSettings & settings, Form formatSettingName(setting_name, settings.ostr); - if (!value.isNull()) + if (value) { - settings.ostr << " = " << applyVisitor(FieldVisitorToString{}, value); + settings.ostr << " = " << applyVisitor(FieldVisitorToString{}, *value); } - if (!min_value.isNull()) + if (min_value) { settings.ostr << (settings.hilite ? IAST::hilite_keyword : "") << " MIN " << (settings.hilite ? IAST::hilite_none : "") - << applyVisitor(FieldVisitorToString{}, min_value); + << applyVisitor(FieldVisitorToString{}, *min_value); } - if (!max_value.isNull()) + if (max_value) { settings.ostr << (settings.hilite ? IAST::hilite_keyword : "") << " MAX " << (settings.hilite ? IAST::hilite_none : "") - << applyVisitor(FieldVisitorToString{}, max_value); + << applyVisitor(FieldVisitorToString{}, *max_value); } if (writability) diff --git a/src/Parsers/Access/ASTSettingsProfileElement.h b/src/Parsers/Access/ASTSettingsProfileElement.h index 275257e4f8e..13c1926d9b0 100644 --- a/src/Parsers/Access/ASTSettingsProfileElement.h +++ b/src/Parsers/Access/ASTSettingsProfileElement.h @@ -14,9 +14,9 @@ class ASTSettingsProfileElement : public IAST public: String parent_profile; String setting_name; - Field value; - Field min_value; - Field max_value; + std::optional value; + std::optional min_value; + std::optional max_value; std::optional writability; bool id_mode = false; /// If true then `parent_profile` keeps UUID, not a name. bool use_inherit_keyword = false; /// If true then this element is a part of ASTCreateSettingsProfileQuery. diff --git a/src/Parsers/Access/ParserSettingsProfileElement.cpp b/src/Parsers/Access/ParserSettingsProfileElement.cpp index db23a806a12..36330b96622 100644 --- a/src/Parsers/Access/ParserSettingsProfileElement.cpp +++ b/src/Parsers/Access/ParserSettingsProfileElement.cpp @@ -52,7 +52,7 @@ namespace } - bool parseValue(IParserBase::Pos & pos, Expected & expected, Field & res) + bool parseValue(IParserBase::Pos & pos, Expected & expected, std::optional & res) { return IParserBase::wrapParseImpl(pos, [&] { @@ -69,7 +69,7 @@ namespace } - bool parseMinMaxValue(IParserBase::Pos & pos, Expected & expected, Field & min_value, Field & max_value) + bool parseMinMaxValue(IParserBase::Pos & pos, Expected & expected, std::optional & min_value, std::optional & max_value) { return IParserBase::wrapParseImpl(pos, [&] { @@ -124,9 +124,9 @@ namespace IParserBase::Pos & pos, Expected & expected, String & setting_name, - Field & value, - Field & min_value, - Field & max_value, + std::optional & value, + std::optional & min_value, + std::optional & max_value, std::optional & writability) { return IParserBase::wrapParseImpl(pos, [&] @@ -136,9 +136,9 @@ namespace return false; String res_setting_name = getIdentifierName(name_ast); - Field res_value; - Field res_min_value; - Field res_max_value; + std::optional res_value; + std::optional res_min_value; + std::optional res_max_value; std::optional res_writability; bool has_value_or_constraint = false; @@ -151,7 +151,7 @@ namespace if (!has_value_or_constraint) return false; - if (boost::iequals(res_setting_name, "PROFILE") && res_value.isNull() && res_min_value.isNull() && res_max_value.isNull() + if (boost::iequals(res_setting_name, "PROFILE") && !res_value && !res_min_value && !res_max_value && res_writability == SettingConstraintWritability::CONST) { /// Ambiguity: "profile readonly" can be treated either as a profile named "readonly" or @@ -181,9 +181,9 @@ namespace { String parent_profile; String setting_name; - Field value; - Field min_value; - Field max_value; + std::optional value; + std::optional min_value; + std::optional max_value; std::optional writability; bool ok = parseSettingNameWithValueOrConstraints(pos, expected, setting_name, value, min_value, max_value, writability); diff --git a/src/Storages/System/StorageSystemSettingsProfileElements.cpp b/src/Storages/System/StorageSystemSettingsProfileElements.cpp index 6785a4392e1..e01d3cb0ace 100644 --- a/src/Storages/System/StorageSystemSettingsProfileElements.cpp +++ b/src/Storages/System/StorageSystemSettingsProfileElements.cpp @@ -87,27 +87,27 @@ void StorageSystemSettingsProfileElements::fillData(MutableColumns & res_columns size_t current_index = index++; bool inserted_value = false; - if (!element.value.isNull() && !element.setting_name.empty()) + if (element.value && !element.setting_name.empty()) { - String str = Settings::valueToStringUtil(element.setting_name, element.value); + String str = Settings::valueToStringUtil(element.setting_name, *element.value); column_value.insertData(str.data(), str.length()); column_value_null_map.push_back(false); inserted_value = true; } bool inserted_min = false; - if (!element.min_value.isNull() && !element.setting_name.empty()) + if (element.min_value && !element.setting_name.empty()) { - String str = Settings::valueToStringUtil(element.setting_name, element.min_value); + String str = Settings::valueToStringUtil(element.setting_name, *element.min_value); column_min.insertData(str.data(), str.length()); column_min_null_map.push_back(false); inserted_min = true; } bool inserted_max = false; - if (!element.max_value.isNull() && !element.setting_name.empty()) + if (element.max_value && !element.setting_name.empty()) { - String str = Settings::valueToStringUtil(element.setting_name, element.max_value); + String str = Settings::valueToStringUtil(element.setting_name, *element.max_value); column_max.insertData(str.data(), str.length()); column_max_null_map.push_back(false); inserted_max = true; From a4694ac1858890bcd3d35547cf5c4417252933c9 Mon Sep 17 00:00:00 2001 From: Vitaly Baranov Date: Fri, 12 May 2023 12:29:29 +0200 Subject: [PATCH 2/2] Add test. --- .../01418_custom_settings.reference | 16 +++++++++---- .../0_stateless/01418_custom_settings.sql | 24 +++++++++++++++---- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/tests/queries/0_stateless/01418_custom_settings.reference b/tests/queries/0_stateless/01418_custom_settings.reference index cf0cb35c72a..8484a5d0e6f 100644 --- a/tests/queries/0_stateless/01418_custom_settings.reference +++ b/tests/queries/0_stateless/01418_custom_settings.reference @@ -1,3 +1,4 @@ +--- assigning --- 5 UInt8 -177 Int16 98.11 Float64 @@ -6,7 +7,7 @@ custom_a UInt64_5 custom_b Int64_-177 custom_c Float64_98.11 custom_d \'abc def\' - +--- modifying --- changed String \N Nullable(Nothing) 50000 UInt16 @@ -15,9 +16,10 @@ custom_a \'changed\' custom_b NULL custom_c UInt64_50000 custom_d Float64_1.11 - +--- undefined setting --- 404 UInt16 - +--- wrong prefix --- +--- using query context --- -0.333 Float64 custom_e Float64_-0.333 404 UInt16 @@ -25,7 +27,13 @@ custom_e UInt64_404 word String custom_f \'word\' 0 - +--- compound identifier --- test String custom_compound.identifier.v1 \'test\' CREATE SETTINGS PROFILE s1_01418 SETTINGS custom_compound.identifier.v2 = 100 +--- null type --- +\N Nullable(Nothing) +custom_null NULL +\N Nullable(Nothing) +custom_null NULL +CREATE SETTINGS PROFILE s2_01418 SETTINGS custom_null = NULL diff --git a/tests/queries/0_stateless/01418_custom_settings.sql b/tests/queries/0_stateless/01418_custom_settings.sql index 95051db3a34..be18f553589 100644 --- a/tests/queries/0_stateless/01418_custom_settings.sql +++ b/tests/queries/0_stateless/01418_custom_settings.sql @@ -1,3 +1,6 @@ +DROP SETTINGS PROFILE IF EXISTS s1_01418, s2_01418; + +SELECT '--- assigning ---'; SET custom_a = 5; SET custom_b = -177; SET custom_c = 98.11; @@ -8,7 +11,7 @@ SELECT getSetting('custom_c') as v, toTypeName(v); SELECT getSetting('custom_d') as v, toTypeName(v); SELECT name, value FROM system.settings WHERE name LIKE 'custom_%' ORDER BY name; -SELECT ''; +SELECT '--- modifying ---'; SET custom_a = 'changed'; SET custom_b = NULL; SET custom_c = 50000; @@ -19,14 +22,15 @@ SELECT getSetting('custom_c') as v, toTypeName(v); SELECT getSetting('custom_d') as v, toTypeName(v); SELECT name, value FROM system.settings WHERE name LIKE 'custom_%' ORDER BY name; -SELECT ''; +SELECT '--- undefined setting ---'; SELECT getSetting('custom_e') as v, toTypeName(v); -- { serverError 115 } -- Setting not found. SET custom_e = 404; SELECT getSetting('custom_e') as v, toTypeName(v); +SELECT '--- wrong prefix ---'; SET invalid_custom = 8; -- { serverError 115 } -- Setting is neither a builtin nor started with one of the registered prefixes for user-defined settings. -SELECT ''; +SELECT '--- using query context ---'; SELECT getSetting('custom_e') as v, toTypeName(v) SETTINGS custom_e = -0.333; SELECT name, value FROM system.settings WHERE name = 'custom_e' SETTINGS custom_e = -0.333; SELECT getSetting('custom_e') as v, toTypeName(v); @@ -37,7 +41,7 @@ SELECT name, value FROM system.settings WHERE name = 'custom_f' SETTINGS custom_ SELECT getSetting('custom_f') as v, toTypeName(v); -- { serverError 115 } -- Setting not found. SELECT COUNT() FROM system.settings WHERE name = 'custom_f'; -SELECT ''; +SELECT '--- compound identifier ---'; SET custom_compound.identifier.v1 = 'test'; SELECT getSetting('custom_compound.identifier.v1') as v, toTypeName(v); SELECT name, value FROM system.settings WHERE name = 'custom_compound.identifier.v1'; @@ -45,3 +49,15 @@ SELECT name, value FROM system.settings WHERE name = 'custom_compound.identifier CREATE SETTINGS PROFILE s1_01418 SETTINGS custom_compound.identifier.v2 = 100; SHOW CREATE SETTINGS PROFILE s1_01418; DROP SETTINGS PROFILE s1_01418; + +SELECT '--- null type ---'; +SELECT getSetting('custom_null') as v, toTypeName(v) SETTINGS custom_null = NULL; +SELECT name, value FROM system.settings WHERE name = 'custom_null' SETTINGS custom_null = NULL; + +SET custom_null = NULL; +SELECT getSetting('custom_null') as v, toTypeName(v); +SELECT name, value FROM system.settings WHERE name = 'custom_null'; + +CREATE SETTINGS PROFILE s2_01418 SETTINGS custom_null = NULL; +SHOW CREATE SETTINGS PROFILE s2_01418; +DROP SETTINGS PROFILE s2_01418;