Review fixes

dbms/src/Common/ErrorCodes.cpp

@@ -155,7 +155,7 @@ namespace ErrorCodes
     extern const int NOT_FOUND_FUNCTION_ELEMENT_FOR_AGGREGATE = 147;
     extern const int NOT_FOUND_RELATION_ELEMENT_FOR_CONDITION = 148;
     extern const int NOT_FOUND_RHS_ELEMENT_FOR_CONDITION = 149;
-    extern const int NO_ATTRIBUTES_LISTED = 150;
+    extern const int EMPTY_LIST_OF_ATTRIBUTES_PASSED = 150;
     extern const int INDEX_OF_COLUMN_IN_SORT_CLAUSE_IS_OUT_OF_RANGE = 151;
     extern const int UNKNOWN_DIRECTION_OF_SORTING = 152;
     extern const int ILLEGAL_DIVISION = 153;
@@ -361,7 +361,7 @@ namespace ErrorCodes
     extern const int PART_IS_TEMPORARILY_LOCKED = 384;
     extern const int MULTIPLE_STREAMS_REQUIRED = 385;
     extern const int NO_COMMON_TYPE = 386;
-    extern const int EXTERNAL_LOADABLE_ALREADY_EXISTS = 387;
+    extern const int DICTIONARY_ALREADY_EXISTS = 387;
     extern const int CANNOT_ASSIGN_OPTIMIZE = 388;
     extern const int INSERT_WAS_DEDUPLICATED = 389;
     extern const int CANNOT_GET_CREATE_TABLE_QUERY = 390;
@@ -460,10 +460,8 @@ namespace ErrorCodes
     extern const int TOO_MANY_REDIRECTS = 483;
     extern const int INTERNAL_REDIS_ERROR = 484;
     extern const int CANNOT_GET_CREATE_DICTIONARY_QUERY = 485;
-    extern const int DICTIONARY_ALREADY_EXISTS = 486;
-    extern const int UNKNOWN_DICTIONARY = 487;
-    extern const int EMPTY_LIST_OF_ATTRIBUTES_PASSED = 488;
-    extern const int INCORRECT_DICTIONARY_DEFINITION = 489;
+    extern const int UNKNOWN_DICTIONARY = 486;
+    extern const int INCORRECT_DICTIONARY_DEFINITION = 487;
 
     extern const int KEEPER_EXCEPTION = 999;
     extern const int POCO_EXCEPTION = 1000;

dbms/src/Databases/DatabaseFactory.cpp

@@ -54,13 +54,12 @@ DatabasePtr DatabaseFactory::get(
     {
         const ASTFunction * engine = engine_define->engine;
 
-        std::vector<ASTPtr> arguments;
-        if (engine->arguments)
-            arguments = engine->arguments->children;
+        if (!engine->arguments || engine->arguments->children.size() != 4)
+            throw Exception(
+                "MySQL Database require mysql_hostname, mysql_database_name, mysql_username, mysql_password arguments.",
+                ErrorCodes::BAD_ARGUMENTS);
 
-        if (arguments.size() != 4)
-            throw Exception("MySQL Database require mysql_hostname, mysql_database_name, mysql_username, mysql_password arguments.",
-                            ErrorCodes::BAD_ARGUMENTS);
+        const auto & arguments = engine->arguments->children;
 
         const auto & mysql_host_name = arguments[0]->as<ASTLiteral>()->value.safeGet<String>();
         const auto & mysql_database_name = arguments[1]->as<ASTLiteral>()->value.safeGet<String>();
@@ -78,13 +77,10 @@ DatabasePtr DatabaseFactory::get(
     {
         const ASTFunction * engine = engine_define->engine;
 
-        std::vector<ASTPtr> arguments;
-        if (engine->arguments)
-            arguments = engine->arguments->children;
+        if (!engine->arguments || engine->arguments->children.size() != 1)
+            throw Exception("Lazy database require cache_expiration_time_seconds argument", ErrorCodes::BAD_ARGUMENTS);
 
-        if (arguments.size() != 1)
-            throw Exception("Lazy database require cache_expiration_time_seconds argument",
-                            ErrorCodes::BAD_ARGUMENTS);
+        const auto & arguments = engine->arguments->children;
 
         const auto cache_expiration_time_seconds = arguments[0]->as<ASTLiteral>()->value.safeGet<UInt64>();
         return std::make_shared<DatabaseLazy>(database_name, metadata_path, cache_expiration_time_seconds, context);

dbms/src/Databases/DatabaseOnDisk.cpp

@@ -165,28 +165,35 @@ std::pair<String, StoragePtr> createTableFromAST(
 String getObjectDefinitionFromCreateQuery(const ASTPtr & query)
 {
     ASTPtr query_clone = query->clone();
-    auto & create = query_clone->as<ASTCreateQuery &>();
+    auto * create = query_clone->as<ASTCreateQuery>();
 
-    if (!create.is_dictionary)
-        create.attach = true;
+    if (!create)
+    {
+        std::ostringstream query_stream;
+        formatAST(*create, query_stream, true);
+        throw Exception("Query '" + query_stream.str() + "' is not CREATE query", ErrorCodes::LOGICAL_ERROR);
+    }
+
+    if (!create->is_dictionary)
+        create->attach = true;
 
     /// We remove everything that is not needed for ATTACH from the query.
-    create.database.clear();
-    create.as_database.clear();
-    create.as_table.clear();
-    create.if_not_exists = false;
-    create.is_populate = false;
-    create.replace_view = false;
+    create->database.clear();
+    create->as_database.clear();
+    create->as_table.clear();
+    create->if_not_exists = false;
+    create->is_populate = false;
+    create->replace_view = false;
 
     /// For views it is necessary to save the SELECT query itself, for the rest - on the contrary
-    if (!create.is_view && !create.is_materialized_view && !create.is_live_view)
-        create.select = nullptr;
+    if (!create->is_view && !create->is_materialized_view && !create->is_live_view)
+        create->select = nullptr;
 
-    create.format = nullptr;
-    create.out_file = nullptr;
+    create->format = nullptr;
+    create->out_file = nullptr;
 
     std::ostringstream statement_stream;
-    formatAST(create, statement_stream, false);
+    formatAST(*create, statement_stream, false);
     statement_stream << '\n';
     return statement_stream.str();
 }
@@ -260,18 +267,14 @@ void DatabaseOnDisk::createDictionary(
 {
     const auto & settings = context.getSettingsRef();
 
-    /// Create a file with metadata if necessary - if the query is not ATTACH.
-    /// Write the query of `ATTACH table` to it.
-
     /** The code is based on the assumption that all threads share the same order of operations
       * - creating the .sql.tmp file;
-      * - adding a table to `tables`;
+      * - adding a dictionary to `dictionaries`;
       * - rename .sql.tmp to .sql.
       */
 
-    /// A race condition would be possible if a table with the same name is simultaneously created using CREATE and using ATTACH.
+    /// A race condition would be possible if a dictionary with the same name is simultaneously created using CREATE and using ATTACH.
     /// But there is protection from it - see using DDLGuard in InterpreterCreateQuery.
-
     if (database.isDictionaryExist(context, dictionary_name))
         throw Exception("Dictionary " + backQuote(database.getDatabaseName()) + "." + backQuote(dictionary_name) + " already exists.", ErrorCodes::DICTIONARY_ALREADY_EXISTS);
 
@@ -297,7 +300,7 @@ void DatabaseOnDisk::createDictionary(
 
     try
     {
-        /// Do not load it now
+        /// Do not load it now because we want more strict loading
         database.attachDictionary(dictionary_name, context, false);
         /// Load dictionary
         bool lazy_load = context.getConfigRef().getBool("dictionaries_lazy_load", true);
@@ -305,7 +308,7 @@ void DatabaseOnDisk::createDictionary(
         context.getExternalDictionariesLoader().addDictionaryWithConfig(
             dict_name, database.getDatabaseName(), query->as<const ASTCreateQuery &>(), !lazy_load);
 
-        /// If it was ATTACH query and file with table metadata already exist
+        /// If it was ATTACH query and file with dictionary metadata already exist
         /// (so, ATTACH is done after DETACH), then rename atomically replaces old file with new one.
         Poco::File(dictionary_metadata_tmp_path).renameTo(dictionary_metadata_path);
 
@@ -354,7 +357,7 @@ void DatabaseOnDisk::removeDictionary(
     IDatabase & database,
     const Context & context,
     const String & dictionary_name,
-    Poco::Logger * log)
+    Poco::Logger * /*log*/)
 {
     database.detachDictionary(dictionary_name, context);
 
@@ -366,15 +369,7 @@ void DatabaseOnDisk::removeDictionary(
     }
     catch (...)
     {
-        try
-        {
-            Poco::File(dictionary_metadata_path + ".tmp_drop").remove();
-            return;
-        }
-        catch (...)
-        {
-            LOG_WARNING(log, getCurrentExceptionMessage(__PRETTY_FUNCTION__));
-        }
+        /// If it's not possible for some reason
         database.attachDictionary(dictionary_name, context);
         throw;
     }

dbms/src/Databases/DatabaseOrdinary.cpp

@@ -55,18 +55,6 @@ namespace
 {
 
 
-String createDictionaryFromAST(
-    ASTCreateQuery ast_create_query,
-    const String & database_name)
-{
-    ast_create_query.database = database_name;
-
-    if (!ast_create_query.dictionary_attributes_list)
-        throw Exception("Missing definition of dictionary attributes.", ErrorCodes::EMPTY_LIST_OF_ATTRIBUTES_PASSED);
-
-    return ast_create_query.table;
-}
-
 void loadObject(
     Context & context,
     const ASTCreateQuery & query,
@@ -78,7 +66,7 @@ try
 {
     if (query.is_dictionary)
     {
-        String dictionary_name = createDictionaryFromAST(query, database_name);
+        String dictionary_name = query.table;
         database.attachDictionary(dictionary_name, context, false);
     }
     else
@@ -130,7 +118,7 @@ void DatabaseOrdinary::loadStoredObjects(
       * Otherwise (for the ext4 filesystem), `DirectoryIterator` iterates through them in some order,
       *  which does not correspond to order tables creation and does not correspond to order of their location on disk.
       */
-    using FileNames = std::map<std::string, ASTCreateQuery>;
+    using FileNames = std::map<std::string, ASTPtr>;
     FileNames file_names;
 
     size_t total_dictionaries = 0;
@@ -142,9 +130,9 @@ void DatabaseOrdinary::loadStoredObjects(
             auto ast = parseCreateQueryFromMetadataFile(full_path, log);
             if (ast)
             {
-                auto create_query = ast->as<const ASTCreateQuery &>();
-                file_names[file_name] = create_query;
-                total_dictionaries += create_query.is_dictionary;
+                auto * create_query = ast->as<ASTCreateQuery>();
+                file_names[file_name] = ast;
+                total_dictionaries += create_query->is_dictionary;
             }
         }
         catch (const Exception & e)
@@ -179,7 +167,7 @@ void DatabaseOrdinary::loadStoredObjects(
 
     for (const auto & name_with_query : file_names)
     {
-        pool.scheduleOrThrowOnError([&]() { loadOneObject(name_with_query.second); });
+        pool.scheduleOrThrowOnError([&]() { loadOneObject(name_with_query.second->as<const ASTCreateQuery &>()); });
     }
 
     pool.wait();

dbms/src/Dictionaries/getDictionaryConfigurationFromAST.cpp

@@ -28,8 +28,11 @@ namespace ErrorCodes
 namespace
 {
 
-String unescapeString(const String & string)
+/// Get value from field and convert it to string.
+/// Also remove quotes from strings.
+String getUnescapedFieldString(const Field & field)
 {
+    String string = applyVisitor(FieldVisitorToString(), field);
     if (!string.empty() && string.front() == '\'' && string.back() == '\'')
         return string.substr(1, string.size() - 2);
     return string;
@@ -324,8 +327,7 @@ void buildConfigurationFromFunctionWithKeyValueArguments(
         }
         else if (auto literal = pair->second->as<const ASTLiteral>(); literal)
         {
-            String str_literal = applyVisitor(FieldVisitorToString(), literal->value);
-            AutoPtr<Text> value(doc->createTextNode(unescapeString(str_literal)));
+            AutoPtr<Text> value(doc->createTextNode(getUnescapedFieldString(literal->value)));
             current_xml_element->appendChild(value);
         }
         else if (auto list = pair->second->as<const ASTExpressionList>(); list)

dbms/src/Interpreters/Context.cpp

@@ -192,7 +192,7 @@ struct ContextShared
     bool shutdown_called = false;
 
     /// Do not allow simultaneous execution of DDL requests on the same table.
-    /// database -> table -> (mutex, counter), counter: how many threads are running a query on the table at the same time
+    /// database -> object -> (mutex, counter), counter: how many threads are running a query on the table at the same time
     /// For the duration of the operation, an element is placed here, and an object is returned,
     /// which deletes the element in the destructor when counter becomes zero.
     /// In case the element already exists, waits, when query will be executed in other thread. See class DDLGuard below.