This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 848c067af0 [#12768] feat(clickhouse): support altering table settings
(#12772)
848c067af0 is described below
commit 848c067af0d51d772850e0f1ff630d7641f6d68f
Author: StormSpirit <[email protected]>
AuthorDate: Fri Sep 18 23:10:04 2026 +0800
[#12768] feat(clickhouse): support altering table settings (#12772)
### What changes were proposed in this pull request?
This change adds ALTER support for ClickHouse table-level settings
exposed through the existing `settings.*` table-property namespace.
- Map settings-only `TableChange.SetProperty` requests to `MODIFY
SETTING` and settings-only `TableChange.RemoveProperty` requests to
`RESET SETTING`.
- Validate setting requests before opening a JDBC connection, reject
duplicate or unsupported command-family mixtures, and serialize settings
in deterministic key order.
- Validate setting identifiers and scalar literals while leaving
concrete setting mutability, range, and permission checks to ClickHouse.
- Preserve trusted `ON CLUSTER` behavior and avoid logging setting
values.
- Document the supported property contract and its scope boundaries.
### Why are the changes needed?
The ClickHouse catalog already loads table settings into `settings.*`
properties and writes them during CREATE TABLE, but ALTER rejects
attempts to set or remove those properties. Supporting MODIFY and RESET
completes the table-settings lifecycle without requiring users to bypass
Gravitino or recreate tables.
Fix: #12768
### Does this PR introduce _any_ user-facing change?
Yes. Users can modify or reset table-level ClickHouse `settings.*`
properties through `alterTable`. Each request must contain only set
operations or only remove operations; settings cannot be mixed with
schema, comment, or index changes. Numeric and boolean values are
unquoted ClickHouse scalar literals, while string values must be valid
single-quoted literals. Non-`settings.*` table properties remain
immutable.
Column-level SETTINGS and quoted-comma SETTINGS load/recreate parsing
are out of scope; this PR also makes no new ReplicatedMergeTree
compatibility claim.
### How was this patch tested?
- `./gradlew :catalogs-contrib:catalog-jdbc-clickhouse:test -PskipITs` —
passed; 92 tests, 0 skipped, 0 failures, 0 errors.
- `CatalogClickHouseIT.testAlterTableSettings` — passed against
ClickHouse 24.8.14.39, covering numeric and string modify/load/reset
plus `READONLY_SETTING` and `UNKNOWN_SETTING` propagation.
- `CatalogClickHouseIT.testAlterTableSettingReadOnlyConnectionError` —
passed against ClickHouse 24.8.14.39 with ClickHouse JDBC 0.7.1
`custom_settings=readonly%3D1`.
- `CatalogClickHouseClusterIT.testAlterTableSettingsOnCluster` — passed
against the project cluster fixture; `system.query_log` verified that
both MODIFY and RESET SQL contain `ON CLUSTER` (`tests=1`, `skipped=0`,
`failures=0`, `errors=0`).
- `./gradlew :catalogs-contrib:catalog-jdbc-clickhouse:spotlessCheck` —
passed.
- `./gradlew rat` — passed.
- `./gradlew :catalogs-contrib:catalog-jdbc-clickhouse:build -x test` —
passed.
- A local-only supplemental Gravitino precheck completed without errors.
---------
Signed-off-by: jiangxt2 <[email protected]>
---
.../operations/ClickHouseTableOperations.java | 172 ++++++++++++++--
.../test/CatalogClickHouseClusterIT.java | 62 ++++++
.../integration/test/CatalogClickHouseIT.java | 124 ++++++++++++
.../operations/TestClickHouseTableOperations.java | 2 +-
.../TestClickHouseTableOperationsUnit.java | 221 +++++++++++++++++++++
docs/jdbc-clickhouse-catalog.md | 38 ++--
6 files changed, 589 insertions(+), 30 deletions(-)
diff --git
a/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java
b/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java
index b6f566a4e0..148172cefe 100644
---
a/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java
+++
b/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java
@@ -118,6 +118,11 @@ public class ClickHouseTableOperations extends
JdbcTableOperations {
/** Matches ClickHouse wide integer type names (Int128/256, UInt128/256, and
future variants). */
private static final Pattern WIDE_INTEGER_PATTERN =
Pattern.compile("^U?INT\\d+$");
+ private static final Pattern TABLE_SETTING_NAME_PATTERN =
+ Pattern.compile("^[A-Za-z_][A-Za-z0-9_]*$");
+ private static final Pattern NUMERIC_SETTING_LITERAL_PATTERN =
+ Pattern.compile("^[+-]?(?:\\d+(?:\\.\\d*)?|\\.\\d+)(?:[eE][+-]?\\d+)?$");
+
private static final String QUERY_INDEXES_SQL =
"""
SELECT NULL AS TABLE_CAT,
@@ -695,6 +700,7 @@ public class ClickHouseTableOperations extends
JdbcTableOperations {
@Override
public void alterTable(String databaseName, String tableName, TableChange...
changes)
throws NoSuchTableException {
+ validateTableSettingChanges(changes);
LOG.info("Attempting to alter table {} from database {}", tableName,
databaseName);
try (Connection connection = getConnection(databaseName)) {
String sql = generateAlterTableSql(databaseName, tableName, changes);
@@ -1035,20 +1041,21 @@ public class ClickHouseTableOperations extends
JdbcTableOperations {
JdbcTable lazyLoadTable = null;
TableChange.UpdateComment updateComment = null;
List<TableChange.SetProperty> setProperties = new ArrayList<>();
+ List<TableChange.RemoveProperty> removeProperties = new ArrayList<>();
List<String> alterSql = new ArrayList<>();
+ collectAndValidateTableSettingChanges(changes, setProperties,
removeProperties);
+
for (TableChange change : changes) {
if (change instanceof TableChange.UpdateComment) {
updateComment = (TableChange.UpdateComment) change;
- } else if (change instanceof TableChange.SetProperty setProperty) {
- // The set attribute needs to be added at the end.
- setProperties.add(setProperty);
-
- } else if (change instanceof TableChange.RemoveProperty) {
- // Clickhouse does not support deleting table attributes, it can be
replaced by Set Property
- throw new UnsupportedOperationException(
- "Remove property for ClickHouse is not supported yet");
+ } else if (change instanceof TableChange.SetProperty
+ || change instanceof TableChange.RemoveProperty) {
+ // Table setting changes were fully validated before any table
metadata was loaded. They are
+ // rendered together after the other change branches, which are
mutually exclusive with
+ // settings changes.
+ continue;
} else if (change instanceof TableChange.AddColumn addColumn) {
lazyLoadTable = getOrCreateTable(databaseName, tableName,
lazyLoadTable);
@@ -1130,8 +1137,8 @@ public class ClickHouseTableOperations extends
JdbcTableOperations {
alterSql.add(" MODIFY COMMENT
'%s'".formatted(escapeSingleQuotes(newComment)));
}
- if (!setProperties.isEmpty()) {
- alterSql.add(generateAlterTableProperties(setProperties));
+ if (!setProperties.isEmpty() || !removeProperties.isEmpty()) {
+ alterSql.add(generateAlterTableProperties(setProperties,
removeProperties));
}
// Remove all empty SQL statements
@@ -1163,7 +1170,15 @@ public class ClickHouseTableOperations extends
JdbcTableOperations {
"ALTER TABLE %s \n%s;"
.formatted(quoteIdentifier(tableName), String.join(",\n",
nonEmptySQLs));
}
- LOG.info("Generated alter table:{} sql: {}", databaseName + "." +
tableName, result);
+ if (!setProperties.isEmpty() || !removeProperties.isEmpty()) {
+ LOG.info(
+ "Generated alter table settings for {}.{} with keys {}",
+ databaseName,
+ tableName,
+ tableSettingNames(setProperties, removeProperties));
+ } else {
+ LOG.info("Generated alter table:{} sql: {}", databaseName + "." +
tableName, result);
+ }
return result;
}
@@ -1282,15 +1297,144 @@ public class ClickHouseTableOperations extends
JdbcTableOperations {
appendColumnDefinition(updateColumn, new StringBuilder()));
}
- private String generateAlterTableProperties(List<TableChange.SetProperty>
setProperties) {
- if (CollectionUtils.isNotEmpty(setProperties)) {
+ private static void collectAndValidateTableSettingChanges(
+ TableChange[] changes,
+ List<TableChange.SetProperty> setProperties,
+ List<TableChange.RemoveProperty> removeProperties) {
+ for (TableChange change : changes) {
+ if (change instanceof TableChange.SetProperty setProperty) {
+ tableSettingName(setProperty.getProperty());
+ validateTableSettingLiteral(setProperty.getValue());
+ setProperties.add(setProperty);
+ } else if (change instanceof TableChange.RemoveProperty removeProperty) {
+ tableSettingName(removeProperty.getProperty());
+ removeProperties.add(removeProperty);
+ }
+ }
+
+ int settingChangeCount = setProperties.size() + removeProperties.size();
+ if (settingChangeCount == 0) {
+ return;
+ }
+
+ if (settingChangeCount != changes.length) {
+ throw new UnsupportedOperationException(
+ "ClickHouse table setting changes cannot be mixed with other table
changes");
+ }
+ if (!setProperties.isEmpty() && !removeProperties.isEmpty()) {
+ throw new UnsupportedOperationException(
+ "ClickHouse MODIFY SETTING and RESET SETTING cannot be combined in
one request");
+ }
+
+ validateNoDuplicateTableSettings(
+ setProperties.stream()
+ .map(change -> tableSettingName(change.getProperty()))
+ .collect(Collectors.toList()));
+ validateNoDuplicateTableSettings(
+ removeProperties.stream()
+ .map(change -> tableSettingName(change.getProperty()))
+ .collect(Collectors.toList()));
+
+ setProperties.sort(Comparator.comparing(change ->
tableSettingName(change.getProperty())));
+ removeProperties.sort(Comparator.comparing(change ->
tableSettingName(change.getProperty())));
+ }
+
+ private static void validateTableSettingChanges(TableChange[] changes) {
+ collectAndValidateTableSettingChanges(changes, new ArrayList<>(), new
ArrayList<>());
+ }
+
+ private static void validateNoDuplicateTableSettings(List<String>
settingNames) {
+ Set<String> uniqueNames = new HashSet<>();
+ for (String settingName : settingNames) {
+ Preconditions.checkArgument(
+ uniqueNames.add(settingName), "Duplicate ClickHouse table setting:
%s", settingName);
+ }
+ }
+
+ private static String tableSettingName(String property) {
+ Preconditions.checkArgument(
+ StringUtils.isNotBlank(property), "ClickHouse table setting property
is required");
+ if (!property.startsWith(TableConstants.SETTINGS_PREFIX)) {
throw new UnsupportedOperationException(
- "Alter table properties in ClickHouse is not supported");
+ "Only ClickHouse table properties with the 'settings.' prefix can be
altered");
+ }
+
+ String settingName =
property.substring(TableConstants.SETTINGS_PREFIX.length());
+ Preconditions.checkArgument(
+ TABLE_SETTING_NAME_PATTERN.matcher(settingName).matches(),
+ "Invalid ClickHouse table setting name: %s",
+ settingName);
+ return settingName;
+ }
+
+ private static void validateTableSettingLiteral(String value) {
+ Preconditions.checkArgument(
+ StringUtils.isNotBlank(value), "ClickHouse table setting value is
required");
+ String literal = value.trim();
+ boolean valid =
+ NUMERIC_SETTING_LITERAL_PATTERN.matcher(literal).matches()
+ || "true".equalsIgnoreCase(literal)
+ || "false".equalsIgnoreCase(literal)
+ || isValidQuotedSettingLiteral(literal);
+ Preconditions.checkArgument(valid, "Invalid ClickHouse table setting
literal");
+ }
+
+ private static boolean isValidQuotedSettingLiteral(String literal) {
+ if (literal.length() < 2
+ || literal.charAt(0) != '\''
+ || literal.charAt(literal.length() - 1) != '\'') {
+ return false;
}
+ for (int i = 1; i < literal.length() - 1; i++) {
+ char current = literal.charAt(i);
+ if (current == '\\') {
+ if (i + 1 >= literal.length() - 1) {
+ return false;
+ }
+ i++;
+ } else if (current == '\'') {
+ if (i + 1 >= literal.length() - 1 || literal.charAt(i + 1) != '\'') {
+ return false;
+ }
+ i++;
+ }
+ }
+ return true;
+ }
+
+ private static String generateAlterTableProperties(
+ List<TableChange.SetProperty> setProperties,
+ List<TableChange.RemoveProperty> removeProperties) {
+ if (!setProperties.isEmpty()) {
+ return setProperties.stream()
+ .map(
+ change ->
+ "%s = %s"
+ .formatted(tableSettingName(change.getProperty()),
change.getValue().trim()))
+ .collect(Collectors.joining(", ", "MODIFY SETTING ", ""));
+ }
+ if (!removeProperties.isEmpty()) {
+ return removeProperties.stream()
+ .map(change -> tableSettingName(change.getProperty()))
+ .collect(Collectors.joining(", ", "RESET SETTING ", ""));
+ }
return "";
}
+ private static List<String> tableSettingNames(
+ List<TableChange.SetProperty> setProperties,
+ List<TableChange.RemoveProperty> removeProperties) {
+ if (!setProperties.isEmpty()) {
+ return setProperties.stream()
+ .map(change -> tableSettingName(change.getProperty()))
+ .collect(Collectors.toList());
+ }
+ return removeProperties.stream()
+ .map(change -> tableSettingName(change.getProperty()))
+ .collect(Collectors.toList());
+ }
+
private String updateColumnCommentFieldDefinition(
TableChange.UpdateColumnComment updateColumnComment, JdbcTable
jdbcTable) {
String newComment = updateColumnComment.getNewComment();
diff --git
a/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseClusterIT.java
b/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseClusterIT.java
index bd6b34c0ab..0cee91a224 100644
---
a/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseClusterIT.java
+++
b/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseClusterIT.java
@@ -48,6 +48,7 @@ import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.Namespace;
import org.apache.gravitino.Schema;
import org.apache.gravitino.StringIdentifier;
+import
org.apache.gravitino.catalog.clickhouse.ClickHouseConstants.TableConstants;
import
org.apache.gravitino.catalog.clickhouse.integration.test.service.ClickHouseService;
import
org.apache.gravitino.catalog.clickhouse.operations.ClickHouseClusterUtils;
import org.apache.gravitino.catalog.jdbc.config.JdbcConfig;
@@ -1005,6 +1006,48 @@ public class CatalogClickHouseClusterIT extends BaseIT {
}
}
+ @Test
+ public void testAlterTableSettingsOnCluster() throws Exception {
+ String tableName =
GravitinoITUtils.genRandomName("ck_alter_settings_cluster");
+ NameIdentifier tableIdentifier = NameIdentifier.of(schemaName, tableName);
+ TableCatalog tableCatalog = catalog.asTableCatalog();
+ tableCatalog.createTable(
+ tableIdentifier,
+ createColumns(),
+ tableComment,
+ clusterMergeTreeProperties(),
+ Transforms.EMPTY_TRANSFORM,
+ Distributions.NONE,
+ getSortOrders("col_3"),
+ Indexes.EMPTY_INDEXES);
+
+ tableCatalog.alterTable(
+ tableIdentifier,
+ TableChange.setProperty(TableConstants.SETTINGS_PREFIX +
"merge_with_ttl_timeout", "3600"));
+ Table modified = tableCatalog.loadTable(tableIdentifier);
+ Assertions.assertEquals(
+ "3600",
+ modified.properties().get(TableConstants.SETTINGS_PREFIX +
"merge_with_ttl_timeout"));
+
+ tableCatalog.alterTable(
+ tableIdentifier,
+ TableChange.removeProperty(TableConstants.SETTINGS_PREFIX +
"merge_with_ttl_timeout"));
+ Table reset = tableCatalog.loadTable(tableIdentifier);
+ Assertions.assertFalse(
+ reset.properties().containsKey(TableConstants.SETTINGS_PREFIX +
"merge_with_ttl_timeout"));
+
+ try (Connection connection =
+ DriverManager.getConnection(
+ clickHouseClusterContainer.getJdbcUrl(TEST_DB_NAME),
+ clickHouseClusterContainer.getUsername(),
+ clickHouseClusterContainer.getPassword());
+ Statement statement = connection.createStatement()) {
+ statement.execute("SYSTEM FLUSH LOGS");
+ assertSettingAlterUsesOnCluster(statement, tableName, "MODIFY SETTING");
+ assertSettingAlterUsesOnCluster(statement, tableName, "RESET SETTING");
+ }
+ }
+
//
---------------------------------------------------------------------------
// Shard key validation IT tests
//
---------------------------------------------------------------------------
@@ -1257,6 +1300,25 @@ public class CatalogClickHouseClusterIT extends BaseIT {
}
}
+ private static void assertSettingAlterUsesOnCluster(
+ Statement statement, String tableName, String command) throws
SQLException {
+ try (ResultSet resultSet =
+ statement.executeQuery(
+ String.format(
+ "SELECT query FROM system.query_log "
+ + "WHERE type = 'QueryFinish' "
+ + "AND query_kind = 'Alter' "
+ + "AND query LIKE '%%`%s`%%' "
+ + "AND query LIKE '%%%s%%' "
+ + "ORDER BY event_time DESC LIMIT 1",
+ tableName, command))) {
+ Assertions.assertTrue(resultSet.next(), "Should find " + command + "
query");
+ String sql = resultSet.getString("query");
+ Assertions.assertTrue(
+ sql.contains("ON CLUSTER"), command + " must include ON CLUSTER,
actual: " + sql);
+ }
+ }
+
private void awaitTableStateOnEveryNode(
String oldTableName, boolean oldTableExists, String newTableName,
boolean newTableExists) {
Awaitility.await()
diff --git
a/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseIT.java
b/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseIT.java
index a6619bd95d..855fcd51ef 100644
---
a/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseIT.java
+++
b/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/integration/test/CatalogClickHouseIT.java
@@ -2882,6 +2882,130 @@ public class CatalogClickHouseIT extends BaseIT {
metalake.dropCatalog(testCatalogName, true);
}
+ @Test
+ void testAlterTableSettings() {
+ String name = GravitinoITUtils.genRandomName("alter_settings");
+ NameIdentifier ident = NameIdentifier.of(schemaName, name);
+ Column[] columns =
+ new Column[] {
+ Column.of("id", Types.IntegerType.get(), "id", false, false,
DEFAULT_VALUE_NOT_SET)
+ };
+ TableCatalog tableCatalog = catalog.asTableCatalog();
+ tableCatalog.createTable(
+ ident,
+ columns,
+ "alter settings",
+ createProperties(),
+ Distributions.NONE,
+ getSortOrders("id"));
+
+ tableCatalog.alterTable(
+ ident,
+ TableChange.setProperty(TableConstants.SETTINGS_PREFIX +
"merge_with_ttl_timeout", "3600"));
+ Table modified = tableCatalog.loadTable(ident);
+ Assertions.assertEquals(
+ "3600",
+ modified.properties().get(TableConstants.SETTINGS_PREFIX +
"merge_with_ttl_timeout"));
+
+ tableCatalog.alterTable(
+ ident,
+ TableChange.removeProperty(TableConstants.SETTINGS_PREFIX +
"merge_with_ttl_timeout"));
+ Table reset = tableCatalog.loadTable(ident);
+ Assertions.assertFalse(
+ reset.properties().containsKey(TableConstants.SETTINGS_PREFIX +
"merge_with_ttl_timeout"));
+
+ tableCatalog.alterTable(
+ ident,
+ TableChange.setProperty(TableConstants.SETTINGS_PREFIX +
"storage_policy", "'default'"));
+ Table stringModified = tableCatalog.loadTable(ident);
+ Assertions.assertEquals(
+ "'default'",
+ stringModified.properties().get(TableConstants.SETTINGS_PREFIX +
"storage_policy"));
+ tableCatalog.alterTable(
+ ident, TableChange.removeProperty(TableConstants.SETTINGS_PREFIX +
"storage_policy"));
+ Table stringReset = tableCatalog.loadTable(ident);
+ Assertions.assertFalse(
+ stringReset.properties().containsKey(TableConstants.SETTINGS_PREFIX +
"storage_policy"));
+
+ RuntimeException readOnlyException =
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () ->
+ tableCatalog.alterTable(
+ ident,
+ TableChange.setProperty(
+ TableConstants.SETTINGS_PREFIX + "index_granularity",
"4096")));
+ Assertions.assertTrue(
+ readOnlyException.getMessage().contains("READONLY_SETTING"),
+ readOnlyException.getMessage());
+
+ RuntimeException unknownSettingException =
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () ->
+ tableCatalog.alterTable(
+ ident,
+ TableChange.setProperty(
+ TableConstants.SETTINGS_PREFIX +
"gravitino_unknown_setting", "1")));
+ Assertions.assertTrue(
+ unknownSettingException.getMessage().contains("UNKNOWN_SETTING"),
+ unknownSettingException.getMessage());
+ }
+
+ @Test
+ void testAlterTableSettingReadOnlyConnectionError() throws SQLException {
+ String tableName =
GravitinoITUtils.genRandomName("alter_settings_readonly");
+ NameIdentifier tableIdentifier = NameIdentifier.of(schemaName, tableName);
+ TableCatalog tableCatalog = catalog.asTableCatalog();
+ tableCatalog.createTable(
+ tableIdentifier,
+ new Column[] {
+ Column.of("id", Types.IntegerType.get(), "id", false, false,
DEFAULT_VALUE_NOT_SET)
+ },
+ "alter settings privilege",
+ createProperties(),
+ Distributions.NONE,
+ getSortOrders("id"));
+
+ String restrictedCatalogName =
+ GravitinoITUtils.genRandomName("alter_settings_restricted_catalog");
+ Map<String, String> catalogProperties = Maps.newHashMap();
+ String jdbcUrl =
+ StringUtils.substring(
+ CLICKHOUSE_CONTAINER.getJdbcUrl(TEST_DB_NAME),
+ 0,
+ CLICKHOUSE_CONTAINER.getJdbcUrl(TEST_DB_NAME).lastIndexOf("/"));
+ catalogProperties.put(JdbcConfig.JDBC_URL.getKey(), jdbcUrl +
"?custom_settings=readonly%3D1");
+ catalogProperties.put(
+ JdbcConfig.JDBC_DRIVER.getKey(),
CLICKHOUSE_CONTAINER.getDriverClassName(TEST_DB_NAME));
+ catalogProperties.put(JdbcConfig.USERNAME.getKey(),
CLICKHOUSE_CONTAINER.getUsername());
+ catalogProperties.put(JdbcConfig.PASSWORD.getKey(),
CLICKHOUSE_CONTAINER.getPassword());
+
+ Catalog restrictedCatalog =
+ metalake.createCatalog(
+ restrictedCatalogName,
+ Catalog.Type.RELATIONAL,
+ provider,
+ "read-only alter settings catalog",
+ catalogProperties);
+ try {
+ RuntimeException readOnlyException =
+ Assertions.assertThrows(
+ RuntimeException.class,
+ () ->
+ restrictedCatalog
+ .asTableCatalog()
+ .alterTable(
+ tableIdentifier,
+ TableChange.setProperty(
+ TableConstants.SETTINGS_PREFIX +
"merge_with_ttl_timeout", "3600")));
+ Assertions.assertTrue(
+ readOnlyException.getMessage().contains("READONLY"),
readOnlyException.getMessage());
+ } finally {
+ metalake.dropCatalog(restrictedCatalogName, true);
+ }
+ }
+
@Test
void testLoadTableWithSettingsFromNativeSql() {
String name = GravitinoITUtils.genRandomName("settings_native");
diff --git
a/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperations.java
b/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperations.java
index c1b00b36b9..088709c1cd 100644
---
a/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperations.java
+++
b/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperations.java
@@ -174,7 +174,7 @@ public class TestClickHouseTableOperations extends
TestClickHouse {
Assertions.assertTrue(
StringUtils.contains(
gravitinoRuntimeException.getMessage(),
- "Alter table properties in ClickHouse is not supported"));
+ "Only ClickHouse table properties with the 'settings.' prefix can
be altered"));
// delete column
TABLE_OPERATIONS.alterTable(
diff --git
a/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperationsUnit.java
b/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperationsUnit.java
index 86ee8538fd..736f327c68 100644
---
a/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperationsUnit.java
+++
b/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperationsUnit.java
@@ -29,14 +29,17 @@ import java.util.HashMap;
import java.util.List;
import java.util.Map;
import javax.sql.DataSource;
+import
org.apache.gravitino.catalog.clickhouse.ClickHouseConstants.ClusterConstants;
import
org.apache.gravitino.catalog.clickhouse.ClickHouseConstants.TableConstants;
import
org.apache.gravitino.catalog.clickhouse.ClickHouseTablePropertiesMetadata.ENGINE;
import
org.apache.gravitino.catalog.clickhouse.converter.ClickHouseColumnDefaultValueConverter;
import
org.apache.gravitino.catalog.clickhouse.converter.ClickHouseExceptionConverter;
import
org.apache.gravitino.catalog.clickhouse.converter.ClickHouseTypeConverter;
import org.apache.gravitino.catalog.jdbc.JdbcColumn;
+import org.apache.gravitino.catalog.jdbc.JdbcTable;
import org.apache.gravitino.exceptions.GravitinoRuntimeException;
import org.apache.gravitino.exceptions.NoSuchTableException;
+import org.apache.gravitino.rel.TableChange;
import org.apache.gravitino.rel.expressions.FunctionExpression;
import org.apache.gravitino.rel.expressions.NamedReference;
import org.apache.gravitino.rel.expressions.distributions.Distributions;
@@ -53,6 +56,8 @@ import org.mockito.Mockito;
public class TestClickHouseTableOperationsUnit {
private static final class ExposedClickHouseTableOperations extends
ClickHouseTableOperations {
+ private JdbcTable table;
+
List<Index> callGetIndexes(Connection connection, String databaseName,
String tableName)
throws Exception {
return getIndexes(connection, databaseName, tableName);
@@ -87,6 +92,20 @@ public class TestClickHouseTableOperationsUnit {
Indexes.EMPTY_INDEXES,
getSortOrders("id"));
}
+
+ void setTable(JdbcTable table) {
+ this.table = table;
+ }
+
+ @Override
+ protected JdbcTable getOrCreateTable(
+ String databaseName, String tableName, JdbcTable lazyLoadCreateTable) {
+ return table;
+ }
+
+ String callGenerateAlterTableSql(TableChange... changes) {
+ return generateAlterTableSql("db", "test_table", changes);
+ }
}
private ExposedClickHouseTableOperations newOps() {
@@ -104,6 +123,29 @@ public class TestClickHouseTableOperationsUnit {
return ops;
}
+ private ExposedClickHouseTableOperations newAlterOps(Map<String, String>
properties) {
+ ExposedClickHouseTableOperations ops = newOps();
+ JdbcColumn idColumn =
+ JdbcColumn.builder()
+ .withName("id")
+ .withType(Types.IntegerType.get())
+ .withNullable(false)
+ .build();
+ ops.setTable(
+ JdbcTable.builder()
+ .withName("test_table")
+ .withColumns(new JdbcColumn[] {idColumn})
+ .withIndexes(Indexes.EMPTY_INDEXES)
+ .withProperties(properties)
+ .withTableOperation(null)
+ .build());
+ return ops;
+ }
+
+ private static String settingProperty(String name) {
+ return TableConstants.SETTINGS_PREFIX + name;
+ }
+
private Map<String, String> loadTableProperties(String engine, String
engineFull)
throws Exception {
PreparedStatement statement = Mockito.mock(PreparedStatement.class);
@@ -848,6 +890,185 @@ public class TestClickHouseTableOperationsUnit {
Mockito.verify(connection,
Mockito.times(2)).prepareStatement(Mockito.anyString());
}
+ @Test
+ void testGenerateModifyAndResetTableSettingsSql() {
+ ExposedClickHouseTableOperations ops = newAlterOps(Map.of());
+
+ String modifySql =
+ ops.callGenerateAlterTableSql(
+ TableChange.setProperty(settingProperty("z_setting"), "2"),
+ TableChange.setProperty(settingProperty("a_setting"), "1"));
+ Assertions.assertTrue(
+ modifySql.contains("MODIFY SETTING a_setting = 1, z_setting = 2"),
modifySql);
+
+ String resetSql =
+ ops.callGenerateAlterTableSql(
+ TableChange.removeProperty(settingProperty("z_setting")),
+ TableChange.removeProperty(settingProperty("a_setting")));
+ Assertions.assertTrue(resetSql.contains("RESET SETTING a_setting,
z_setting"), resetSql);
+ }
+
+ @Test
+ void testGenerateTableSettingsSqlOnCluster() {
+ ExposedClickHouseTableOperations ops =
+ newAlterOps(
+ Map.of(
+ ClusterConstants.ON_CLUSTER,
+ "true",
+ ClusterConstants.CLUSTER_NAME,
+ "test_cluster"));
+
+ String sql =
+ ops.callGenerateAlterTableSql(
+ TableChange.setProperty(settingProperty("merge_with_ttl_timeout"),
"3600"));
+
+ Assertions.assertTrue(
+ sql.startsWith("ALTER TABLE `test_table` ON CLUSTER `test_cluster`"),
sql);
+ Assertions.assertTrue(sql.contains("MODIFY SETTING merge_with_ttl_timeout
= 3600"), sql);
+ }
+
+ @Test
+ void testAcceptValidTableSettingLiterals() {
+ ExposedClickHouseTableOperations ops = newAlterOps(Map.of());
+ String[] validLiterals = {
+ "0", "-1", "+1.5", ".25", "1e3", "true", "FALSE", "'default'",
"'a,b\\\\c''d'"
+ };
+
+ for (String literal : validLiterals) {
+ String sql =
+ ops.callGenerateAlterTableSql(
+ TableChange.setProperty(settingProperty("test_setting"),
literal));
+ Assertions.assertTrue(sql.contains("test_setting = " + literal), sql);
+ }
+ }
+
+ @Test
+ void testRejectInvalidTableSettingNamesAndLiterals() {
+ ExposedClickHouseTableOperations ops = newOps();
+ String[] invalidNames = {
+ null,
+ settingProperty(""),
+ settingProperty("1setting"),
+ settingProperty("bad-setting"),
+ settingProperty("bad setting"),
+ settingProperty("setting;DROP")
+ };
+ for (String property : invalidNames) {
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
ops.callGenerateAlterTableSql(TableChange.setProperty(property, "1")));
+ }
+
+ String[] invalidLiterals = {
+ "",
+ "value",
+ "'unterminated",
+ "'bad\\'",
+ "1, RESET SETTING other",
+ "1; DROP TABLE t",
+ "'ok' OR 1"
+ };
+ for (String literal : invalidLiterals) {
+ IllegalArgumentException exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ ops.callGenerateAlterTableSql(
+ TableChange.setProperty(settingProperty("test_setting"),
literal)));
+ if (!literal.isEmpty()) {
+ Assertions.assertFalse(exception.getMessage().contains(literal));
+ }
+ }
+
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ ops.callGenerateAlterTableSql(
+ TableChange.setProperty(settingProperty("test_setting"),
null)));
+ }
+
+ @Test
+ void testRejectUnsupportedAndMixedTablePropertyChanges() {
+ ExposedClickHouseTableOperations ops = newOps();
+
+ Assertions.assertThrows(
+ UnsupportedOperationException.class,
+ () -> ops.callGenerateAlterTableSql(TableChange.setProperty("engine",
"MergeTree")));
+ Assertions.assertThrows(
+ UnsupportedOperationException.class,
+ () ->
ops.callGenerateAlterTableSql(TableChange.removeProperty("engine")));
+ Assertions.assertThrows(
+ UnsupportedOperationException.class,
+ () ->
+ ops.callGenerateAlterTableSql(
+ TableChange.setProperty(settingProperty("a"), "1"),
+ TableChange.removeProperty(settingProperty("b"))));
+ Assertions.assertThrows(
+ UnsupportedOperationException.class,
+ () ->
+ ops.callGenerateAlterTableSql(
+ TableChange.setProperty(settingProperty("a"), "1"),
+ TableChange.updateComment("new comment")));
+ Assertions.assertThrows(
+ UnsupportedOperationException.class,
+ () ->
+ ops.callGenerateAlterTableSql(
+ TableChange.removeProperty(settingProperty("a")),
+ TableChange.updateComment("new comment")));
+ }
+
+ @Test
+ void testRejectDuplicateTableSettingChanges() {
+ ExposedClickHouseTableOperations ops = newOps();
+
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ ops.callGenerateAlterTableSql(
+ TableChange.setProperty(settingProperty("a"), "1"),
+ TableChange.setProperty(settingProperty("a"), "2")));
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ ops.callGenerateAlterTableSql(
+ TableChange.removeProperty(settingProperty("a")),
+ TableChange.removeProperty(settingProperty("a"))));
+ }
+
+ @Test
+ void testInvalidTableSettingChangesFailBeforeJdbcConnection() {
+ DataSource dataSource = Mockito.mock(DataSource.class);
+ ClickHouseTableOperations ops = new ClickHouseTableOperations();
+ ops.initialize(
+ dataSource,
+ new ClickHouseExceptionConverter(),
+ new ClickHouseTypeConverter(),
+ new ClickHouseColumnDefaultValueConverter(),
+ new HashMap<>());
+
+ Assertions.assertThrows(
+ UnsupportedOperationException.class,
+ () -> ops.alterTable("db", "test_table",
TableChange.setProperty("engine", "MergeTree")));
+ Assertions.assertThrows(
+ UnsupportedOperationException.class,
+ () ->
+ ops.alterTable(
+ "db",
+ "test_table",
+ TableChange.setProperty(settingProperty("a"), "1"),
+ TableChange.removeProperty(settingProperty("b"))));
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ ops.alterTable(
+ "db",
+ "test_table",
+ TableChange.setProperty(settingProperty("a"), "1"),
+ TableChange.setProperty(settingProperty("a"), "2")));
+
+ Mockito.verifyNoInteractions(dataSource);
+ }
+
private RenameMocks renameMocks(String storedComment, String engineFull)
throws Exception {
DataSource dataSource = Mockito.mock(DataSource.class);
Connection connection = Mockito.mock(Connection.class);
diff --git a/docs/jdbc-clickhouse-catalog.md b/docs/jdbc-clickhouse-catalog.md
index 845964b021..9674448af0 100644
--- a/docs/jdbc-clickhouse-catalog.md
+++ b/docs/jdbc-clickhouse-catalog.md
@@ -175,7 +175,7 @@ See [Manage Catalogs and
Schemas](./manage-catalogs-and-schemas.md#schema-operat
| Indexes | Primary key; data-skipping indexes
`DATA_SKIPPING_MINMAX`, `DATA_SKIPPING_BLOOM_FILTER`, `DATA_SKIPPING_SET`,
`DATA_SKIPPING_NGRAMBFV1`, and `DATA_SKIPPING_TOKENBFV1` (configurable
granularity via `Index.properties()`).
[...]
| Distribution | Gravitino enforces `Distributions.NONE`; no custom
distribution strategies.
|
| Column defaults | Supported.
|
-| Unsupported | Engine change after creation; removing table
properties; auto-increment columns.
|
+| Unsupported | Engine and connector-owned property changes after
creation; mixing table setting changes with schema changes; auto-increment
columns.
|
### Table Column Types
@@ -204,7 +204,12 @@ Other ClickHouse types are exposed as [External
Type](./tables-and-views.md#exte
### Table Properties
:::note
-- `settings.*` keys are passed to the ClickHouse `SETTINGS` clause verbatim.
+- `settings.*` keys are passed to the ClickHouse `SETTINGS` clause during
CREATE TABLE. Their
+ values use ClickHouse scalar literal text: numbers and booleans are
unquoted, while strings must
+ be valid single-quoted literals.
+- ALTER TABLE supports setting or resetting table-level `settings.*`
properties. A request may
+ contain multiple setting operations of the same form, but cannot mix set and
remove operations
+ or combine settings with schema, comment, or index changes.
- The `engine` value is immutable after creation.
:::
@@ -223,18 +228,18 @@ If you need Gravitino to manage an existing cluster
database or table, recreate
**Memory engine data volatility**: Tables created with `engine=Memory` store
data in RAM only. After a ClickHouse server restart the table definition
persists (Gravitino's `loadTable` succeeds), but all data is permanently lost.
Gravitino metadata and ClickHouse remain consistent at the schema level, but
users are responsible for repopulating data after restarts. Consider using
`TinyLog`, `StripeLog`, or a MergeTree-family engine if data durability is
required.
:::
-| Property Name | Description
| Default Value |
Required | Reserved | Immutable |
-|---------------------------|----------------------------------------------------------------------------------------------------------|---------------|------------|----------|-----------|
-| `engine` | Table engine (for example `MergeTree`,
`ReplacingMergeTree`, `Distributed`, `Memory`, etc.) | `MergeTree`
| No | No | Yes |
-| `graphite.config` | Name of the `<graphite_rollup>` configuration
element used by `GraphiteMergeTree` | (none) |
No\*\*\* | No | No |
-| `engine_parameters` | Parameters for supported parameterized MergeTree
engines | (none) | No
| No | No |
-| `cluster-name` | Cluster name used with `ON CLUSTER` and
Distributed engine | (none)
| No\* | No | No |
-| `on-cluster` | Use `ON CLUSTER` when creating the table
| (none) | No
| No | No |
-| `cluster-remote-database` | Remote database for `Distributed` engine
| (none) |
No\*\* | No | No |
-| `cluster-remote-table` | Remote table for `Distributed` engine
| (none) |
No\*\* | No | No |
-| `cluster-sharding-key` | Sharding key for `Distributed` engine
(expression allowed; referenced columns must be non-null integral) | (none)
| No\*\* | No | No |
-| `settings.<name>` | ClickHouse engine setting forwarded as `SETTINGS
<name>=<value>` | (none) | No
| No | No |
-| `partition-key` | ClickHouse's canonical native partition
expression (from `system.tables.partition_key`). Read-only; always present on
load, empty string means unpartitioned. | `""` | No | Yes
| Yes |
+| Property Name | Description
| Default Value | Required | Reserved | Immutable |
+|---------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|----------|----------|-----------|
+| `engine` | Table engine (for example `MergeTree`,
`ReplacingMergeTree`, `Distributed`, `Memory`, etc.)
| `MergeTree` | No | No |
Yes |
+| `graphite.config` | Name of the `<graphite_rollup>` configuration
element used by `GraphiteMergeTree`
| (none) | No\*\*\* | No | No
|
+| `engine_parameters` | Parameters for supported parameterized MergeTree
engines
| (none) | No | No | No |
+| `cluster-name` | Cluster name used with `ON CLUSTER` and
Distributed engine
| (none) | No\* | No |
No |
+| `on-cluster` | Use `ON CLUSTER` when creating the table
| (none) | No | No | No |
+| `cluster-remote-database` | Remote database for `Distributed` engine
| (none) | No\*\* | No | No |
+| `cluster-remote-table` | Remote table for `Distributed` engine
| (none) | No\*\* | No | No |
+| `cluster-sharding-key` | Sharding key for `Distributed` engine
(expression allowed; referenced columns must be non-null integral)
| (none) | No\*\* | No |
No |
+| `settings.<name>` | ClickHouse engine setting forwarded as `SETTINGS
<name>=<scalar-literal>`; supports settings-only set or remove requests after
creation | (none) | No | No | No
|
+| `partition-key` | ClickHouse's canonical native partition
expression (from `system.tables.partition_key`). Read-only; always present on
load, empty string means unpartitioned. | `""` | No | Yes |
Yes |
\* Required when `on-cluster=true` or `engine=Distributed`.
\*\* Required when `engine=Distributed`.
@@ -366,10 +371,13 @@ Supported:
- Delete columns (with `IF EXISTS` support).
- Add and drop data-skipping indexes; configure custom `GRANULARITY`,
`set(N)`, and `ngrambf_v1`/`tokenbf_v1` Bloom-filter parameters via
`Index.properties()`. Adding/dropping primary key is not supported.
- Update table comment.
+- Modify table-level `settings.*` properties with `MODIFY SETTING` and reset
them with `RESET SETTING`. Each request must contain only set operations or
only remove operations.
Unsupported:
- Changing engine after creation.
-- Removing table properties or arbitrary `ALTER TABLE ... SETTINGS`.
+- Altering non-`settings.*` table properties.
+- Mixing settings with schema/comment/index changes, or mixing setting
modifications and resets in one request.
+- Column-level SETTINGS and quoted-comma SETTINGS load/recreate round-trip.
- Auto-increment columns.
See [Manage Relational Metadata Using
Gravitino](./manage-relational-metadata-using-gravitino.md#table-operations)
for common JDBC semantics.
