Merge pull request #61013 from ClickHouse/vdimir/fix_astrename_clone

Fix ASTRenameQuery::clone

src/Interpreters/AddDefaultDatabaseVisitor.h

@@ -275,13 +275,7 @@ private:
         if (only_replace_current_database_function)
             return;
 
-        for (ASTRenameQuery::Element & elem : node.elements)
-        {
-            if (!elem.from.database)
-                elem.from.database = std::make_shared<ASTIdentifier>(database_name);
-            if (!elem.to.database)
-                elem.to.database = std::make_shared<ASTIdentifier>(database_name);
-        }
+        node.setDatabaseIfNotExists(database_name);
     }
 
     void visitDDL(ASTAlterQuery & node, ASTPtr &) const

src/Interpreters/InterpreterCreateQuery.cpp

@@ -1612,7 +1612,6 @@ BlockIO InterpreterCreateQuery::doCreateOrReplaceTable(ASTCreateQuery & create,
         executeTrivialBlockIO(fill_io, getContext());
 
         /// Replace target table with created one
-        auto ast_rename = std::make_shared<ASTRenameQuery>();
         ASTRenameQuery::Element elem
         {
             ASTRenameQuery::Table
@@ -1627,7 +1626,7 @@ BlockIO InterpreterCreateQuery::doCreateOrReplaceTable(ASTCreateQuery & create,
             }
         };
 
-        ast_rename->elements.push_back(std::move(elem));
+        auto ast_rename = std::make_shared<ASTRenameQuery>(ASTRenameQuery::Elements{std::move(elem)});
         ast_rename->dictionary = create.is_dictionary;
         if (create.create_or_replace)
         {

src/Interpreters/InterpreterRenameQuery.cpp

@@ -47,12 +47,12 @@ BlockIO InterpreterRenameQuery::execute()
       */
 
     RenameDescriptions descriptions;
-    descriptions.reserve(rename.elements.size());
+    descriptions.reserve(rename.getElements().size());
 
     /// Don't allow to drop tables (that we are renaming); don't allow to create tables in places where tables will be renamed.
     TableGuards table_guards;
 
-    for (const auto & elem : rename.elements)
+    for (const auto & elem : rename.getElements())
     {
         descriptions.emplace_back(elem, current_database);
         const auto & description = descriptions.back();
@@ -186,7 +186,7 @@ AccessRightsElements InterpreterRenameQuery::getRequiredAccess(InterpreterRename
 {
     AccessRightsElements required_access;
     const auto & rename = query_ptr->as<const ASTRenameQuery &>();
-    for (const auto & elem : rename.elements)
+    for (const auto & elem : rename.getElements())
     {
         if (type == RenameType::RenameTable)
         {
@@ -214,7 +214,7 @@ AccessRightsElements InterpreterRenameQuery::getRequiredAccess(InterpreterRename
 void InterpreterRenameQuery::extendQueryLogElemImpl(QueryLogElement & elem, const ASTPtr & ast, ContextPtr) const
 {
     const auto & rename = ast->as<const ASTRenameQuery &>();
-    for (const auto & element : rename.elements)
+    for (const auto & element : rename.getElements())
     {
         {
             String database = backQuoteIfNeed(!element.from.database ? getContext()->getCurrentDatabase() : element.from.getDatabase());

src/Interpreters/MySQL/InterpretersMySQLDDLQuery.cpp

@@ -579,7 +579,7 @@ ASTs InterpreterRenameImpl::getRewrittenQueries(
     const InterpreterRenameImpl::TQuery & rename_query, ContextPtr context, const String & mapped_to_database, const String & mysql_database)
 {
     ASTRenameQuery::Elements elements;
-    for (const auto & rename_element : rename_query.elements)
+    for (const auto & rename_element : rename_query.getElements())
     {
         const auto & to_database = resolveDatabase(rename_element.to.getDatabase(), mysql_database, mapped_to_database, context);
         const auto & from_database = resolveDatabase(rename_element.from.getDatabase(), mysql_database, mapped_to_database, context);
@@ -600,8 +600,7 @@ ASTs InterpreterRenameImpl::getRewrittenQueries(
     if (elements.empty())
         return ASTs{};
 
-    auto rewritten_query = std::make_shared<ASTRenameQuery>();
-    rewritten_query->elements = elements;
+    auto rewritten_query = std::make_shared<ASTRenameQuery>(std::move(elements));
     return ASTs{rewritten_query};
 }
 
@@ -616,7 +615,8 @@ ASTs InterpreterAlterImpl::getRewrittenQueries(
         return {};
 
     auto rewritten_alter_query = std::make_shared<ASTAlterQuery>();
-    auto rewritten_rename_query = std::make_shared<ASTRenameQuery>();
+    ASTRenameQuery::Elements rename_elements;
+
     rewritten_alter_query->setDatabase(mapped_to_database);
     rewritten_alter_query->setTable(alter_query.table);
     rewritten_alter_query->alter_object = ASTAlterQuery::AlterObjectType::TABLE;
@@ -749,13 +749,13 @@ ASTs InterpreterAlterImpl::getRewrittenQueries(
 
             /// For ALTER TABLE table_name RENAME TO new_table_name_1, RENAME TO new_table_name_2;
             /// We just need to generate RENAME TABLE table_name TO new_table_name_2;
-            if (rewritten_rename_query->elements.empty())
-                rewritten_rename_query->elements.push_back(ASTRenameQuery::Element());
+            if (rename_elements.empty())
+                rename_elements.push_back(ASTRenameQuery::Element());
 
-            rewritten_rename_query->elements.back().from.database = std::make_shared<ASTIdentifier>(mapped_to_database);
-            rewritten_rename_query->elements.back().from.table = std::make_shared<ASTIdentifier>(alter_query.table);
-            rewritten_rename_query->elements.back().to.database = std::make_shared<ASTIdentifier>(mapped_to_database);
-            rewritten_rename_query->elements.back().to.table = std::make_shared<ASTIdentifier>(alter_command->new_table_name);
+            rename_elements.back().from.database = std::make_shared<ASTIdentifier>(mapped_to_database);
+            rename_elements.back().from.table = std::make_shared<ASTIdentifier>(alter_query.table);
+            rename_elements.back().to.database = std::make_shared<ASTIdentifier>(mapped_to_database);
+            rename_elements.back().to.table = std::make_shared<ASTIdentifier>(alter_command->new_table_name);
         }
     }
 
@@ -765,8 +765,11 @@ ASTs InterpreterAlterImpl::getRewrittenQueries(
     if (!rewritten_alter_query->command_list->children.empty())
         rewritten_queries.push_back(rewritten_alter_query);
 
-    if (!rewritten_rename_query->elements.empty())
+    if (!rename_elements.empty())
+    {
+        auto rewritten_rename_query = std::make_shared<ASTRenameQuery>(std::move(rename_elements));
         rewritten_queries.push_back(rewritten_rename_query);
+    }
 
     return rewritten_queries;
 }

src/Interpreters/SystemLog.cpp

@@ -563,7 +563,6 @@ void SystemLog<LogElement>::prepareTable()
                 {table_id.database_name, table_id.table_name + "_" + toString(suffix)}, getContext()))
                 ++suffix;
 
-            auto rename = std::make_shared<ASTRenameQuery>();
             ASTRenameQuery::Element elem
             {
                 ASTRenameQuery::Table
@@ -586,7 +585,7 @@ void SystemLog<LogElement>::prepareTable()
                 old_create_query,
                 create_query);
 
-            rename->elements.emplace_back(std::move(elem));
+            auto rename = std::make_shared<ASTRenameQuery>(ASTRenameQuery::Elements{std::move(elem)});
 
             ActionLock merges_lock;
             if (DatabaseCatalog::instance().getDatabase(table_id.database_name)->getUUID() == UUIDHelpers::Nil)

src/Parsers/ASTRenameQuery.h

@@ -45,7 +45,6 @@ public:
     };
 
     using Elements = std::vector<Element>;
-    Elements elements;
 
     bool exchange{false};   /// For EXCHANGE TABLES
     bool database{false};   /// For RENAME DATABASE
@@ -54,12 +53,48 @@ public:
     /// Special flag for CREATE OR REPLACE. Do not throw if the second table does not exist.
     bool rename_if_cannot_exchange{false};
 
+    explicit ASTRenameQuery(Elements elements_ = {})
+        : elements(std::move(elements_))
+    {
+        for (const auto & elem : elements)
+        {
+            if (elem.from.database)
+                children.push_back(elem.from.database);
+            if (elem.from.table)
+                children.push_back(elem.from.table);
+            if (elem.to.database)
+                children.push_back(elem.to.database);
+            if (elem.to.table)
+                children.push_back(elem.to.table);
+        }
+    }
+
+    void setDatabaseIfNotExists(const String & database_name)
+    {
+        for (auto & elem : elements)
+        {
+            if (!elem.from.database)
+            {
+                elem.from.database = std::make_shared<ASTIdentifier>(database_name);
+                children.push_back(elem.from.database);
+            }
+            if (!elem.to.database)
+            {
+                elem.to.database = std::make_shared<ASTIdentifier>(database_name);
+                children.push_back(elem.to.database);
+            }
+        }
+    }
+
+    const Elements & getElements() const { return elements; }
+
     /** Get the text that identifies this element. */
     String getID(char) const override { return "Rename"; }
 
     ASTPtr clone() const override
     {
         auto res = std::make_shared<ASTRenameQuery>(*this);
+        res->cloneChildren();
         cloneOutputOptions(*res);
         return res;
     }
@@ -145,6 +180,8 @@ protected:
 
         formatOnCluster(settings);
     }
+
+    Elements elements;
 };
 
 }

src/Parsers/ParserRenameQuery.cpp

@@ -44,15 +44,14 @@ bool ParserRenameQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected)
             if (!ASTQueryWithOnCluster::parse(pos, cluster_str, expected))
                 return false;
         }
+        ASTRenameQuery::Elements rename_elements;
+        rename_elements.emplace_back();
+        rename_elements.back().if_exists = if_exists;
+        rename_elements.back().from.database = from_db;
+        rename_elements.back().to.database = to_db;
 
-        auto query = std::make_shared<ASTRenameQuery>();
+        auto query = std::make_shared<ASTRenameQuery>(std::move(rename_elements));
         query->database = true;
-        query->elements.emplace({});
-        query->elements.front().if_exists = if_exists;
-        query->elements.front().from.database = from_db;
-        query->elements.front().to.database = to_db;
-        query->children.push_back(std::move(from_db));
-        query->children.push_back(std::move(to_db));
         query->cluster = cluster_str;
         node = query;
         return true;
@@ -75,9 +74,8 @@ bool ParserRenameQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected)
 
     const auto ignore_delim = [&] { return exchange ? s_and.ignore(pos) : s_to.ignore(pos); };
 
-    auto query = std::make_shared<ASTRenameQuery>();
 
-    ASTRenameQuery::Elements & elements = query->elements;
+    ASTRenameQuery::Elements elements;
 
     while (true)
     {
@@ -93,15 +91,6 @@ bool ParserRenameQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected)
             || !ignore_delim()
             || !parseDatabaseAndTableAsAST(pos, expected, ref.to.database, ref.to.table))
             return false;
-
-        if (ref.from.database)
-            query->children.push_back(ref.from.database);
-        if (ref.from.table)
-            query->children.push_back(ref.from.table);
-        if (ref.to.database)
-            query->children.push_back(ref.to.database);
-        if (ref.to.table)
-            query->children.push_back(ref.to.table);
     }
 
     String cluster_str;
@@ -111,6 +100,7 @@ bool ParserRenameQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected)
             return false;
     }
 
+    auto query = std::make_shared<ASTRenameQuery>(std::move(elements));
     query->cluster = cluster_str;
     query->exchange = exchange;
     query->dictionary = dictionary;

src/Parsers/tests/gtest_Parser.cpp

@@ -9,6 +9,7 @@
 #include <Parsers/ParserAlterQuery.h>
 #include <Parsers/ParserCreateQuery.h>
 #include <Parsers/ParserOptimizeQuery.h>
+#include <Parsers/ParserRenameQuery.h>
 #include <Parsers/ParserQueryWithOutput.h>
 #include <Parsers/ParserAttachAccessEntity.h>
 #include <Parsers/formatAST.h>
@@ -62,10 +63,29 @@ TEST_P(ParserTest, parseQuery)
             if (std::string("CREATE USER or ALTER USER query") != parser->getName()
                     && std::string("ATTACH access entity query") != parser->getName())
             {
-                WriteBufferFromOwnString buf;
-                formatAST(*ast->clone(), buf, false, false);
-                String formatted_ast = buf.str();
-                EXPECT_EQ(expected_ast, formatted_ast);
+                ASTPtr ast_clone = ast->clone();
+                {
+                    WriteBufferFromOwnString buf;
+                    formatAST(*ast_clone, buf, false, false);
+                    String formatted_ast = buf.str();
+                    EXPECT_EQ(expected_ast, formatted_ast);
+                }
+
+
+                ASTPtr ast_clone2 = ast_clone->clone();
+                /// Break `ast_clone2`, it should not affect `ast_clone` if `clone()` implemented properly
+                for (auto & child : ast_clone2->children)
+                {
+                    if (auto * identifier = dynamic_cast<ASTIdentifier *>(child.get()))
+                        identifier->setShortName("new_name");
+                }
+
+                {
+                    WriteBufferFromOwnString buf;
+                    formatAST(*ast_clone, buf, false, false);
+                    String formatted_ast = buf.str();
+                    EXPECT_EQ(expected_ast, formatted_ast);
+                }
             }
             else
             {
@@ -299,6 +319,16 @@ INSTANTIATE_TEST_SUITE_P(ParserAttachUserQuery, ParserTest,
         }
 })));
 
+INSTANTIATE_TEST_SUITE_P(ParserRenameQuery, ParserTest,
+    ::testing::Combine(
+        ::testing::Values(std::make_shared<ParserRenameQuery>()),
+        ::testing::ValuesIn(std::initializer_list<ParserTestCase>{
+        {
+            "RENAME TABLE eligible_test TO eligible_test2",
+            "RENAME TABLE eligible_test TO eligible_test2"
+        }
+})));
+
 INSTANTIATE_TEST_SUITE_P(ParserKQLQuery, ParserKQLTest,
     ::testing::Combine(
         ::testing::Values(std::make_shared<ParserKQLQuery>()),

src/Storages/StorageMaterializedView.cpp

@@ -465,8 +465,8 @@ void StorageMaterializedView::renameInMemory(const StorageID & new_table_id)
     if (!from_atomic_to_atomic_database && has_inner_table && tryGetTargetTable())
     {
         auto new_target_table_name = generateInnerTableName(new_table_id);
-        auto rename = std::make_shared<ASTRenameQuery>();
 
+        ASTRenameQuery::Elements rename_elements;
         assert(inner_table_id.database_name == old_table_id.database_name);
 
         ASTRenameQuery::Element elem
@@ -482,8 +482,9 @@ void StorageMaterializedView::renameInMemory(const StorageID & new_table_id)
                 std::make_shared<ASTIdentifier>(new_target_table_name)
             }
         };
-        rename->elements.emplace_back(std::move(elem));
+        rename_elements.emplace_back(std::move(elem));
 
+        auto rename = std::make_shared<ASTRenameQuery>(std::move(rename_elements));
         InterpreterRenameQuery(rename, getContext()).execute();
         updateTargetTableId(new_table_id.database_name, new_target_table_name);
     }