From babc0c84b08e5d399b52a7f8765dc7e6aca093d4 Mon Sep 17 00:00:00 2001 From: kssenii Date: Fri, 18 Nov 2022 13:00:46 +0100 Subject: [PATCH] Revert "Not better" This reverts commit 66175cb9c8260ac3d956477c6047f0ab58c8e363. --- 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 | 30 ++--- src/Storages/createDiskFromDiskAST.cpp | 74 ++++++++++++ src/Storages/createDiskFromDiskAST.h | 23 ++++ src/Storages/getOrCreateDiskFromSettings.cpp | 112 ------------------ src/Storages/getOrCreateDiskFromSettings.h | 21 ---- 13 files changed, 262 insertions(+), 212 deletions(-) create mode 100644 src/Disks/getDiskConfigurationFromAST.cpp create mode 100644 src/Disks/getDiskConfigurationFromAST.h create mode 100644 src/Storages/createDiskFromDiskAST.cpp create mode 100644 src/Storages/createDiskFromDiskAST.h delete mode 100644 src/Storages/getOrCreateDiskFromSettings.cpp delete mode 100644 src/Storages/getOrCreateDiskFromSettings.h diff --git a/src/Backups/BackupSettings.cpp b/src/Backups/BackupSettings.cpp index 295ab723326..d297859f1f4 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.name == "compression_level") - res.compression_level = static_cast(SettingFieldInt64{setting.value}.value); + if (setting.getName() == "compression_level") + res.compression_level = static_cast(SettingFieldInt64{setting.getFieldValue()}.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 9e7f874d843..10516cfabd4 100644 --- a/src/Common/SettingsChanges.h +++ b/src/Common/SettingsChanges.h @@ -14,12 +14,16 @@ 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); } + 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 == rhs); } }; diff --git a/src/Dictionaries/getDictionaryConfigurationFromAST.cpp b/src/Dictionaries/getDictionaryConfigurationFromAST.cpp index 4868413dabd..f8d5f9d61db 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 new file mode 100644 index 00000000000..628defe56ef --- /dev/null +++ b/src/Disks/getDiskConfigurationFromAST.cpp @@ -0,0 +1,93 @@ +#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 new file mode 100644 index 00000000000..1f9d7c1bfe6 --- /dev/null +++ b/src/Disks/getDiskConfigurationFromAST.h @@ -0,0 +1,28 @@ +#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 506c3b1e73c..fe05283eef5 100644 --- a/src/Interpreters/maskSensitiveInfoInQueryForLogging.cpp +++ b/src/Interpreters/maskSensitiveInfoInQueryForLogging.cpp @@ -169,10 +169,6 @@ 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) @@ -391,33 +387,6 @@ 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 26420f4988c..cf4853ba434 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 &, FormatStateStacked) const +void ASTSetQuery::formatImpl(const FormatSettings & format, FormatState & state, FormatStateStacked stacked) const { if (is_standalone) format.ostr << (format.hilite ? hilite_keyword : "") << "SET " << (format.hilite ? hilite_none : ""); @@ -34,7 +34,13 @@ void ASTSetQuery::formatImpl(const FormatSettings & format, FormatState &, Forma first = false; formatSettingName(change.name, format.ostr); - format.ostr << " = " << applyVisitor(FieldVisitorToString(), change.value); + if (change.value_ast) + { + format.ostr << " = "; + change.value_ast->formatImpl(format, state, stacked); + } + else + 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 bc9e5225a5e..19351f37385 100644 --- a/src/Parsers/ParserSetQuery.cpp +++ b/src/Parsers/ParserSetQuery.cpp @@ -13,7 +13,6 @@ #include #include #include -#include namespace DB { @@ -99,11 +98,11 @@ bool ParserSetQuery::parseNameValuePair(SettingChange & change, IParser::Pos & p ParserLiteralOrMap literal_or_map_p; ParserToken s_eq(TokenType::Equals); ParserSetQuery set_p(true); - ParserToken l_br(TokenType::OpeningRoundBracket); - ParserToken r_br(TokenType::ClosingRoundBracket); + ParserFunction function_p; ASTPtr name; ASTPtr value; + ASTPtr function_ast; if (!name_p.parse(pos, name, expected)) return false; @@ -115,15 +114,12 @@ 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))); - else if (ParserKeyword("SETTINGS").ignore(pos, expected) - && l_br.ignore(pos, expected) - && set_p.parse(pos, value, expected) - && r_br.ignore(pos, expected)) + /// for SETTINGS disk=disk(type='s3', path='', ...) + else if (function_p.parse(pos, function_ast, expected) && function_ast->as()->name == "disk") { 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()); + change.value_ast = function_ast; + return true; } else if (!literal_or_map_p.parse(pos, value, expected)) @@ -140,13 +136,12 @@ bool ParserSetQuery::parseNameValuePairWithDefault(SettingChange & change, Strin ParserCompoundIdentifier name_p; ParserLiteralOrMap value_p; ParserToken s_eq(TokenType::Equals); - ParserSetQuery set_p(true); - ParserToken l_br(TokenType::OpeningRoundBracket); - ParserToken r_br(TokenType::ClosingRoundBracket); + ParserFunction function_p; ASTPtr name; ASTPtr value; bool is_default = false; + ASTPtr function_ast; if (!name_p.parse(pos, name, expected)) return false; @@ -160,25 +155,22 @@ bool ParserSetQuery::parseNameValuePairWithDefault(SettingChange & change, Strin value = std::make_shared(Field(static_cast(0))); else if (ParserKeyword("DEFAULT").ignore(pos, expected)) is_default = true; - else if (ParserKeyword("SETTINGS").ignore(pos, expected) - && l_br.ignore(pos, expected) - && set_p.parse(pos, value, expected) - && r_br.ignore(pos, expected)) + /// for SETTINGS disk=disk(type='s3', path='', ...) + else if (function_p.parse(pos, function_ast, expected) && function_ast->as()->name == "disk") { - 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()); + tryGetIdentifierNameInto(name, change.getName()); + change.setASTValue(function_ast); + return true; } else if (!value_p.parse(pos, value, expected)) return false; - tryGetIdentifierNameInto(name, change.name); + tryGetIdentifierNameInto(name, change.getName()); if (is_default) - default_settings = change.name; + default_settings = change.getName(); else - change.value = value->as().value; + change.setFieldValue(value->as().value); return true; } diff --git a/src/Storages/MergeTree/registerStorageMergeTree.cpp b/src/Storages/MergeTree/registerStorageMergeTree.cpp index e8c9e283b9f..4fc47c1cc11 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,7 +24,6 @@ #include #include #include -#include namespace DB @@ -608,26 +607,21 @@ static StoragePtr create(const StorageFactory::Arguments & args) metadata.column_ttls_by_name[name] = new_ttl_entry; } - storage_settings->loadFromQuery(*args.storage_def); - - /** - * 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)) + if (args.storage_def->settings) { - auto disk_settings = disk_value.safeGet() | std::views::transform([](const Field & field) + for (auto & [_, value, value_ast] : args.storage_def->settings->changes) { - 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); + if (isDiskFunction(value_ast)) + { + value = createDiskFromDiskAST(*value_ast->as(), context); + break; + } + } } - /// Updates the default storage_settings with settings specified via SETTINGS arg in a query + storage_settings->loadFromQuery(*args.storage_def); + + // 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 new file mode 100644 index 00000000000..9abbb5c0a9b --- /dev/null +++ b/src/Storages/createDiskFromDiskAST.cpp @@ -0,0 +1,74 @@ +#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 new file mode 100644 index 00000000000..6047a494a6f --- /dev/null +++ b/src/Storages/createDiskFromDiskAST.h @@ -0,0 +1,23 @@ +#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 deleted file mode 100644 index 7385a2905c6..00000000000 --- a/src/Storages/getOrCreateDiskFromSettings.cpp +++ /dev/null @@ -1,112 +0,0 @@ -#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 deleted file mode 100644 index 6c800825ad7..00000000000 --- a/src/Storages/getOrCreateDiskFromSettings.h +++ /dev/null @@ -1,21 +0,0 @@ -#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); - -}