rename/exchange database/table/dictionary support IF EXISTS syntax #31081

kafka1991 · 2021-11-04T14:37:15Z

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Support IF EXISTS modifier for RENAME DATABASE/TABLE/DICTIONARY query, If this directive is used, one will not get an error if the DATABASE/TABLE/DICTIONARY to be renamed doesn't exist.

kafka1991 · 2021-11-05T03:59:57Z

Needs 'can be tested' label
@alexey-milovidov

tavplubix · 2021-11-06T15:27:24Z

tests/queries/0_stateless/01109_exchange_tables.sql

@@ -13,17 +13,23 @@ CREATE TABLE t2 ENGINE=MergeTree() ORDER BY tuple() AS SELECT rowNumberInAllBloc
 EXCHANGE TABLES t1 AND t3; -- { serverError 60 }
 EXCHANGE TABLES t4 AND t2; -- { serverError 60 }
 RENAME TABLE t0 TO t1; -- { serverError 57 }
-DROP TABLE t1;
-RENAME TABLE t0 TO t1;
+


It's better to make new test for these queries. Or, at least, add new queries to the end of this test without modifying existing queries.

ACK, has fixed.

tavplubix · 2021-11-06T15:29:29Z

src/Parsers/ParserRenameQuery.cpp

@@ -67,6 +68,7 @@ bool ParserRenameQuery::parseImpl(Pos & pos, ASTPtr & node, Expected & expected)
        ASTPtr from_db;
        ASTPtr to_db;
        ParserIdentifier db_name_p;
+        bool if_exists = s_if_exists.ignore(pos, expected);


Also need to update ASTRenameQuery::formatQueryImpl(...) to make formatter consistent with parser.

tavplubix · 2021-11-06T15:37:03Z

src/Interpreters/InterpreterRenameQuery.cpp

-            return typeid_cast<DatabaseReplicated *>(database.get())->tryEnqueueReplicatedDDL(query_ptr, getContext());
+            if (rename.exchange)
+            {
+                ignore = rename.dictionary ? (!database_catalog.isDictionaryExist(StorageID(elem.to_database_name, elem.to_table_name)) ||


Seems like special check for dictionaries is not needed, because if an DDL-dictionary exists, then isTableExist(...) returns true.

ACK
It is impossible for table/dictionary with the same StorageID to exist in the same database

tavplubix · 2021-11-06T15:48:26Z

src/Interpreters/InterpreterRenameQuery.cpp

+            bool exchange_tables;
+            if (rename.exchange)
+            {
+                exchange_tables = true;
+            }
+            else if (rename.rename_if_cannot_exchange)
+            {
+                exchange_tables = database_catalog.isTableExist(StorageID(elem.to_database_name, elem.to_table_name), getContext());
+                renamed_instead_of_exchange = !exchange_tables;
+            }
+            else
+            {
+                exchange_tables = false;
+                database_catalog.assertTableDoesntExist(StorageID(elem.to_database_name, elem.to_table_name), getContext());
+            }


Will this logic work correctly after the changes?

yeah, it work correctly.
when rename.rename_if_cannot_exchange is true, rename.exchange is false, in which situation only check the source table is exists or not.

tavplubix · 2021-11-06T15:56:31Z

src/Interpreters/InterpreterRenameQuery.cpp

+                ignore = rename.dictionary ? (!database_catalog.isDictionaryExist(StorageID(elem.to_database_name, elem.to_table_name))) :
+                    (!database_catalog.isTableExist(StorageID(elem.to_database_name, elem.to_table_name), getContext()));


Do you mean from_database_name and from_table_name?

tavplubix · 2021-11-06T16:23:26Z

Thanks for contributing into ClickHouse!

The RENAME ... IF EXISTS a TO b query, that renames a to b if a exists and does nothing if a doesn't exist, makes sense. I've just found that people ask for this feature in MySQL for 17 years :)

However, I'm not sure what is the semantics of EXCHANGE ... IF EXISTS a AND b and how should it work, because EXCHANGE query requires that both a and b must exist, so it's not clear to which table (or dictionary) this condition should be applied and what EXCHANGE query should do if only one table (or dictionary) exists. We have rename_if_cannot_exchange flag that means "exchange a and b tables if both tables exist or rename a to b if b does not", but it's not available for users (it's only used internally in the CREATE OR REPLACE query). Maybe we can make it available, but I think that IF EXISTS is not suitable modifier, we need another syntax for this (and we need to think it over).

So I suggest not to consider EXCHANGE query for now and allow IF EXISTS modifier for RENAME query only.

kafka1991 · 2021-11-08T08:43:25Z

Thanks for contributing into ClickHouse!

The RENAME ... IF EXISTS a TO b query, that renames a to b if a exists and does nothing if a doesn't exist, makes sense. I've just found that people ask for this feature in MySQL for 17 years :)

However, I'm not sure what is the semantics of EXCHANGE ... IF EXISTS a AND b and how should it work, because EXCHANGE query requires that both a and b must exist, so it's not clear to which table (or dictionary) this condition should be applied and what EXCHANGE query should do if only one table (or dictionary) exists. We have rename_if_cannot_exchange flag that means "exchange a and b tables if both tables exist or rename a to b if b does not", but it's not available for users (it's only used internally in the CREATE OR REPLACE query). Maybe we can make it available, but I think that IF EXISTS is not suitable modifier, we need another syntax for this (and we need to think it over).

So I suggest not to consider EXCHANGE query for now and allow IF EXISTS modifier for RENAME query only.

For EXCHANGE ... IF EXISTS a AND b, the semantics of if existsdoes unclear, we should find a more suitable modifier. So we keep the IF EXISTS implementation and limit the sql interface here for EXCHANGE.

kafka1991 · 2021-11-10T07:04:12Z

the failed case of Integration tests has nothing to do with this pr.

Merging #31081

rename/exchange database/table/dictionary support IF EXISTS syntax

ec2c70a

robot-clickhouse added the pr-improvement Pull request with some product improvements label Nov 4, 2021

alexey-milovidov mentioned this pull request Nov 4, 2021

Rename support if exist 1 #31078

Closed

qoega added the can be tested label Nov 5, 2021

tavplubix self-assigned this Nov 6, 2021

tavplubix reviewed Nov 6, 2021

View reviewed changes

kafka1991 added 2 commits November 8, 2021 15:14

code review

6020c23

rename database/table/dictionary support IF EXISTS syntax

8e7023c

kafka1991 added 2 commits November 8, 2021 16:49

code style

cdb1227

code style

209d1a2

tavplubix approved these changes Nov 11, 2021

View reviewed changes

tavplubix added a commit that referenced this pull request Nov 12, 2021

Merge pull request #31305 from ClickHouse/rename_support_ifexists

8916d3b

Merging #31081

tavplubix merged commit 209d1a2 into ClickHouse:master Nov 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rename/exchange database/table/dictionary support IF EXISTS syntax #31081

rename/exchange database/table/dictionary support IF EXISTS syntax #31081

kafka1991 commented Nov 4, 2021 •

edited by tavplubix

kafka1991 commented Nov 5, 2021

tavplubix Nov 6, 2021

kafka1991 Nov 8, 2021

tavplubix Nov 6, 2021

kafka1991 Nov 8, 2021

tavplubix Nov 6, 2021

kafka1991 Nov 8, 2021 •

edited

tavplubix Nov 6, 2021

kafka1991 Nov 8, 2021 •

edited

tavplubix Nov 6, 2021

kafka1991 Nov 8, 2021

tavplubix commented Nov 6, 2021

kafka1991 commented Nov 8, 2021 •

edited

kafka1991 commented Nov 10, 2021

		ignore = rename.dictionary ? (!database_catalog.isDictionaryExist(StorageID(elem.to_database_name, elem.to_table_name))) :
		(!database_catalog.isTableExist(StorageID(elem.to_database_name, elem.to_table_name), getContext()));

rename/exchange database/table/dictionary support IF EXISTS syntax #31081

rename/exchange database/table/dictionary support IF EXISTS syntax #31081

Conversation

kafka1991 commented Nov 4, 2021 • edited by tavplubix

kafka1991 commented Nov 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kafka1991 Nov 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kafka1991 Nov 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tavplubix commented Nov 6, 2021

kafka1991 commented Nov 8, 2021 • edited

kafka1991 commented Nov 10, 2021

kafka1991 commented Nov 4, 2021 •

edited by tavplubix

kafka1991 Nov 8, 2021 •

edited

kafka1991 Nov 8, 2021 •

edited

kafka1991 commented Nov 8, 2021 •

edited