Implement renameSchema for ClickHouse

[tangjiangling]

Fixes: #10558

The current implementation checks if it is an ATOMIC
database in ClickHouseClient when RENAME SCHEMA

[cla-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla.

[tangjiangling]

cc @hashhar

[hashhar]

Thanks for this. Looks good.

Couple of comments to make it more future proof.

• Let's try to do renameSchema unconditionally.
• Instead of overriding renameSchema we can override renameSchemaSql to return something like "RENAME DATABASE " + quoted(schemaName) + " TO " + quoted(newSchemaName).
• Let's add a class called TestClickhouseLatestConnectorSmokeTest and use a newer Clickhouse version there. See TestSqlServerLatestConnectorSmokeTest for example. This will allow us to test that rename schema works out of box when used with newer clickhouse versions.
• Let's add one more test method to TestClickhouseConnectorTest which creates a new database directly on Clickhouse with the ATOMIC engine and verify that rename schema works for it. That way we can also test that rename schema works with older versions of Clickhouse too if the database engine is ATOMIC.

[hashhar]

Let's try to do renameSchema unconditionally.

The reasoning for this is that we already do this for some other methods in some connectors. And it's possible that our assumptions today will become false in future if there are more changes in Clickhouse.

The only additional thing we should do is to make sure that the error message received when renaming a non-ATOMIC database is still useful/actionable for the user.

[tangjiangling]

@hashhar @ebyhr Thanks for your nice guidance, I will update the current PR by this Sunday.

[ebyhr]

@tangjiangling Could you rebase on upstream to resolve conflicts?

[tangjiangling]

@tangjiangling Could you rebase on upstream to resolve conflicts?

Okay, I will update the current PR as soon as possible.

[tangjiangling]

Okay, I will update the current PR as soon as possible.

Updated, still some TOODs that can be seen in the code.

[tangjiangling]

Let's add one more test method to TestClickhouseConnectorTest which creates a new database directly on Clickhouse with the ATOMIC engine and verify that rename schema works for it. That way we can also test that rename schema works with older versions of Clickhouse too if the database engine is ATOMIC.

This requires us to support ClickHouse database properties for CREATE SCHEMA ... WITH (e.g.: CREATE SCHEMA ... WITH (engine = Atomic)).

Is this in our follow-up planning?

If it is, I'd like to do it.

[hashhar]

@tangjiangling Regarding the new test you can use onRemoteDatabase to execute the CREATE DATABASE test_rename_atomic ENGINE = atomic within Clickhouse and then use Trino to try to execute ALTER SCHEMA test_rename_atomic RENAME TO some_new_name.

[tangjiangling]

@tangjiangling Regarding the new test you can use onRemoteDatabase to execute the CREATE DATABASE test_rename_atomic ENGINE = atomic within Clickhouse and then use Trino to try to execute ALTER SCHEMA test_rename_atomic RENAME TO some_new_name.

Okay, I'll take a look.

[tangjiangling]

The only additional thing we should do is to make sure that the error message received when renaming a non-ATOMIC database is still useful/actionable for the user.

<"ClickHouse exception, code: 48, host: localhost, port: 57137; Code: 48, e.displayText() = DB::Exception: Ordinary: RENAME DATABASE is not supported (version 20.8.19.4 (official build))
">

The full error message is as above, which I think is sufficient for the user.

[tangjiangling]

Okay, I'll take a look.

Updated.

[ebyhr]

Could you add below two lines to docs/src/main/sphinx/connector/clickhouse.rst?

• * :doc:`/sql/alter-schema` at L192
• .. include:: alter-schema-limitation.fragment at L194

[tangjiangling]

Could add below two lines to docs/src/main/sphinx/connector/clickhouse.rst?

• * :doc:`/sql/alter-schema` at L192
• .. include:: alter-schema-limitation.fragment at L194

Already added and verified in local build.

[hashhar]

Squash all the commits. Features and tests should be in same commit. It makes bisecting failures in future a lot easier (and also ensures that a later commit doesn't fix something in a previous commit - i.e. all commits build and pass tests)

Thanks for working on this.

[tangjiangling]

Squash all the commits. Features and tests should be in same commit. It makes bisecting failures in future a lot easier (and also ensures that a later commit doesn't fix something in a previous commit - i.e. all commits build and pass tests)

Good suggestion.

@hashhar One more question: does the commit about document also need to be merged into the same commit?

[ebyhr]

Looks good except for one question.

does the commit about document also need to be merged into the same commit?

Yes in this case :)
When you modify unrelated docs in a PR, it should be separated commits.

[tangjiangling]

@hashhar @ebyhr Updated and thank you both again for your guidance :-)

[ebyhr]

Merged, thanks!