Review

Author: MikhailBurdukov

src/Common/NamedCollections/NamedCollections.cpp

@@ -175,16 +175,6 @@ class NamedCollection::Impl
     }
 };
 
-String NamedCollection::sourceIdToString(NamedCollection::SourceId id)
-{
-    switch (id)
-    {
-        case NamedCollection::SourceId::NONE: return "NONE";
-        case NamedCollection::SourceId::CONFIG: return "CONFIG";
-        case NamedCollection::SourceId::SQL: return "SQL";
-    }
-}
-
 NamedCollection::NamedCollection(
     ImplPtr pimpl_,
     const std::string & collection_name_,
@@ -345,7 +335,7 @@ NamedCollectionFromConfig::NamedCollectionFromConfig(
     const std::string & collection_name_,
     const std::string & collection_path_,
     const Keys & keys_)
-    : NamedCollection(Impl::create(config_, collection_name_, collection_path_, keys_), collection_name_, false)
+    : NamedCollection(Impl::create(config_, collection_name_, collection_path_, keys_), collection_name_, /* is_mutable */ false)
 {
 }
 

src/Common/NamedCollections/NamedCollections.h

@@ -27,13 +27,13 @@ class NamedCollection
     using Keys = std::set<Key, std::less<>>;
     enum class SourceId : uint8_t
     {
+        /// None source_id is possible only if the object is a
+        /// duplicate of some named collection. See `duplicate` method.
         NONE = 0,
         CONFIG = 1,
         SQL = 2,
     };
 
-    static String sourceIdToString(SourceId id);
-
     bool has(const Key & key) const;
 
     bool hasAny(const std::initializer_list<Key> & keys) const;
@@ -76,7 +76,7 @@ class NamedCollection
 
     virtual SourceId getSourceId() const { return SourceId::NONE; }
 
-    virtual String getCreateStatement(bool /*show_secrects*/) { return  " "; }
+    virtual String getCreateStatement(bool /*show_secrects*/) { return  {}; }
 
     virtual void update(const ASTAlterNamedCollectionQuery & query);
 
@@ -127,7 +127,7 @@ class NamedCollectionFromConfig final : public NamedCollection
         const std::string & collection_path,
         const Keys & keys);
 
-    String getCreateStatement(bool /*show_secrects*/) override { return " "; }
+    String getCreateStatement(bool /*show_secrects*/) override { return {}; }
 
     void update(const ASTAlterNamedCollectionQuery & /*query*/) override { NamedCollection::assertMutable(); }
 

src/Common/NamedCollections/NamedCollectionsFactory.cpp

@@ -15,6 +15,7 @@ namespace ErrorCodes
     extern const int NAMED_COLLECTION_DOESNT_EXIST;
     extern const int NAMED_COLLECTION_ALREADY_EXISTS;
     extern const int NAMED_COLLECTION_IS_IMMUTABLE;
+    extern const int LOGICAL_ERROR;
 }
 
 NamedCollectionFactory & NamedCollectionFactory::instance()
@@ -335,7 +336,13 @@ void NamedCollectionFactory::updateFromSQL(const ASTAlterNamedCollectionQuery &
     auto updated_collection_ptr = metadata_storage->update(query);
 
     auto it = loaded_named_collections.find(collection_name);
-    chassert(it != loaded_named_collections.end());
+    if (it == loaded_named_collections.end())
+    {
+        throw Exception(
+            ErrorCodes::LOGICAL_ERROR,
+            "The named collection {} unexpectedly does not exist.",
+            collection_name);
+    }
 
     if (!it->second->isMutable())
     {

src/Common/NamedCollections/NamedCollectionsMetadataStorage.cpp

@@ -524,7 +524,7 @@ ASTCreateNamedCollectionQuery NamedCollectionsMetadataStorage::readCreateQuery(c
     return create_query;
 }
 
-void NamedCollectionsMetadataStorage::writeCreateQuery(const String& collection_name, const String& create_statement, bool replace)
+void NamedCollectionsMetadataStorage::writeCreateQuery(const String & collection_name, const String & create_statement, bool replace)
 {
     storage->write(getFileName(collection_name), create_statement, replace);
 }

src/Common/NamedCollections/NamedCollectionsMetadataStorage.h

@@ -46,7 +46,7 @@ class NamedCollectionsMetadataStorage : private WithContext
 
     ASTCreateNamedCollectionQuery readCreateQuery(const std::string & collection_name) const;
 
-    void writeCreateQuery(const String& collection_name, const String& create_statement, bool replace = false);
+    void writeCreateQuery(const String & collection_name, const String & create_statement, bool replace = false);
 };
 
 

src/Storages/System/StorageSystemNamedCollections.cpp

@@ -1,5 +1,6 @@
 #include "StorageSystemNamedCollections.h"
 
+#include <base/EnumReflection.h>
 #include <Columns/ColumnArray.h>
 #include <Columns/ColumnTuple.h>
 #include <DataTypes/DataTypeString.h>
@@ -67,7 +68,7 @@ void StorageSystemNamedCollections::fillData(MutableColumns & res_columns, Conte
 
         offsets.push_back(offsets.back() + size);
 
-        res_columns[2]->insert(NamedCollection::sourceIdToString(collection->getSourceId()));
+        res_columns[2]->insert(magic_enum::enum_name(collection->getSourceId()));
         res_columns[3]->insert(collection->getCreateStatement(access_secrets));
     }
 }

tests/integration/test_named_collections/test.py

@@ -842,7 +842,7 @@ def validate_named_collection(collection_name, source, key_value):
                 node.query(
                     f"SELECT create_query FROM system.named_collections WHERE name='{collection_name}'"
                 )
-                == " \n"
+                == "\n"
             )
         else:
             hidden_str = "\\'[HIDDEN]\\'"