From d84d282cd58b34972336e7ed7f361d6ed1a70b0f Mon Sep 17 00:00:00 2001 From: JaySon Date: Tue, 19 Nov 2024 12:07:50 +0800 Subject: [PATCH] *: Simplify the code of executing "CREATE TABLE" (#9629) ref pingcap/tiflash#6233 *: Simplify the code of executing "CREATE TABLE" Remove the clickhouse code for "CREATE TEMPORARY TABLE"/"CREATE TABLE ... AS ..."/"CREATE Materialized View" Co-authored-by: Lloyd-Pottiger <60744015+Lloyd-Pottiger@users.noreply.github.com> --- dbms/src/Databases/DatabasesCommon.cpp | 12 -- dbms/src/Debug/dbgFuncSchema.cpp | 1 - .../Interpreters/InterpreterCreateQuery.cpp | 158 +++--------------- .../src/Interpreters/InterpreterCreateQuery.h | 6 +- .../tests/gtest_interpreter_create_query.cpp | 12 +- dbms/src/Parsers/ASTCreateQuery.h | 48 +----- dbms/src/Parsers/ASTQueryWithOutput.cpp | 2 +- dbms/src/Parsers/ParserCreateQuery.cpp | 129 +------------- dbms/src/Parsers/ParserCreateQuery.h | 46 +++-- .../KVStore/tests/gtest_sync_schema.cpp | 21 +-- dbms/src/Storages/StorageFactory.cpp | 95 ++++------- 11 files changed, 90 insertions(+), 440 deletions(-) diff --git a/dbms/src/Databases/DatabasesCommon.cpp b/dbms/src/Databases/DatabasesCommon.cpp index ef66f3673df..4f2ab332a58 100644 --- a/dbms/src/Databases/DatabasesCommon.cpp +++ b/dbms/src/Databases/DatabasesCommon.cpp @@ -51,14 +51,7 @@ String getTableDefinitionFromCreateQuery(const ASTPtr & query) /// We remove everything that is not needed for ATTACH from the query. create.attach = true; create.database.clear(); - create.as_database.clear(); - create.as_table.clear(); create.if_not_exists = false; - create.is_populate = 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.select = nullptr; create.format = nullptr; create.out_file = nullptr; @@ -77,12 +70,7 @@ String getDatabaseDefinitionFromCreateQuery(const ASTPtr & query) /// We remove everything that is not needed for ATTACH from the query create.attach = true; create.table.clear(); - create.as_database.clear(); - create.as_table.clear(); create.if_not_exists = false; - create.is_populate = false; - - create.select = nullptr; create.format = nullptr; create.out_file = nullptr; diff --git a/dbms/src/Debug/dbgFuncSchema.cpp b/dbms/src/Debug/dbgFuncSchema.cpp index c310b09ead8..36bfdc6d635 100644 --- a/dbms/src/Debug/dbgFuncSchema.cpp +++ b/dbms/src/Debug/dbgFuncSchema.cpp @@ -18,7 +18,6 @@ #include #include #include -#include #include #include #include diff --git a/dbms/src/Interpreters/InterpreterCreateQuery.cpp b/dbms/src/Interpreters/InterpreterCreateQuery.cpp index ec79af18604..6649ba2bbd0 100644 --- a/dbms/src/Interpreters/InterpreterCreateQuery.cpp +++ b/dbms/src/Interpreters/InterpreterCreateQuery.cpp @@ -400,33 +400,12 @@ ColumnsDescription InterpreterCreateQuery::getColumnsDescription( } -ColumnsDescription InterpreterCreateQuery::setColumns( - ASTCreateQuery & create, - const Block & as_select_sample, - const StoragePtr & as_storage) const +ColumnsDescription InterpreterCreateQuery::setColumns(ASTCreateQuery & create) const { - ColumnsDescription res; - - if (create.columns) - { - res = getColumnsDescription(*create.columns, context); - } - else if (!create.as_table.empty()) - { - res = as_storage->getColumns(); - } - else if (create.select) - { - for (size_t i = 0; i < as_select_sample.columns(); ++i) - res.ordinary.emplace_back( - as_select_sample.safeGetByPosition(i).name, - as_select_sample.safeGetByPosition(i).type); - } - else - throw Exception( - "Incorrect CREATE query: required list of column descriptions or AS section or SELECT.", - ErrorCodes::INCORRECT_QUERY); + if (!create.columns) + throw Exception("Incorrect CREATE query: required list of column descriptions", ErrorCodes::INCORRECT_QUERY); + ColumnsDescription res = getColumnsDescription(*create.columns, context); /// Even if query has list of columns, canonicalize it (unfold Nested columns). ASTPtr new_columns = formatColumns(res); if (create.columns) @@ -453,45 +432,6 @@ ColumnsDescription InterpreterCreateQuery::setColumns( return res; } -void InterpreterCreateQuery::setEngine(ASTCreateQuery & create) const -{ - if (create.storage) - { - if (create.is_temporary && create.storage->engine->name != "Memory") - throw Exception( - "Temporary tables can only be created with ENGINE = Memory, not " + create.storage->engine->name, - ErrorCodes::INCORRECT_QUERY); - - return; - } - - if (create.is_temporary) - { - auto engine_ast = std::make_shared(); - engine_ast->name = "Memory"; - auto storage_ast = std::make_shared(); - storage_ast->set(storage_ast->engine, engine_ast); - create.set(create.storage, storage_ast); - } - else if (!create.as_table.empty()) - { - /// NOTE Getting the structure from the table specified in the AS is done not atomically with the creation of the table. - - String as_database_name = create.as_database.empty() ? context.getCurrentDatabase() : create.as_database; - String as_table_name = create.as_table; - - ASTPtr as_create_ptr = context.getCreateTableQuery(as_database_name, as_table_name); - const auto & as_create = typeid_cast(*as_create_ptr); - - if (as_create.is_view) - throw Exception( - "Cannot CREATE a table AS " + as_database_name + "." + as_table_name + ", it is a View", - ErrorCodes::INCORRECT_QUERY); - - create.set(create.storage, as_create.storage->ptr()); - } -} - /** * Try to acquire a DDLGuard to execute the "CREATE TABLE" actions. @@ -595,60 +535,26 @@ BlockIO InterpreterCreateQuery::createTable(ASTCreateQuery & create) create.attach = true; } - if (create.to_database.empty()) - create.to_database = current_database; - - if (create.select && (create.is_view || create.is_materialized_view)) - create.select->setDatabaseIfNeeded(current_database); - - Block as_select_sample; - if (create.select && (!create.attach || !create.columns)) - as_select_sample = InterpreterSelectWithUnionQuery::getSampleBlock(create.select->clone(), context); - - String as_database_name = create.as_database.empty() ? current_database : create.as_database; - String as_table_name = create.as_table; - - StoragePtr as_storage; - TableStructureLockHolder as_storage_lock; - if (!as_table_name.empty()) - { - as_storage = context.getTable(as_database_name, as_table_name); - as_storage_lock = as_storage->lockStructureForShare(context.getCurrentQueryId()); - } - /// Set and retrieve list of columns. - ColumnsDescription columns = setColumns(create, as_select_sample, as_storage); - - /// Set the table engine if it was not specified explicitly. - setEngine(create); + ColumnsDescription columns = setColumns(create); { - std::unique_ptr guard; - - String data_path; - DatabasePtr database; + DatabasePtr database = context.getDatabase(database_name); + String data_path = database->getDataPath(); - if (!create.is_temporary) + const auto guard = tryGetDDLGuard( + context, + database_name, + table_name, + create.if_not_exists, + /*timeout_seconds=*/5, + log_suffix); + if (!guard) { - database = context.getDatabase(database_name); - data_path = database->getDataPath(); - - guard = tryGetDDLGuard( - context, - database_name, - table_name, - create.if_not_exists, - /*timeout_seconds=*/5, - log_suffix); - if (!guard) - { - // Not the owner to create IStorage instance, and the table is created - // completely, let's return - return {}; - } - } - else if (context.tryGetExternalTable(table_name) && create.if_not_exists) + // Not the owner to create IStorage instance, and the table is created + // completely, let's return return {}; + } // Guard is acquired, let's create the IStorage instance StoragePtr res = StorageFactory::instance().get( @@ -671,35 +577,16 @@ BlockIO InterpreterCreateQuery::createTable(ASTCreateQuery & create) // If we do step 2 before step 1, we may run into "can't find .sql file" error when applying DDL jobs. // Besides, we make step 3 the final one, to ensure once we pass the check of context.isTableExist(database_name, table_name)`, the table must be created completely. - if (create.is_temporary) - context.getSessionContext().addExternalTable(table_name, res, query_ptr); - else - database->createTable(context, table_name, query_ptr); + database->createTable(context, table_name, query_ptr); // register the storage instance into `ManagedStorages` res->startup(); - if (!create.is_temporary) - database->attachTable(table_name, res); + database->attachTable(table_name, res); // the table has been created completely } - /// If the query is a CREATE SELECT, insert the data into the table. - if (create.select && !create.attach && !create.is_view && (!create.is_materialized_view || create.is_populate)) - { - auto insert = std::make_shared(); - - if (!create.is_temporary) - insert->database = database_name; - - insert->table = table_name; - insert->select = create.select->clone(); - - return InterpreterInsertQuery(insert, create.is_temporary ? context.getSessionContext() : context, false) - .execute(); - } - return {}; } @@ -740,11 +627,6 @@ void InterpreterCreateQuery::checkAccess(const ASTCreateQuery & create) throw Exception("Cannot create database in readonly mode", ErrorCodes::READONLY); } - if (create.is_temporary && readonly >= 2) - { - return; - } - throw Exception("Cannot create table in readonly mode", ErrorCodes::READONLY); } } // namespace DB diff --git a/dbms/src/Interpreters/InterpreterCreateQuery.h b/dbms/src/Interpreters/InterpreterCreateQuery.h index d15e5887519..838e5d9304b 100644 --- a/dbms/src/Interpreters/InterpreterCreateQuery.h +++ b/dbms/src/Interpreters/InterpreterCreateQuery.h @@ -59,11 +59,7 @@ class InterpreterCreateQuery : public IInterpreter BlockIO createTable(ASTCreateQuery & create); /// Calculate list of columns of table and return it. - ColumnsDescription setColumns( - ASTCreateQuery & create, - const Block & as_select_sample, - const StoragePtr & as_storage) const; - void setEngine(ASTCreateQuery & create) const; + ColumnsDescription setColumns(ASTCreateQuery & create) const; void checkAccess(const ASTCreateQuery & create); ASTPtr query_ptr; diff --git a/dbms/src/Interpreters/tests/gtest_interpreter_create_query.cpp b/dbms/src/Interpreters/tests/gtest_interpreter_create_query.cpp index 60b7bb376d5..132893422ef 100644 --- a/dbms/src/Interpreters/tests/gtest_interpreter_create_query.cpp +++ b/dbms/src/Interpreters/tests/gtest_interpreter_create_query.cpp @@ -22,9 +22,7 @@ #include #include -namespace DB -{ -namespace tests +namespace DB::tests { class InterperCreateQueryTiFlashTest : public ::testing::Test { @@ -34,7 +32,7 @@ class InterperCreateQueryTiFlashTest : public ::testing::Test static void TearDownTestCase() {} InterperCreateQueryTiFlashTest() - : log(&Poco::Logger::get("InterperCreateQuery")) + : log(Logger::get("InterperCreateQuery")) , context(TiFlashTestEnv::getGlobalContext()) {} @@ -136,7 +134,7 @@ class InterperCreateQueryTiFlashTest : public ::testing::Test protected: - Poco::Logger * log; + LoggerPtr log; Context & context; }; @@ -165,5 +163,5 @@ try } } CATCH -} // namespace tests -} // namespace DB + +} // namespace DB::tests diff --git a/dbms/src/Parsers/ASTCreateQuery.h b/dbms/src/Parsers/ASTCreateQuery.h index 458a1c652da..ef4c0a8c7ff 100644 --- a/dbms/src/Parsers/ASTCreateQuery.h +++ b/dbms/src/Parsers/ASTCreateQuery.h @@ -92,22 +92,13 @@ class ASTCreateQuery : public ASTQueryWithOutput public: bool attach{false}; /// Query ATTACH TABLE, not CREATE TABLE. bool if_not_exists{false}; - bool is_view{false}; - bool is_materialized_view{false}; - bool is_populate{false}; - bool is_temporary{false}; String database; String table; ASTExpressionList * columns = nullptr; - String to_database; /// For CREATE MATERIALIZED VIEW mv TO table. - String to_table; ASTStorage * storage = nullptr; - String as_database; - String as_table; - ASTSelectWithUnionQuery * select = nullptr; /** Get the text that identifies this element. */ - String getID() const override { return (attach ? "AttachQuery_" : "CreateQuery_") + database + "_" + table; }; + String getID() const override { return (attach ? "AttachQuery_" : "CreateQuery_") + database + "_" + table; } ASTPtr clone() const override { @@ -118,8 +109,6 @@ class ASTCreateQuery : public ASTQueryWithOutput res->set(res->columns, columns->clone()); if (storage) res->set(res->storage, storage->clone()); - if (select) - res->set(res->select, select->clone()); cloneOutputOptions(*res); @@ -146,31 +135,11 @@ class ASTCreateQuery : public ASTQueryWithOutput { std::string what = "TABLE"; - if (is_view) - what = "VIEW"; - if (is_materialized_view) - what = "MATERIALIZED VIEW"; - - settings.ostr << (settings.hilite ? hilite_keyword : "") << (attach ? "ATTACH " : "CREATE ") - << (is_temporary ? "TEMPORARY " : "") << what << " " + settings.ostr << (settings.hilite ? hilite_keyword : "") << (attach ? "ATTACH " : "CREATE ") << what << " " << (if_not_exists ? "IF NOT EXISTS " : "") << (settings.hilite ? hilite_none : "") << (!database.empty() ? backQuoteIfNeed(database) + "." : "") << backQuoteIfNeed(table); } - if (!to_table.empty()) - { - settings.ostr << (settings.hilite ? hilite_keyword : "") << " TO " << (settings.hilite ? hilite_none : "") - << (!to_database.empty() ? backQuoteIfNeed(to_database) + "." : "") - << backQuoteIfNeed(to_table); - } - - if (!as_table.empty()) - { - settings.ostr << (settings.hilite ? hilite_keyword : "") << " AS " << (settings.hilite ? hilite_none : "") - << (!as_database.empty() ? backQuoteIfNeed(as_database) + "." : "") - << backQuoteIfNeed(as_table); - } - if (columns) { settings.ostr << (settings.one_line ? " (" : "\n("); @@ -182,19 +151,6 @@ class ASTCreateQuery : public ASTQueryWithOutput if (storage) storage->formatImpl(settings, state, frame); - - if (is_populate) - { - settings.ostr << (settings.hilite ? hilite_keyword : "") << " POPULATE" - << (settings.hilite ? hilite_none : ""); - } - - if (select) - { - settings.ostr << (settings.hilite ? hilite_keyword : "") << " AS" << settings.nl_or_ws - << (settings.hilite ? hilite_none : ""); - select->formatImpl(settings, state, frame); - } } }; diff --git a/dbms/src/Parsers/ASTQueryWithOutput.cpp b/dbms/src/Parsers/ASTQueryWithOutput.cpp index 39c4e00bc9f..6f82fb32799 100644 --- a/dbms/src/Parsers/ASTQueryWithOutput.cpp +++ b/dbms/src/Parsers/ASTQueryWithOutput.cpp @@ -54,7 +54,7 @@ void ASTQueryWithOutput::formatImpl(const FormatSettings & s, FormatState & stat bool ASTQueryWithOutput::resetOutputASTIfExist(IAST & ast) { - if (auto ast_with_output = dynamic_cast(&ast)) + if (auto * ast_with_output = dynamic_cast(&ast)) { ast_with_output->format.reset(); ast_with_output->out_file.reset(); diff --git a/dbms/src/Parsers/ParserCreateQuery.cpp b/dbms/src/Parsers/ParserCreateQuery.cpp index 6c6d5a0f53d..afb809ed3ea 100644 --- a/dbms/src/Parsers/ParserCreateQuery.cpp +++ b/dbms/src/Parsers/ParserCreateQuery.cpp @@ -64,10 +64,7 @@ bool ParserIdentifierWithParameters::parseImpl(Pos & pos, ASTPtr & node, Expecte return true; ParserNestedTable nested; - if (nested.parse(pos, node, expected)) - return true; - - return false; + return nested.parse(pos, node, expected); } @@ -196,15 +193,10 @@ bool ParserStorage::parseImpl(Pos & pos, ASTPtr & node, Expected & expected) bool ParserCreateQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected) { ParserKeyword s_create("CREATE"); - ParserKeyword s_temporary("TEMPORARY"); ParserKeyword s_attach("ATTACH"); ParserKeyword s_table("TABLE"); ParserKeyword s_database("DATABASE"); ParserKeyword s_if_not_exists("IF NOT EXISTS"); - ParserKeyword s_as("AS"); - ParserKeyword s_view("VIEW"); - ParserKeyword s_materialized("MATERIALIZED"); - ParserKeyword s_populate("POPULATE"); ParserToken s_dot(TokenType::Dot); ParserToken s_lparen(TokenType::OpeningRoundBracket); ParserToken s_rparen(TokenType::ClosingRoundBracket); @@ -216,18 +208,9 @@ bool ParserCreateQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected) ASTPtr database; ASTPtr table; ASTPtr columns; - ASTPtr to_database; - ASTPtr to_table; ASTPtr storage; - ASTPtr as_database; - ASTPtr as_table; - ASTPtr select; bool attach = false; bool if_not_exists = false; - bool is_view = false; - bool is_materialized_view = false; - bool is_populate = false; - bool is_temporary = false; if (!s_create.ignore(pos, expected)) { @@ -237,11 +220,6 @@ bool ParserCreateQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected) return false; } - if (s_temporary.ignore(pos, expected)) - { - is_temporary = true; - } - if (s_table.ignore(pos, expected)) { if (s_if_not_exists.ignore(pos, expected)) @@ -283,36 +261,14 @@ bool ParserCreateQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected) if (!s_rparen.ignore(pos, expected)) return false; - if (!storage_p.parse(pos, storage, expected) && !is_temporary) + if (!storage_p.parse(pos, storage, expected)) return false; } else { - storage_p.parse(pos, storage, expected); - - if (!s_as.ignore(pos, expected)) - return false; - - if (!select_p.parse(pos, select, expected)) /// AS SELECT ... - { - /// AS [db.]table - if (!name_p.parse(pos, as_table, expected)) - return false; - - if (s_dot.ignore(pos, expected)) - { - as_database = as_table; - if (!name_p.parse(pos, as_table, expected)) - return false; - } - - /// Optional - ENGINE can be specified. - storage_p.parse(pos, storage, expected); - } + return false; } } - else if (is_temporary) - return false; else if (s_database.ignore(pos, expected)) { if (s_if_not_exists.ignore(pos, expected)) @@ -325,70 +281,7 @@ bool ParserCreateQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected) } else { - /// VIEW or MATERIALIZED VIEW - if (s_materialized.ignore(pos, expected)) - { - is_materialized_view = true; - } - else - is_view = true; - - if (!s_view.ignore(pos, expected)) - return false; - - if (s_if_not_exists.ignore(pos, expected)) - if_not_exists = true; - - if (!name_p.parse(pos, table, expected)) - return false; - - if (s_dot.ignore(pos, expected)) - { - database = table; - if (!name_p.parse(pos, table, expected)) - return false; - } - - // TO [db.]table - if (ParserKeyword{"TO"}.ignore(pos, expected)) - { - if (!name_p.parse(pos, to_table, expected)) - return false; - - if (s_dot.ignore(pos, expected)) - { - to_database = to_table; - if (!name_p.parse(pos, to_table, expected)) - return false; - } - } - - /// Optional - a list of columns can be specified. It must fully comply with SELECT. - if (s_lparen.ignore(pos, expected)) - { - if (!columns_p.parse(pos, columns, expected)) - return false; - - if (!s_rparen.ignore(pos, expected)) - return false; - } - - if (is_materialized_view && !to_table) - { - /// Internal ENGINE for MATERIALIZED VIEW must be specified. - if (!storage_p.parse(pos, storage, expected)) - return false; - - if (s_populate.ignore(pos, expected)) - is_populate = true; - } - - /// AS SELECT ... - if (!s_as.ignore(pos, expected)) - return false; - - if (!select_p.parse(pos, select, expected)) - return false; + return false; } auto query = std::make_shared(); @@ -396,28 +289,14 @@ bool ParserCreateQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected) query->attach = attach; query->if_not_exists = if_not_exists; - query->is_view = is_view; - query->is_materialized_view = is_materialized_view; - query->is_populate = is_populate; - query->is_temporary = is_temporary; if (database) query->database = typeid_cast(*database).name; if (table) query->table = typeid_cast(*table).name; - if (to_database) - query->to_database = typeid_cast(*to_database).name; - if (to_table) - query->to_table = typeid_cast(*to_table).name; - query->set(query->columns, columns); query->set(query->storage, storage); - if (as_database) - query->as_database = typeid_cast(*as_database).name; - if (as_table) - query->as_table = typeid_cast(*as_table).name; - query->set(query->select, select); return true; } diff --git a/dbms/src/Parsers/ParserCreateQuery.h b/dbms/src/Parsers/ParserCreateQuery.h index 8b9356257b9..feb8e6abda5 100644 --- a/dbms/src/Parsers/ParserCreateQuery.h +++ b/dbms/src/Parsers/ParserCreateQuery.h @@ -33,8 +33,8 @@ namespace DB class ParserNestedTable : public IParserBase { protected: - const char * getName() const { return "nested table"; } - bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected); + const char * getName() const override { return "nested table"; } + bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected) override; }; @@ -47,8 +47,8 @@ class ParserNestedTable : public IParserBase class ParserIdentifierWithParameters : public IParserBase { protected: - const char * getName() const { return "identifier with parameters"; } - bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected); + const char * getName() const override { return "identifier with parameters"; } + bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected) override; }; @@ -58,15 +58,15 @@ class ParserIdentifierWithParameters : public IParserBase class ParserIdentifierWithOptionalParameters : public IParserBase { protected: - const char * getName() const { return "identifier with optional parameters"; } - bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected); + const char * getName() const override { return "identifier with optional parameters"; } + bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected) override; }; class ParserTypeInCastExpression : public IParserBase { protected: - const char * getName() const { return "type in cast expression"; } - bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected); + const char * getName() const override { return "type in cast expression"; } + bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected) override; }; @@ -74,8 +74,8 @@ template class IParserNameTypePair : public IParserBase { protected: - const char * getName() const { return "name and type pair"; } - bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected); + const char * getName() const override { return "name and type pair"; } + bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected) override; }; /** The name and type are separated by a space. For example, URL String. */ @@ -107,8 +107,8 @@ bool IParserNameTypePair::parseImpl(Pos & pos, ASTPtr & node, Expect class ParserNameTypePairList : public IParserBase { protected: - const char * getName() const { return "name and type pair list"; } - bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected); + const char * getName() const override { return "name and type pair list"; } + bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected) override; }; @@ -116,8 +116,8 @@ template class IParserColumnDeclaration : public IParserBase { protected: - const char * getName() const { return "column declaration"; } - bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected); + const char * getName() const override { return "column declaration"; } + bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected) override; }; using ParserColumnDeclaration = IParserColumnDeclaration; @@ -187,8 +187,8 @@ bool IParserColumnDeclaration::parseImpl(Pos & pos, ASTPtr & node, E class ParserColumnDeclarationList : public IParserBase { protected: - const char * getName() const { return "column declaration list"; } - bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected); + const char * getName() const override { return "column declaration list"; } + bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected) override; }; @@ -196,8 +196,8 @@ class ParserColumnDeclarationList : public IParserBase class ParserStorage : public IParserBase { protected: - const char * getName() const { return "storage definition"; } - bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected); + const char * getName() const override { return "storage definition"; } + bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected) override; }; @@ -213,19 +213,13 @@ class ParserStorage : public IParserBase * CREATE|ATTACH TABLE [IF NOT EXISTS] [db.]name AS [db2.]name2 [ENGINE = engine] * * Or: - * CREATE|ATTACH TABLE [IF NOT EXISTS] [db.]name AS ENGINE = engine SELECT ... - * - * Or: * CREATE|ATTACH DATABASE db [ENGINE = engine] - * - * Or: - * CREATE|ATTACH [MATERIALIZED] VIEW [IF NOT EXISTS] [db.]name [TO [db.]name] [ENGINE = engine] [POPULATE] AS SELECT ... */ class ParserCreateQuery : public IParserBase { protected: - const char * getName() const { return "CREATE TABLE or ATTACH TABLE query"; } - bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected); + const char * getName() const override { return "CREATE TABLE or ATTACH TABLE query"; } + bool parseImpl(Pos & pos, ASTPtr & node, Expected & expected) override; }; } // namespace DB diff --git a/dbms/src/Storages/KVStore/tests/gtest_sync_schema.cpp b/dbms/src/Storages/KVStore/tests/gtest_sync_schema.cpp index d473b91eb48..7ef8725ccf6 100644 --- a/dbms/src/Storages/KVStore/tests/gtest_sync_schema.cpp +++ b/dbms/src/Storages/KVStore/tests/gtest_sync_schema.cpp @@ -15,12 +15,6 @@ #include #include #include -#include -#include -#include -#include -#include -#include #include #include #include @@ -35,19 +29,17 @@ #include #include -namespace DB -{ -namespace ErrorCodes +namespace DB::ErrorCodes { extern const int SYNTAX_ERROR; -} // namespace ErrorCodes +} // namespace DB::ErrorCodes -namespace FailPoints +namespace DB::FailPoints { extern const char sync_schema_request_failure[]; -} // namespace FailPoints +} // namespace DB::FailPoints -namespace tests +namespace DB::tests { class SyncSchemaTest : public ::testing::Test { @@ -173,5 +165,4 @@ try } CATCH -} // namespace tests -} // namespace DB +} // namespace DB::tests diff --git a/dbms/src/Storages/StorageFactory.cpp b/dbms/src/Storages/StorageFactory.cpp index 66aee43022f..aac3a6c8f42 100644 --- a/dbms/src/Storages/StorageFactory.cpp +++ b/dbms/src/Storages/StorageFactory.cpp @@ -41,17 +41,18 @@ static void checkAllTypesAreAllowedInTable(const NamesAndTypesList & names_and_t for (const auto & elem : names_and_types) if (elem.type->cannotBeStoredInTables()) throw Exception( - "Data type " + elem.type->getName() + " cannot be used in tables", - ErrorCodes::DATA_TYPE_CANNOT_BE_USED_IN_TABLES); + ErrorCodes::DATA_TYPE_CANNOT_BE_USED_IN_TABLES, + "Data type {} cannot be used in tables", + elem.type->getName()); } void StorageFactory::registerStorage(const std::string & name, Creator creator) { - if (!storages.emplace(name, std::move(creator)).second) - throw Exception( - "TableFunctionFactory: the table function name '" + name + "' is not unique", - ErrorCodes::LOGICAL_ERROR); + RUNTIME_CHECK_MSG( + storages.emplace(name, std::move(creator)).second, + "TableFunctionFactory: the table function name '{}' is not unique", + name); } @@ -67,68 +68,34 @@ StoragePtr StorageFactory::get( bool attach, bool has_force_restore_data_flag) const { - String name; - ASTs args; ASTStorage * storage_def = query.storage; - if (query.is_view) - { - if (query.storage) - throw Exception("Specifying ENGINE is not allowed for a View", ErrorCodes::INCORRECT_QUERY); + /// Check for some special types, that are not allowed to be stored in tables. Example: NULL data type. + /// Exception: any type is allowed in View, because plain (non-materialized) View does not store anything itself. + checkAllTypesAreAllowedInTable(columns.getAll()); - name = "View"; - } - else + if (!storage_def) + throw Exception("Incorrect CREATE query: ENGINE required", ErrorCodes::ENGINE_REQUIRED); + + const ASTFunction & engine_def = *storage_def->engine; + if (engine_def.parameters) + throw Exception( + "Engine definition cannot take the form of a parametric function", + ErrorCodes::FUNCTION_CANNOT_HAVE_PARAMETERS); + ASTs args; + if (engine_def.arguments) + args = engine_def.arguments->children; + + String name = engine_def.name; + + if ((storage_def->partition_by || storage_def->order_by || storage_def->sample_by || storage_def->settings) + && !endsWith(name, "MergeTree")) { - /// Check for some special types, that are not allowed to be stored in tables. Example: NULL data type. - /// Exception: any type is allowed in View, because plain (non-materialized) View does not store anything itself. - checkAllTypesAreAllowedInTable(columns.getAll()); - - if (query.is_materialized_view) - { - name = "MaterializedView"; - } - else - { - if (!storage_def) - throw Exception("Incorrect CREATE query: ENGINE required", ErrorCodes::ENGINE_REQUIRED); - - const ASTFunction & engine_def = *storage_def->engine; - - if (engine_def.parameters) - throw Exception( - "Engine definition cannot take the form of a parametric function", - ErrorCodes::FUNCTION_CANNOT_HAVE_PARAMETERS); - - if (engine_def.arguments) - args = engine_def.arguments->children; - - name = engine_def.name; - - if ((storage_def->partition_by || storage_def->order_by || storage_def->sample_by || storage_def->settings) - && !endsWith(name, "MergeTree")) - { - throw Exception( - "Engine " + name - + " doesn't support PARTITION BY, ORDER BY, SAMPLE BY or SETTINGS clauses. " - "Currently only the MergeTree family of engines supports them", - ErrorCodes::BAD_ARGUMENTS); - } - - if (name == "View") - { - throw Exception( - "Direct creation of tables with ENGINE View is not supported, use CREATE VIEW statement", - ErrorCodes::INCORRECT_QUERY); - } - else if (name == "MaterializedView") - { - throw Exception( - "Direct creation of tables with ENGINE MaterializedView is not supported, use CREATE MATERIALIZED " - "VIEW statement", - ErrorCodes::INCORRECT_QUERY); - } - } + throw Exception( + ErrorCodes::BAD_ARGUMENTS, + "Engine {} doesn't support PARTITION BY, ORDER BY, SAMPLE BY or SETTINGS clauses. " + "Currently only the MergeTree family of engines supports them", + name); } auto it = storages.find(name);