Merge pull request #52820 from kssenii/fix-dynamic-disk-conf

Better dynamic disk configuration

src/Disks/getDiskConfigurationFromAST.cpp

@@ -31,7 +31,7 @@ namespace ErrorCodes
         message.empty() ? "" : ": " + message);
 }
 
-Poco::AutoPtr<Poco::XML::Document> getDiskConfigurationFromASTImpl(const std::string & root_name, const ASTs & disk_args, ContextPtr context)
+Poco::AutoPtr<Poco::XML::Document> getDiskConfigurationFromASTImpl(const ASTs & disk_args, ContextPtr context)
 {
     if (disk_args.empty())
         throwBadConfiguration("expected non-empty list of arguments");
@@ -39,8 +39,6 @@ Poco::AutoPtr<Poco::XML::Document> getDiskConfigurationFromASTImpl(const std::st
     Poco::AutoPtr<Poco::XML::Document> xml_document(new Poco::XML::Document());
     Poco::AutoPtr<Poco::XML::Element> root(xml_document->createElement("disk"));
     xml_document->appendChild(root);
-    Poco::AutoPtr<Poco::XML::Element> disk_configuration(xml_document->createElement(root_name));
-    root->appendChild(disk_configuration);
 
     for (const auto & arg : disk_args)
     {
@@ -62,7 +60,7 @@ Poco::AutoPtr<Poco::XML::Document> getDiskConfigurationFromASTImpl(const std::st
 
         const std::string & key = key_identifier->name();
         Poco::AutoPtr<Poco::XML::Element> key_element(xml_document->createElement(key));
-        disk_configuration->appendChild(key_element);
+        root->appendChild(key_element);
 
         if (!function_args[1]->as<ASTLiteral>() && !function_args[1]->as<ASTIdentifier>())
             throwBadConfiguration("expected values to be literals or identifiers");
@@ -75,9 +73,9 @@ Poco::AutoPtr<Poco::XML::Document> getDiskConfigurationFromASTImpl(const std::st
     return xml_document;
 }
 
-DiskConfigurationPtr getDiskConfigurationFromAST(const std::string & root_name, const ASTs & disk_args, ContextPtr context)
+DiskConfigurationPtr getDiskConfigurationFromAST(const ASTs & disk_args, ContextPtr context)
 {
-    auto xml_document = getDiskConfigurationFromASTImpl(root_name, disk_args, context);
+    auto xml_document = getDiskConfigurationFromASTImpl(disk_args, context);
     Poco::AutoPtr<Poco::Util::XMLConfiguration> conf(new Poco::Util::XMLConfiguration());
     conf->load(xml_document);
     return conf;

src/Disks/getDiskConfigurationFromAST.h

@@ -14,19 +14,19 @@ using DiskConfigurationPtr = Poco::AutoPtr<Poco::Util::AbstractConfiguration>;
 /**
  * Transform a list of pairs ( key1=value1, key2=value2, ... ), where keys and values are ASTLiteral or ASTIdentifier
  * into
- * <root_name>
+ * <disk>
  *     <key1>value1</key1>
  *     <key2>value2</key2>
  *     ...
- * </root_name>
+ * </disk>
  *
  * 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);
+DiskConfigurationPtr getDiskConfigurationFromAST(const ASTs & disk_args, ContextPtr context);
 
 /// The same as above function, but return XML::Document for easier modification of result configuration.
-[[ maybe_unused ]] Poco::AutoPtr<Poco::XML::Document> getDiskConfigurationFromASTImpl(const std::string & root_name, const ASTs & disk_args, ContextPtr context);
+[[ maybe_unused ]] Poco::AutoPtr<Poco::XML::Document> getDiskConfigurationFromASTImpl(const ASTs & disk_args, ContextPtr context);
 
 /*
  * A reverse function.

src/Disks/getOrCreateDiskFromAST.cpp

@@ -26,8 +26,16 @@ namespace
 {
     std::string getOrCreateDiskFromDiskAST(const ASTFunction & function, ContextPtr context)
     {
+        const auto * function_args_expr = assert_cast<const ASTExpressionList *>(function.arguments.get());
+        const auto & function_args = function_args_expr->children;
+        auto config = getDiskConfigurationFromAST(function_args, context);
+
         std::string disk_name;
-        if (function.name == "disk")
+        if (config->has("name"))
+        {
+            disk_name = config->getString("name");
+        }
+        else
         {
             /// 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
@@ -36,21 +44,9 @@ namespace
             disk_name = DiskSelector::TMP_INTERNAL_DISK_PREFIX
                 + toString(sipHash128(disk_setting_string.data(), disk_setting_string.size()));
         }
-        else
-        {
-            static constexpr std::string_view custom_disk_prefix = "disk_";
-
-            if (function.name.size() <= custom_disk_prefix.size() || !function.name.starts_with(custom_disk_prefix))
-                throw Exception(ErrorCodes::BAD_ARGUMENTS, "Invalid disk name: {}", function.name);
-
-            disk_name = function.name.substr(custom_disk_prefix.size());
-        }
 
         auto result_disk = context->getOrCreateDisk(disk_name, [&](const DisksMap & disks_map) -> DiskPtr {
-            const auto * function_args_expr = assert_cast<const ASTExpressionList *>(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);
+            auto disk = DiskFactory::instance().create(disk_name, *config, "", context, disks_map);
             /// Mark that disk can be used without storage policy.
             disk->markDiskAsCustom();
             return disk;

src/Parsers/ParserSetQuery.cpp

@@ -215,7 +215,7 @@ bool ParserSetQuery::parseNameValuePair(SettingChange & change, IParser::Pos & p
     else if (ParserKeyword("FALSE").ignore(pos, expected))
         value = std::make_shared<ASTLiteral>(Field(static_cast<UInt64>(0)));
     /// for SETTINGS disk=disk(type='s3', path='', ...)
-    else if (function_p.parse(pos, function_ast, expected) && function_ast->as<ASTFunction>()->name.starts_with("disk"))
+    else if (function_p.parse(pos, function_ast, expected) && function_ast->as<ASTFunction>()->name == "disk")
     {
         tryGetIdentifierNameInto(name, change.name);
         change.value = createFieldFromAST(function_ast);
@@ -280,7 +280,7 @@ bool ParserSetQuery::parseNameValuePairWithParameterOrDefault(
         node = std::make_shared<ASTLiteral>(Field(static_cast<UInt64>(1)));
     else if (ParserKeyword("FALSE").ignore(pos, expected))
         node = std::make_shared<ASTLiteral>(Field(static_cast<UInt64>(0)));
-    else if (function_p.parse(pos, function_ast, expected) && function_ast->as<ASTFunction>()->name.starts_with("disk"))
+    else if (function_p.parse(pos, function_ast, expected) && function_ast->as<ASTFunction>()->name == "disk")
     {
         change.name = name;
         change.value = createFieldFromAST(function_ast);

tests/queries/0_stateless/02808_custom_disk_with_user_defined_name.sh

@@ -12,7 +12,7 @@ $CLICKHOUSE_CLIENT -nm --query """
 DROP TABLE IF EXISTS test;
 CREATE TABLE test (a Int32, b String)
 ENGINE = MergeTree() ORDER BY tuple()
-SETTINGS disk = disk_s3_disk(type = cache, max_size = '100Ki', path = ${CLICKHOUSE_TEST_UNIQUE_NAME}, disk = s3_disk);
+SETTINGS disk = disk(name = 's3_disk', type = cache, max_size = '100Ki', path = ${CLICKHOUSE_TEST_UNIQUE_NAME}, disk = s3_disk);
 """ 2>&1 | grep -q "Disk with name \`s3_disk\` already exist" && echo 'OK' || echo 'FAIL'
 
 disk_name="${CLICKHOUSE_TEST_UNIQUE_NAME}"
@@ -25,7 +25,7 @@ $CLICKHOUSE_CLIENT -nm --query """
 DROP TABLE IF EXISTS test;
 CREATE TABLE test (a Int32, b String)
 ENGINE = MergeTree() ORDER BY tuple()
-SETTINGS disk = disk_$disk_name(type = cache, max_size = '100Ki', path = ${CLICKHOUSE_TEST_UNIQUE_NAME}, disk = s3_disk);
+SETTINGS disk = disk(name = '$disk_name', type = cache, max_size = '100Ki', path = ${CLICKHOUSE_TEST_UNIQUE_NAME}, disk = s3_disk);
 """
 
 $CLICKHOUSE_CLIENT -nm --query """