[#10381][#10378] fix(clickhouse-catalog): Fix ClickHouse alter-table bugs by yuqi1129 · Pull Request #10383 · apache/gravitino

yuqi1129 · 2026-03-11T12:52:45Z

What changes were proposed in this pull request?

Add comprehensive integration coverage for ClickHouse alter-table behavior in:

CatalogClickHouseIT
CatalogClickHouseClusterIT

The new tests exercise ClickHouseTableOperations#generateAlterTableSql branches in real ClickHouse environments, including:

supported alter branches: add/update type/update default/update comment/update position/update nullability/delete index/delete column
edge/no-op branches: delete missing column/index with ifExists=true
unsupported branches: set/remove property, update auto increment, nested column add

Tests also verify resulting table metadata (column type/comment/nullability/index removal) after alter operations.

Why are the changes needed?

To prevent regressions in ClickHouse ALTER TABLE SQL generation and close branch-level test gaps for issues discussed in #10381 and #10378.

Fixed: #10381
Fixed: #10372
Fixed: #10396
Fixed: #10405

Does this PR introduce any user-facing change?

N/A

Closes #10381
Closes #10378

…0381) Add integration tests in CatalogClickHouseIT and CatalogClickHouseClusterIT to cover generateAlterTableSql branches, including supported alter operations and unsupported/edge branches for property, index, and column changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Adds additional ClickHouse JDBC integration tests to exercise more ALTER TABLE branches (including index deletion, missing column/index handling, and unsupported change types) in both single-node and cluster ClickHouse environments, helping prevent regressions discussed in #10381 and #10378.

Changes:

Add a new single-node integration test covering alter-table edge/unsupported branches and index deletion.
Add a new cluster integration test covering alter-table branches including add column, position/type/comment/default/nullability updates, and index deletion.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseIT.java	Adds a new integration test to cover additional alter-table branches (incl. missing column/index behavior and unsupported operations).
catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseClusterIT.java	Adds a new cluster integration test to exercise alter-table branches on clustered ClickHouse tables.

Add integration cases for unsupported addIndex branch, both updateColumnAutoIncrement(true/false) unsupported branches, and multi-index drop validation for minmax and bloom filter indexes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Implement AddIndex handling in ClickHouseTableOperations#generateAlterTableSql for DATA_SKIPPING_MINMAX and DATA_SKIPPING_BLOOM_FILTER, with validation for duplicate/invalid index definitions and explicit unsupported behavior for PRIMARY_KEY alter-add. Update unit and integration tests to validate real behavior for add/drop index and autoIncrement branches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

yuqi1129 · 2026-03-11T13:16:44Z

Verified against real ClickHouse integration tests: ADD INDEX and DROP INDEX for data skipping indexes are supported and now implemented in generateAlterTableSql for DATA_SKIPPING_MINMAX and DATA_SKIPPING_BLOOM_FILTER. ADD INDEX PRIMARY_KEY via ALTER is not supported and stays explicit unsupported behavior. updateColumnAutoIncrement true/false is unsupported in tested ClickHouse 24.8 and tests now assert this behavior. Also updated unit and integration coverage to match runtime behavior.

github-actions · 2026-03-11T14:23:57Z

Code Coverage Report

Overall Project	64.94% `-0.03%`	🟢
Files changed	62.88%	🟢

Module	Coverage
aliyun	1.73%	🔴
api	47.14%	🟢
authorization-common	85.96%	🟢
aws	1.1%	🔴
azure	2.6%	🔴
catalog-common	10.0%	🔴
catalog-fileset	80.02%	🟢
catalog-hive	80.98%	🟢
catalog-jdbc-clickhouse	79.06% `-1.4%`	🟢
catalog-jdbc-common	42.89% `-10.37%`	🟢
catalog-jdbc-doris	80.28%	🟢
catalog-jdbc-hologres	54.03%	🟢
catalog-jdbc-mysql	79.23%	🟢
catalog-jdbc-oceanbase	78.38%	🟢
catalog-jdbc-postgresql	82.05%	🟢
catalog-jdbc-starrocks	78.27%	🟢
catalog-kafka	77.01%	🟢
catalog-lakehouse-generic	45.07%	🟢
catalog-lakehouse-hudi	79.1%	🟢
catalog-lakehouse-iceberg	87.15%	🟢
catalog-lakehouse-paimon	77.71%	🟢
catalog-model	77.72%	🟢
cli	44.51%	🟢
client-java	77.83%	🟢
common	49.42%	🟢
core	81.31%	🟢
filesystem-hadoop3	76.97%	🟢
flink	38.86%	🔴
flink-runtime	0.0%	🔴
gcp	14.2%	🔴
hadoop-common	10.39%	🔴
hive-metastore-common	45.82%	🟢
iceberg-common	50.21%	🟢
iceberg-rest-server	66.24%	🟢
integration-test-common	0.0%	🔴
jobs	66.17%	🟢
lance-common	23.78%	🔴
lance-rest-server	57.84%	🟢
lineage	53.02%	🟢
optimizer	82.95%	🟢
optimizer-api	21.95%	🔴
server	85.6%	🟢
server-common	69.72%	🟢
spark	32.79%	🔴
spark-common	39.09%	🔴
trino-connector	31.62%	🔴

Files

Module	File	Coverage
catalog-jdbc-clickhouse	ClickHouseClusterUtils.java	100.0%	🟢
	ClickHouseTableOperations.java	82.16%	🟢
	ClickHouseDatabaseOperations.java	51.43%	🔴
catalog-jdbc-common	JdbcCatalogOperations.java	9.05%	🔴

Add explicit assertions for column ordering after updateColumnPosition and for col_1 default value after updateColumnDefaultValue in CatalogClickHouseClusterIT. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Validate create-table columns in ClickHouse and fail fast when any column is marked autoIncrement. Add unit and integration tests to lock behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Handle multiline ORDER BY/PARTITION BY clauses when parsing SHOW CREATE TABLE, so loadTable keeps sort orders configured at create time. Add unit parsing test and cluster integration assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

You can also share your feedback on Copilot code review. Take the survey.

… into fix-10381-10378-clickhouse-alter-it

yuqi1129 · 2026-03-18T06:22:02Z

@diqiu50
Could you help review it again?

diqiu50 · 2026-03-18T08:02:45Z

-      JdbcConnectorUtils.executeUpdate(connection, generateDropDatabaseSql(databaseName, cascade));
+      if (!cascade) {
+        try (Statement stmt = connection.createStatement();
+            ResultSet rs = stmt.executeQuery(String.format("SHOW TABLES IN `%s`", databaseName))) {


You'd better use the system table to check table exists in db

What's the side effect of using SHOW TABLES IN?

May be the table is just not in this host, if you don't want handle this case, add some comments

Querying system table does not handle this case too. you'd better Add some comments

diqiu50 · 2026-03-19T09:28:31Z

-      JdbcConnectorUtils.executeUpdate(connection, generateDropDatabaseSql(databaseName, cascade));
+      if (!cascade) {
+        try (Statement stmt = connection.createStatement();
+            ResultSet rs = stmt.executeQuery(String.format("SHOW TABLES IN `%s`", databaseName))) {


May be the table is just not in this host, if you don't want handle this case, add some comments

diqiu50 · 2026-03-20T07:43:39Z

+ *   ENGINE = MergeTree()
+ *   ORDER BY `id`
+ *   COMMENT 'order records\n[Gravitino] ch.cluster=ck_cluster'
+ *


Do we need an end marker?

Mark "\n[Gravitino] ch.cluster=ck_cluster" will always at the end of the comment, what do you mean here?

if the user add some comments after \n[Gravitino] ch.cluster=ck_cluster, is it a problem?

Do you mean users will update the comment without using Gravitino and add something at the end of \n[Gravitino] ch.cluster=ck_cluster?

We can't avoid such a problem as long as users can update comments without notifying Gravitino.

So we usually add an end delimiter to avoid things getting mixed together.

diqiu50 · 2026-03-20T07:55:08Z

-      JdbcConnectorUtils.executeUpdate(connection, generateDropDatabaseSql(databaseName, cascade));
+      if (!cascade) {
+        try (Statement stmt = connection.createStatement();
+            ResultSet rs = stmt.executeQuery(String.format("SHOW TABLES IN `%s`", databaseName))) {


Querying system table does not handle this case too. you'd better Add some comments

diqiu50

LGTM

…bugs (#10383) ### What changes were proposed in this pull request? Add comprehensive integration coverage for ClickHouse alter-table behavior in: - `CatalogClickHouseIT` - `CatalogClickHouseClusterIT` The new tests exercise `ClickHouseTableOperations#generateAlterTableSql` branches in real ClickHouse environments, including: - supported alter branches: add/update type/update default/update comment/update position/update nullability/delete index/delete column - edge/no-op branches: delete missing column/index with `ifExists=true` - unsupported branches: set/remove property, update auto increment, nested column add Tests also verify resulting table metadata (column type/comment/nullability/index removal) after alter operations. ### Why are the changes needed? To prevent regressions in ClickHouse ALTER TABLE SQL generation and close branch-level test gaps for issues discussed in #10381 and #10378. Fixed: #10381 Fixed: #10372 Fixed: #10396 Fixed: #10405 ### Does this PR introduce _any_ user-facing change? N/A Closes #10381 Closes #10378 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

… Fix ClickHouse alter-table bugs (#10383) (#10487) **Cherry-pick Information:** - Original commit: 8afb307 - Target branch: `branch-1.2` - Status: ✅ Clean cherry-pick (no conflicts) Co-authored-by: Qi Yu <yuqi@datastrato.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…alter-table bugs (apache#10383) ### What changes were proposed in this pull request? Add comprehensive integration coverage for ClickHouse alter-table behavior in: - `CatalogClickHouseIT` - `CatalogClickHouseClusterIT` The new tests exercise `ClickHouseTableOperations#generateAlterTableSql` branches in real ClickHouse environments, including: - supported alter branches: add/update type/update default/update comment/update position/update nullability/delete index/delete column - edge/no-op branches: delete missing column/index with `ifExists=true` - unsupported branches: set/remove property, update auto increment, nested column add Tests also verify resulting table metadata (column type/comment/nullability/index removal) after alter operations. ### Why are the changes needed? To prevent regressions in ClickHouse ALTER TABLE SQL generation and close branch-level test gaps for issues discussed in apache#10381 and apache#10378. Fixed: apache#10381 Fixed: apache#10372 Fixed: apache#10396 Fixed: apache#10405 ### Does this PR introduce _any_ user-facing change? N/A Closes apache#10381 Closes apache#10378 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings March 11, 2026 12:52

Copilot started reviewing on behalf of yuqi1129 March 11, 2026 12:53 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

Comment thread ...ava/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseClusterIT.java

Comment thread ...ava/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseClusterIT.java

yuqi1129 and others added 2 commits March 11, 2026 21:04

yuqi1129 changed the title ~~[Improvement] Add ClickHouse alter-table integration branch coverage~~ [#10381][#10378] fix(clickhouse-catalog): Fix ClickHouse alter-table bugs Mar 11, 2026

yuqi1129 and others added 3 commits March 12, 2026 15:23

fix(clickhouse): reject auto increment on create table

0a2e378

Validate create-table columns in ClickHouse and fail fast when any column is marked autoIncrement. Add unit and integration tests to lock behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

yuqi1129 self-assigned this Mar 12, 2026

yuqi1129 requested a review from diqiu50 March 12, 2026 09:26

fix

47ba6e1

jerryshao added the branch-1.2 Automatically cherry-pick commit to branch-1.2 label Mar 12, 2026

diqiu50 reviewed Mar 16, 2026

View reviewed changes

Comment thread .../main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java Outdated

Comment thread .../main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java Outdated

Comment thread docs/jdbc-clickhouse-catalog.md Outdated

fix

22714cc

yuqi1129 requested review from Copilot and diqiu50 March 16, 2026 07:45

Copilot AI reviewed Mar 16, 2026

View reviewed changes

Copilot started reviewing on behalf of yuqi1129 March 16, 2026 07:52 View session

yuqi1129 added 3 commits March 16, 2026 20:32

fix

853ae36

fix

9f52578

Merge remote-tracking branch 'me/fix-10381-10378-clickhouse-alter-it'…

bb10223

… into fix-10381-10378-clickhouse-alter-it

yuqi1129 commented Mar 17, 2026

View reviewed changes

Comment thread .../main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java

Add more ITs

5a29631

fix

ae1aff2

diqiu50 reviewed Mar 18, 2026

View reviewed changes

yuqi1129 added 4 commits March 18, 2026 21:22

Fix

c347c0c

Fix

848a4cd

Fix

c6cad51

Fix

f08b50c

pythaac mentioned this pull request Mar 18, 2026

[Improvement] Customize ClickHouse index properties like GRANULARITY #10446

Open

yuqi1129 requested a review from diqiu50 March 19, 2026 08:17

diqiu50 reviewed Mar 19, 2026

View reviewed changes

yuqi1129 added 2 commits March 19, 2026 19:56

Fix

2c9a630

Optimize constants

d537147

yuqi1129 requested a review from diqiu50 March 20, 2026 03:56

diqiu50 reviewed Mar 20, 2026

View reviewed changes

fix

ffc7ff0

diqiu50 approved these changes Mar 20, 2026

View reviewed changes

yuqi1129 merged commit 8afb307 into apache:main Mar 20, 2026
26 checks passed

yuqi1129 deleted the fix-10381-10378-clickhouse-alter-it branch March 20, 2026 10:20

Conversation

yuqi1129 commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

yuqi1129 commented Mar 11, 2026

Uh oh!

github-actions bot commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yuqi1129 commented Mar 18, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diqiu50 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

yuqi1129 commented Mar 11, 2026 •

edited

Loading

github-actions bot commented Mar 11, 2026 •

edited

Loading