More tests

programs/server/Server.cpp

@@ -734,7 +734,6 @@ int Server::main(const std::vector<std::string> & /*args*/)
         config().getUInt("io_thread_pool_queue_size", 10000));
 
     NamedCollectionUtils::loadFromConfig(config());
-    NamedCollectionUtils::loadFromSQL(global_context);
 
     /// Initialize global local cache for remote filesystem.
     if (config().has("local_cache_for_remote_fs"))
@@ -1120,6 +1119,8 @@ int Server::main(const std::vector<std::string> & /*args*/)
         SensitiveDataMasker::setInstance(std::make_unique<SensitiveDataMasker>(config(), "query_masking_rules"));
     }
 
+    NamedCollectionUtils::loadFromSQL(global_context);
+
     auto main_config_reloader = std::make_unique<ConfigReloader>(
         config_path,
         include_from_path,

src/Parsers/ParserAlterNamedCollectionQuery.cpp

@@ -41,11 +41,20 @@ bool ParserAlterNamedCollectionQuery::parseImpl(IParser::Pos & pos, ASTPtr & nod
             return false;
     }
 
+    bool parsed_delete = false;
     if (!set_p.parse(pos, set, expected))
     {
         if (!s_delete.ignore(pos, expected))
             return false;
 
+        parsed_delete = true;
+    }
+    else if (s_delete.ignore(pos, expected))
+    {
+        parsed_delete = true;
+    }
+
+    if (parsed_delete)
         while (true)
         {
             if (!delete_keys.empty() && !s_comma.ignore(pos))
@@ -57,7 +66,6 @@ bool ParserAlterNamedCollectionQuery::parseImpl(IParser::Pos & pos, ASTPtr & nod
 
             delete_keys.push_back(getIdentifierName(key));
         }
-    }
 
     auto query = std::make_shared<ASTAlterNamedCollectionQuery>();
 

src/Storages/NamedCollectionUtils.cpp

@@ -150,7 +150,8 @@ private:
 public:
     explicit LoadFromSQL(ContextPtr context_)
         : WithContext(context_)
-        , metadata_path(fs::path(context_->getPath()) / NAMED_COLLECTIONS_METADATA_DIRECTORY)
+        , metadata_path(
+            fs::canonical(context_->getPath()) / NAMED_COLLECTIONS_METADATA_DIRECTORY)
     {
         if (!fs::exists(metadata_path))
             fs::create_directories(metadata_path);
@@ -168,6 +169,7 @@ public:
     NamedCollectionsMap getAll() const
     {
         NamedCollectionsMap result;
+
         for (const auto & collection_name : listCollections())
         {
             if (result.contains(collection_name))
@@ -205,10 +207,10 @@ public:
         const auto path = getMetadataPath(query.collection_name);
         auto create_query = readCreateQueryFromMetadata(path, getContext()->getSettings());
 
-        std::unordered_map<std::string, Field> alter_changes_map;
+        std::unordered_map<std::string, Field> result_changes_map;
         for (const auto & [name, value] : query.changes)
         {
-            auto [it, inserted] = alter_changes_map.emplace(name, value);
+            auto [it, inserted] = result_changes_map.emplace(name, value);
             if (!inserted)
             {
                 throw Exception(
@@ -219,15 +221,24 @@ public:
         }
 
         for (auto & [name, value] : create_query.changes)
+            result_changes_map.emplace(name, value);
+
+        for (const auto & delete_key : query.delete_keys)
         {
-            auto it = alter_changes_map.find(name);
+            auto it = result_changes_map.find(delete_key);
-            if (it == alter_changes_map.end())
+            if (it == result_changes_map.end())
-                continue;
+            {
-            value = it->second;
+                throw Exception(
-            alter_changes_map.erase(name);
+                    ErrorCodes::BAD_ARGUMENTS,
+                    "Cannot delete key `{}` because it does not exist in collection",
+                    delete_key);
+            }
+            else
+                result_changes_map.erase(it);
         }
 
-        for (const auto & [name, value] : alter_changes_map)
+        create_query.changes.clear();
+        for (const auto & [name, value] : result_changes_map)
             create_query.changes.emplace_back(name, value);
 
         writeCreateQueryToMetadata(

tests/integration/test_named_collections/configs/users.d/users.xml

@@ -0,0 +1,13 @@
+<clickhouse>
+    <users>
+        <default>
+            <password></password>
+            <networks>
+                <ip>::/0</ip>
+            </networks>
+            <profile>default</profile>
+            <quota>default</quota>
+            <show_named_collections>1</show_named_collections>
+        </default>
+    </users>
+</clickhouse>

tests/integration/test_named_collections/test.py

@@ -1,6 +1,7 @@
 import logging
 import pytest
 import os
+import time
 from helpers.cluster import ClickHouseCluster
 
 SCRIPT_DIR = os.path.dirname(os.path.realpath(__file__))
@@ -18,6 +19,9 @@ def cluster():
             main_configs=[
                 "configs/config.d/named_collections.xml",
             ],
+            user_configs=[
+                "configs/users.d/users.xml",
+            ],
             stay_alive=True,
         )
 
@@ -30,14 +34,12 @@ def cluster():
         cluster.shutdown()
 
 
-def replace_config(path, old, new):
+def replace_config(node, old, new):
-    config = open(path, "r")
+    node.replace_in_config(
-    config_lines = config.readlines()
+        "/etc/clickhouse-server/config.d/named_collections.xml",
-    config.close()
+        old,
-    config_lines = [line.replace(old, new) for line in config_lines]
+        new,
-    config = open(path, "w")
+    )
-    config.writelines(config_lines)
-    config.close()
 
 
 def test_config_reload(cluster):
@@ -58,12 +60,7 @@ def test_config_reload(cluster):
         ).strip()
     )
 
-    replace_config(
+    replace_config(node, "value1", "value2")
-        NAMED_COLLECTIONS_CONFIG,
-        "<key1>value1",
-        "<key1>value2",
-    )
-
     node.query("SYSTEM RELOAD CONFIG")
 
     assert (
@@ -80,14 +77,13 @@ def test_config_reload(cluster):
     )
 
 
-def test_create_and_drop_collection(cluster):
+def test_sql_commands(cluster):
     node = cluster.instances["node"]
     assert "1" == node.query("select count() from system.named_collections").strip()
 
     node.query("CREATE NAMED COLLECTION collection2 AS key1=1, key2='value2'")
 
     def check_created():
-        assert "2" == node.query("select count() from system.named_collections").strip()
         assert (
             "collection1\ncollection2"
             == node.query("select name from system.named_collections").strip()
@@ -118,6 +114,76 @@ def test_create_and_drop_collection(cluster):
     node.restart_clickhouse()
     check_created()
 
+    node.query("ALTER NAMED COLLECTION collection2 SET key1=4, key3='value3'")
+
+    def check_altered():
+        assert (
+            "['key1','key2','key3']"
+            == node.query(
+                "select mapKeys(collection) from system.named_collections where name = 'collection2'"
+            ).strip()
+        )
+
+        assert (
+            "4"
+            == node.query(
+                "select collection['key1'] from system.named_collections where name = 'collection2'"
+            ).strip()
+        )
+
+        assert (
+            "value3"
+            == node.query(
+                "select collection['key3'] from system.named_collections where name = 'collection2'"
+            ).strip()
+        )
+
+
+    check_altered();
+    node.restart_clickhouse()
+    check_altered();
+
+    node.query("ALTER NAMED COLLECTION collection2 DELETE key2")
+
+    def check_deleted():
+        assert (
+            "['key1','key3']"
+            == node.query(
+                "select mapKeys(collection) from system.named_collections where name = 'collection2'"
+            ).strip()
+        )
+
+    check_deleted();
+    node.restart_clickhouse()
+    check_deleted();
+
+    node.query("ALTER NAMED COLLECTION collection2 SET key3=3, key4='value4' DELETE key1")
+
+    def check_altered_and_deleted():
+        assert (
+            "['key3','key4']"
+            == node.query(
+                "select mapKeys(collection) from system.named_collections where name = 'collection2'"
+            ).strip()
+        )
+
+        assert (
+            "3"
+            == node.query(
+                "select collection['key3'] from system.named_collections where name = 'collection2'"
+            ).strip()
+        )
+        assert (
+            "value4"
+            == node.query(
+                "select collection['key4'] from system.named_collections where name = 'collection2'"
+            ).strip()
+        )
+
+    check_altered_and_deleted();
+    node.restart_clickhouse()
+    check_altered_and_deleted();
+
     node.query("DROP NAMED COLLECTION collection2")
 
     def check_dropped():