From 66175cb9c8260ac3d956477c6047f0ab58c8e363 Mon Sep 17 00:00:00 2001 From: kssenii Date: Fri, 18 Nov 2022 13:00:05 +0100 Subject: [PATCH] Not better --- src/Backups/BackupSettings.cpp | 4 +- src/Common/SettingsChanges.h | 6 +- .../getDictionaryConfigurationFromAST.cpp | 2 +- src/Disks/getDiskConfigurationFromAST.cpp | 93 --------------- src/Disks/getDiskConfigurationFromAST.h | 28 ----- .../maskSensitiveInfoInQueryForLogging.cpp | 31 +++++ src/Parsers/ASTSetQuery.cpp | 10 +- src/Parsers/ParserSetQuery.cpp | 40 ++++--- .../MergeTree/registerStorageMergeTree.cpp | 34 +++--- src/Storages/createDiskFromDiskAST.cpp | 74 ------------ src/Storages/createDiskFromDiskAST.h | 23 ---- src/Storages/getOrCreateDiskFromSettings.cpp | 112 ++++++++++++++++++ src/Storages/getOrCreateDiskFromSettings.h | 21 ++++ 13 files changed, 214 insertions(+), 264 deletions(-) delete mode 100644 src/Disks/getDiskConfigurationFromAST.cpp delete mode 100644 src/Disks/getDiskConfigurationFromAST.h delete mode 100644 src/Storages/createDiskFromDiskAST.cpp delete mode 100644 src/Storages/createDiskFromDiskAST.h create mode 100644 src/Storages/getOrCreateDiskFromSettings.cpp create mode 100644 src/Storages/getOrCreateDiskFromSettings.h diff --git a/src/Backups/BackupSettings.cpp b/src/Backups/BackupSettings.cpp index d297859f1f4..295ab723326 100644 --- a/src/Backups/BackupSettings.cpp +++ b/src/Backups/BackupSettings.cpp @@ -82,8 +82,8 @@ BackupSettings BackupSettings::fromBackupQuery(const ASTBackupQuery & query) const auto & settings = query.settings->as().changes; for (const auto & setting : settings) { - if (setting.getName() == "compression_level") - res.compression_level = static_cast(SettingFieldInt64{setting.getFieldValue()}.value); + if (setting.name == "compression_level") + res.compression_level = static_cast(SettingFieldInt64{setting.value}.value); else #define GET_SETTINGS_FROM_BACKUP_QUERY_HELPER(TYPE, NAME) \ if (setting.name == #NAME) \ diff --git a/src/Common/SettingsChanges.h b/src/Common/SettingsChanges.h index 10516cfabd4..9e7f874d843 100644 --- a/src/Common/SettingsChanges.h +++ b/src/Common/SettingsChanges.h @@ -14,16 +14,12 @@ struct SettingChange String name; Field value; - /// A setting value which cannot be put in Field. - ASTPtr value_ast; - SettingChange() = default; SettingChange(std::string_view name_, const Field & value_) : name(name_), value(value_) {} SettingChange(std::string_view name_, Field && value_) : name(name_), value(std::move(value_)) {} - SettingChange(std::string_view name_, const ASTPtr & value_) : name(name_), value_ast(value_->clone()) {} - friend bool operator ==(const SettingChange & lhs, const SettingChange & rhs) { return (lhs.name == rhs.name) && (lhs.value == rhs.value) && (lhs.value_ast == rhs.value_ast); } + friend bool operator ==(const SettingChange & lhs, const SettingChange & rhs) { return (lhs.name == rhs.name) && (lhs.value == rhs.value); } friend bool operator !=(const SettingChange & lhs, const SettingChange & rhs) { return !(lhs == rhs); } }; diff --git a/src/Dictionaries/getDictionaryConfigurationFromAST.cpp b/src/Dictionaries/getDictionaryConfigurationFromAST.cpp index f8d5f9d61db..4868413dabd 100644 --- a/src/Dictionaries/getDictionaryConfigurationFromAST.cpp +++ b/src/Dictionaries/getDictionaryConfigurationFromAST.cpp @@ -511,7 +511,7 @@ void buildSourceConfiguration( { AutoPtr settings_element(doc->createElement("settings")); outer_element->appendChild(settings_element); - for (const auto & [name, value, _] : settings->changes) + for (const auto & [name, value] : settings->changes) { AutoPtr setting_change_element(doc->createElement(name)); settings_element->appendChild(setting_change_element); diff --git a/src/Disks/getDiskConfigurationFromAST.cpp b/src/Disks/getDiskConfigurationFromAST.cpp deleted file mode 100644 index 628defe56ef..00000000000 --- a/src/Disks/getDiskConfigurationFromAST.cpp +++ /dev/null @@ -1,93 +0,0 @@ -#include - -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - - -namespace DB -{ - -namespace ErrorCodes -{ - extern const int BAD_ARGUMENTS; -} - -[[noreturn]] static void throwBadConfiguration(const std::string & message = "") -{ - throw Exception( - ErrorCodes::BAD_ARGUMENTS, - "Incorrect configuration{}. Example of expected configuration: `(type=s3 ...`)`", - message.empty() ? "" : ": " + message); -} - -Poco::AutoPtr getDiskConfigurationFromASTImpl(const std::string & root_name, const ASTs & disk_args, ContextPtr context) -{ - if (disk_args.empty()) - throwBadConfiguration("expected non-empty list of arguments"); - - Poco::AutoPtr xml_document(new Poco::XML::Document()); - Poco::AutoPtr root(xml_document->createElement("disk")); - xml_document->appendChild(root); - Poco::AutoPtr disk_configuration(xml_document->createElement(root_name)); - root->appendChild(disk_configuration); - - for (const auto & arg : disk_args) - { - const auto * setting_function = arg->as(); - if (!setting_function || setting_function->name != "equals") - throwBadConfiguration("expected configuration arguments as key=value pairs"); - - const auto * function_args_expr = assert_cast(setting_function->arguments.get()); - if (!function_args_expr) - throwBadConfiguration("expected a list of key=value arguments"); - - auto function_args = function_args_expr->children; - if (function_args.empty()) - throwBadConfiguration("expected a non-empty list of key=value arguments"); - - auto * key_identifier = function_args[0]->as(); - if (!key_identifier) - throwBadConfiguration("expected the key (key=value) to be identifier"); - - const std::string & key = key_identifier->name(); - Poco::AutoPtr key_element(xml_document->createElement(key)); - disk_configuration->appendChild(key_element); - - if (!function_args[1]->as() && !function_args[1]->as()) - throwBadConfiguration("expected values to be literals or identifiers"); - - auto value = evaluateConstantExpressionOrIdentifierAsLiteral(function_args[1], context); - Poco::AutoPtr value_element(xml_document->createTextNode(convertFieldToString(value->as()->value))); - key_element->appendChild(value_element); - } - - return xml_document; -} - -DiskConfigurationPtr getDiskConfigurationFromAST(const std::string & root_name, const ASTs & disk_args, ContextPtr context) -{ - auto xml_document = getDiskConfigurationFromASTImpl(root_name, disk_args, context); - - Poco::AutoPtr conf(new Poco::Util::XMLConfiguration()); - conf->load(xml_document); - - std::ostringstream ss; // STYLE_CHECK_ALLOW_STD_STRING_STREAM - ss.exceptions(std::ios::failbit); - conf->save(ss); - LOG_TEST(&Poco::Logger::get("getDiskConfigurationFromAST"), "Received disk configuration: {}", ss.str()); - - return conf; -} - -} diff --git a/src/Disks/getDiskConfigurationFromAST.h b/src/Disks/getDiskConfigurationFromAST.h deleted file mode 100644 index 1f9d7c1bfe6..00000000000 --- a/src/Disks/getDiskConfigurationFromAST.h +++ /dev/null @@ -1,28 +0,0 @@ -#pragma once - -#include -#include -#include -#include -#include - -namespace DB -{ - -using DiskConfigurationPtr = Poco::AutoPtr; - -/** - * Transform a list of pairs ( key1=value1, key2=value2, ... ), where keys and values are ASTLiteral or ASTIdentifier - * into - * - * value1 - * value2 - * ... - * - * - * Used in case disk configuration is passed via AST when creating - * a disk object on-the-fly without any configuration file. - */ -DiskConfigurationPtr getDiskConfigurationFromAST(const std::string & root_name, const ASTs & disk_args, ContextPtr context); - -} diff --git a/src/Interpreters/maskSensitiveInfoInQueryForLogging.cpp b/src/Interpreters/maskSensitiveInfoInQueryForLogging.cpp index fe05283eef5..506c3b1e73c 100644 --- a/src/Interpreters/maskSensitiveInfoInQueryForLogging.cpp +++ b/src/Interpreters/maskSensitiveInfoInQueryForLogging.cpp @@ -169,6 +169,10 @@ namespace /// S3('url', ['aws_access_key_id', 'aws_secret_access_key',] ...) wipePasswordFromS3TableEngineArguments(*storage.engine, data); } + else if (storage.engine->arguments && storage.settings) + { + wipeDiskSettingsArguments(*storage.settings, data); + } } static void wipePasswordFromS3TableEngineArguments(ASTFunction & engine, Data & data) @@ -387,6 +391,33 @@ namespace data.password_was_hidden = true; } + static void wipeDiskSettingsArguments(ASTSetQuery & settings, Data & data) + { + if (settings.changes.empty()) + return; + + for (auto & setting : settings.changes) + { + if (setting.name != "disk") + continue; + + if constexpr (check_only) + { + data.can_contain_password = true; + return; + } + + if (setting.value.getType() != Field::Types::Which::Array) + continue; + + for (auto & disk_setting : setting.value.safeGet()) + disk_setting = Tuple({disk_setting.safeGet()[0], "[HIDDEN]"}); + + data.password_was_hidden = true; + return; + } + } + static void removeArgumentsAfter(ASTFunction & function, Data & data, size_t new_num_arguments) { if (!function.arguments) diff --git a/src/Parsers/ASTSetQuery.cpp b/src/Parsers/ASTSetQuery.cpp index cf4853ba434..26420f4988c 100644 --- a/src/Parsers/ASTSetQuery.cpp +++ b/src/Parsers/ASTSetQuery.cpp @@ -19,7 +19,7 @@ void ASTSetQuery::updateTreeHashImpl(SipHash & hash_state) const } } -void ASTSetQuery::formatImpl(const FormatSettings & format, FormatState & state, FormatStateStacked stacked) const +void ASTSetQuery::formatImpl(const FormatSettings & format, FormatState &, FormatStateStacked) const { if (is_standalone) format.ostr << (format.hilite ? hilite_keyword : "") << "SET " << (format.hilite ? hilite_none : ""); @@ -34,13 +34,7 @@ void ASTSetQuery::formatImpl(const FormatSettings & format, FormatState & state, first = false; formatSettingName(change.name, format.ostr); - if (change.value_ast) - { - format.ostr << " = "; - change.value_ast->formatImpl(format, state, stacked); - } - else - format.ostr << " = " << applyVisitor(FieldVisitorToString(), change.value); + format.ostr << " = " << applyVisitor(FieldVisitorToString(), change.value); } for (const auto & setting_name : default_settings) diff --git a/src/Parsers/ParserSetQuery.cpp b/src/Parsers/ParserSetQuery.cpp index 19351f37385..bc9e5225a5e 100644 --- a/src/Parsers/ParserSetQuery.cpp +++ b/src/Parsers/ParserSetQuery.cpp @@ -13,6 +13,7 @@ #include #include #include +#include namespace DB { @@ -98,11 +99,11 @@ bool ParserSetQuery::parseNameValuePair(SettingChange & change, IParser::Pos & p ParserLiteralOrMap literal_or_map_p; ParserToken s_eq(TokenType::Equals); ParserSetQuery set_p(true); - ParserFunction function_p; + ParserToken l_br(TokenType::OpeningRoundBracket); + ParserToken r_br(TokenType::ClosingRoundBracket); ASTPtr name; ASTPtr value; - ASTPtr function_ast; if (!name_p.parse(pos, name, expected)) return false; @@ -114,12 +115,15 @@ bool ParserSetQuery::parseNameValuePair(SettingChange & change, IParser::Pos & p value = std::make_shared(Field(static_cast(1))); else if (ParserKeyword("FALSE").ignore(pos, expected)) value = std::make_shared(Field(static_cast(0))); - /// for SETTINGS disk=disk(type='s3', path='', ...) - else if (function_p.parse(pos, function_ast, expected) && function_ast->as()->name == "disk") + else if (ParserKeyword("SETTINGS").ignore(pos, expected) + && l_br.ignore(pos, expected) + && set_p.parse(pos, value, expected) + && r_br.ignore(pos, expected)) { tryGetIdentifierNameInto(name, change.name); - change.value_ast = function_ast; - + auto changes = value->as()->changes; + auto values = changes | std::views::transform([](const auto & setting) { return Tuple{setting.name, setting.value}; }); + change.value = Array(values.begin(), values.end()); return true; } else if (!literal_or_map_p.parse(pos, value, expected)) @@ -136,12 +140,13 @@ bool ParserSetQuery::parseNameValuePairWithDefault(SettingChange & change, Strin ParserCompoundIdentifier name_p; ParserLiteralOrMap value_p; ParserToken s_eq(TokenType::Equals); - ParserFunction function_p; + ParserSetQuery set_p(true); + ParserToken l_br(TokenType::OpeningRoundBracket); + ParserToken r_br(TokenType::ClosingRoundBracket); ASTPtr name; ASTPtr value; bool is_default = false; - ASTPtr function_ast; if (!name_p.parse(pos, name, expected)) return false; @@ -155,22 +160,25 @@ bool ParserSetQuery::parseNameValuePairWithDefault(SettingChange & change, Strin value = std::make_shared(Field(static_cast(0))); else if (ParserKeyword("DEFAULT").ignore(pos, expected)) is_default = true; - /// for SETTINGS disk=disk(type='s3', path='', ...) - else if (function_p.parse(pos, function_ast, expected) && function_ast->as()->name == "disk") + else if (ParserKeyword("SETTINGS").ignore(pos, expected) + && l_br.ignore(pos, expected) + && set_p.parse(pos, value, expected) + && r_br.ignore(pos, expected)) { - tryGetIdentifierNameInto(name, change.getName()); - change.setASTValue(function_ast); - + tryGetIdentifierNameInto(name, change.name); + auto changes = value->as()->changes; + auto values = changes | std::views::transform([](const auto & setting) { return Tuple{setting.name, setting.value}; }); + change.value = Array(values.begin(), values.end()); return true; } else if (!value_p.parse(pos, value, expected)) return false; - tryGetIdentifierNameInto(name, change.getName()); + tryGetIdentifierNameInto(name, change.name); if (is_default) - default_settings = change.getName(); + default_settings = change.name; else - change.setFieldValue(value->as().value); + change.value = value->as().value; return true; } diff --git a/src/Storages/MergeTree/registerStorageMergeTree.cpp b/src/Storages/MergeTree/registerStorageMergeTree.cpp index 4fc47c1cc11..e8c9e283b9f 100644 --- a/src/Storages/MergeTree/registerStorageMergeTree.cpp +++ b/src/Storages/MergeTree/registerStorageMergeTree.cpp @@ -5,7 +5,7 @@ #include #include #include -#include +#include #include #include @@ -24,6 +24,7 @@ #include #include #include +#include namespace DB @@ -607,21 +608,26 @@ static StoragePtr create(const StorageFactory::Arguments & args) metadata.column_ttls_by_name[name] = new_ttl_entry; } - if (args.storage_def->settings) - { - for (auto & [_, value, value_ast] : args.storage_def->settings->changes) - { - if (isDiskFunction(value_ast)) - { - value = createDiskFromDiskAST(*value_ast->as(), context); - break; - } - } - } - storage_settings->loadFromQuery(*args.storage_def); - // updates the default storage_settings with settings specified via SETTINGS arg in a query + /** + * If settings contain disk setting as disk=setting(), + * create a disk from this configuration and substitute disk setting with result disk name. + */ + Field disk_value; + if (storage_settings->tryGet("disk", disk_value)) + { + auto disk_settings = disk_value.safeGet() | std::views::transform([](const Field & field) + { + auto setting = field.safeGet(); + return SettingChange(setting[0].safeGet(), setting[1]); + }); + auto changes = SettingsChanges(disk_settings.begin(), disk_settings.end()); + auto disk_name = getOrCreateDiskFromSettings(changes, context); + storage_settings->set("disk", disk_name); + } + + /// Updates the default storage_settings with settings specified via SETTINGS arg in a query if (args.storage_def->settings) metadata.settings_changes = args.storage_def->settings->ptr(); } diff --git a/src/Storages/createDiskFromDiskAST.cpp b/src/Storages/createDiskFromDiskAST.cpp deleted file mode 100644 index 9abbb5c0a9b..00000000000 --- a/src/Storages/createDiskFromDiskAST.cpp +++ /dev/null @@ -1,74 +0,0 @@ -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - -namespace DB -{ - -namespace ErrorCodes -{ - extern const int BAD_ARGUMENTS; -} - -bool isDiskFunction(ASTPtr ast) -{ - if (!ast) - return false; - - const auto * function = ast->as(); - return function && function->name == "disk" && function->arguments->as(); -} - -std::string createDiskFromDiskAST(const ASTFunction & function, ContextPtr context) -{ - /// We need a unique name for a created custom disk, but it needs to be the same - /// after table is reattached or server is restarted, so take a hash of the disk - /// configuration serialized ast as a disk name suffix. - auto disk_setting_string = serializeAST(function, true); - auto disk_name = DiskSelector::TMP_DISK_PREFIX - + toString(sipHash128(disk_setting_string.data(), disk_setting_string.size())); - - LOG_TRACE( - &Poco::Logger::get("createDiskFromDiskAST"), - "Using disk name `{}` for custom disk {}", - disk_name, disk_setting_string); - - auto result_disk = context->getOrCreateDisk(disk_name, [&](const DisksMap & disks_map) -> DiskPtr { - const auto * function_args_expr = assert_cast(function.arguments.get()); - const auto & function_args = function_args_expr->children; - auto config = getDiskConfigurationFromAST(disk_name, function_args, context); - auto disk = DiskFactory::instance().create(disk_name, *config, disk_name, context, disks_map); - /// Mark that disk can be used without storage policy. - disk->markDiskAsCustom(); - return disk; - }); - - if (!result_disk->isRemote()) - { - static constexpr auto custom_disks_base_dir_in_config = "custom_local_disks_base_directory"; - auto disk_path_expected_prefix = context->getConfigRef().getString(custom_disks_base_dir_in_config, ""); - - if (disk_path_expected_prefix.empty()) - throw Exception( - ErrorCodes::BAD_ARGUMENTS, - "Base path for custom local disks must be defined in config file by `{}`", - custom_disks_base_dir_in_config); - - if (!pathStartsWith(result_disk->getPath(), disk_path_expected_prefix)) - throw Exception( - ErrorCodes::BAD_ARGUMENTS, - "Path of the custom local disk must be inside `{}` directory", - disk_path_expected_prefix); - } - - return disk_name; -} - -} diff --git a/src/Storages/createDiskFromDiskAST.h b/src/Storages/createDiskFromDiskAST.h deleted file mode 100644 index 6047a494a6f..00000000000 --- a/src/Storages/createDiskFromDiskAST.h +++ /dev/null @@ -1,23 +0,0 @@ -#pragma once -#include -#include -#include - -namespace DB -{ - -class ASTFunction; - -/** - * Create a DiskPtr from disk AST function like disk(), - * add it to DiskSelector by a unique (but always the same for given configuration) disk name - * and return this name. - */ -std::string createDiskFromDiskAST(const ASTFunction & function, ContextPtr context); - -/* - * Is given ast has form of a disk() function. - */ -bool isDiskFunction(ASTPtr ast); - -} diff --git a/src/Storages/getOrCreateDiskFromSettings.cpp b/src/Storages/getOrCreateDiskFromSettings.cpp new file mode 100644 index 00000000000..7385a2905c6 --- /dev/null +++ b/src/Storages/getOrCreateDiskFromSettings.cpp @@ -0,0 +1,112 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + + +namespace DB +{ + +namespace ErrorCodes +{ + extern const int BAD_ARGUMENTS; +} + +namespace +{ + using DiskSettings = std::vector>; + + Poco::AutoPtr getDiskConfigurationFromSettingsImpl(const std::string & root_name, const DiskSettings & settings) + { + Poco::AutoPtr xml_document(new Poco::XML::Document()); + Poco::AutoPtr root(xml_document->createElement("disk")); + xml_document->appendChild(root); + Poco::AutoPtr disk_configuration(xml_document->createElement(root_name)); + root->appendChild(disk_configuration); + + for (const auto & [name, value] : settings) + { + Poco::AutoPtr key_element(xml_document->createElement(name)); + disk_configuration->appendChild(key_element); + + Poco::AutoPtr value_element(xml_document->createTextNode(value)); + key_element->appendChild(value_element); + } + + return xml_document; + } + + DiskConfigurationPtr getDiskConfigurationFromSettings(const std::string & root_name, const DiskSettings & settings) + { + auto xml_document = getDiskConfigurationFromSettingsImpl(root_name, settings); + Poco::AutoPtr conf(new Poco::Util::XMLConfiguration()); + conf->load(xml_document); + return conf; + } +} + +std::string getOrCreateDiskFromSettings(const SettingsChanges & configuration, ContextPtr context) +{ + /// We need a unique name for a created custom disk, but it needs to be the same + /// after table is reattached or server is restarted, so take a hash of the disk + /// configuration serialized ast as a disk name suffix. + auto settings_range = configuration + | std::views::transform( + [](const auto & setting) { return std::pair(setting.name, convertFieldToString(setting.value)); }); + + DiskSettings sorted_settings(settings_range.begin(), settings_range.end()); + std::sort(sorted_settings.begin(), sorted_settings.end()); + + std::string disk_setting_string; + for (auto it = sorted_settings.begin(); it != sorted_settings.end(); ++it) + { + if (it != sorted_settings.begin()) + disk_setting_string += ","; + disk_setting_string += std::get<0>(*it) + '=' + std::get<1>(*it); + } + + auto disk_name = DiskSelector::TMP_DISK_PREFIX + + toString(sipHash128(disk_setting_string.data(), disk_setting_string.size())); + + LOG_TRACE( + &Poco::Logger::get("getOrCreateDiskFromSettings"), + "Using disk name `{}` for custom disk {}", + disk_name, disk_setting_string); + + auto result_disk = context->getOrCreateDisk(disk_name, [&](const DisksMap & disks_map) -> DiskPtr { + auto config = getDiskConfigurationFromSettings(disk_name, sorted_settings); + auto disk = DiskFactory::instance().create(disk_name, *config, disk_name, context, disks_map); + /// Mark that disk can be used without storage policy. + disk->markDiskAsCustom(); + return disk; + }); + + if (!result_disk->isRemote()) + { + static constexpr auto custom_disks_base_dir_in_config = "custom_local_disks_base_directory"; + auto disk_path_expected_prefix = context->getConfigRef().getString(custom_disks_base_dir_in_config, ""); + + if (disk_path_expected_prefix.empty()) + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "Base path for custom local disks must be defined in config file by `{}`", + custom_disks_base_dir_in_config); + + if (!pathStartsWith(result_disk->getPath(), disk_path_expected_prefix)) + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "Path of the custom local disk must be inside `{}` directory", + disk_path_expected_prefix); + } + + return disk_name; +} + +} diff --git a/src/Storages/getOrCreateDiskFromSettings.h b/src/Storages/getOrCreateDiskFromSettings.h new file mode 100644 index 00000000000..6c800825ad7 --- /dev/null +++ b/src/Storages/getOrCreateDiskFromSettings.h @@ -0,0 +1,21 @@ +#pragma once + +#include +#include +#include + +namespace DB +{ + + +class SettingsChanges; +using DiskConfigurationPtr = Poco::AutoPtr; + +/** + * Create a DiskPtr from key value list of disk settings. + * add it to DiskSelector by a unique (but always the same for given configuration) disk name + * and return this name. + */ +std::string getOrCreateDiskFromSettings(const SettingsChanges & configuration, ContextPtr context); + +}