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 441e590b98 [#12915] fix(clickhouse): skip function-based index
expressions on load (#12917)
441e590b98 is described below
commit 441e590b98dba14bbd176a0660ce88cd39368476
Author: StormSpirit <[email protected]>
AuthorDate: Mon Sep 14 10:35:49 2026 +0800
[#12915] fix(clickhouse): skip function-based index expressions on load
(#12917)
### What changes were proposed in this pull request?
This pull request stops the ClickHouse index metadata parser from
recursively removing function wrappers. Direct column identifiers and
tuples containing only identifiers retain their existing behavior, while
function expressions such as `lower(name)` are rejected by the existing
field validation and handled by the existing per-index warning-and-skip
path.
The previous wrapper-unwrapping behavior is retained in a
sharding-key-specific helper so function-wrapped sharding keys such as
`cityHash64(user_id)` remain unchanged. Unsupported index warnings
retain the index, table, and type context without logging the raw
expression, `type_full`, or parser exception.
Bloom-filter parameter parsing remains outside this skip boundary, so
malformed parameter metadata continues to fail with index context.
Unit coverage verifies rejection of direct and nested function
expressions, including hash functions, and mixed tuples, while retaining
existing arithmetic-expression rejection and simple/tuple compatibility
coverage. A focused ClickHouse integration test verifies that a native
expression index remains present in the source DDL, is not exposed as a
different column index, and is not recreated with changed semantics.
### Why are the changes needed?
Gravitino's public `Index.fieldNames()` contract can represent column
references but not arbitrary ClickHouse expressions. Reporting
`lower(name)` as the field `name` fabricates metadata and causes
load-to-create round trips to produce a semantically different index.
Skipping the unsupported index preserves metadata correctness without
adding a new expression API or SQL parser.
Fix: #12915
### Does this PR introduce _any_ user-facing change?
Yes. Loading a ClickHouse table no longer exposes a function-based
data-skipping index as a plain-column Gravitino index. Unsupported
expression indexes are omitted with a warning while the table remains
loadable. Simple identifier and identifier-only tuple indexes are
unchanged, and no public API or property key is added or removed.
### How was this patch tested?
- `./gradlew :catalogs-contrib:catalog-jdbc-clickhouse:spotlessCheck
--console=plain` - passed.
- `./gradlew rat --console=plain` - passed.
- `./gradlew :catalogs-contrib:catalog-jdbc-clickhouse:test -PskipITs
--console=plain` - passed with 100 tests, 0 skipped, 0 failures, and 0
errors.
- `./gradlew :catalogs-contrib:catalog-jdbc-clickhouse:test --tests
'org.apache.gravitino.catalog.clickhouse.integration.test.CatalogClickHouseIT.testLoadExpressionIndexDoesNotFabricateColumnIndex'
-PskipDockerTests=false --console=plain --no-daemon` - passed against
ClickHouse 24.8.14 with 1 test, 0 skipped, 0 failures, and 0 errors.
- `./gradlew :catalogs-contrib:catalog-jdbc-clickhouse:test --tests
'org.apache.gravitino.catalog.clickhouse.integration.test.CatalogClickHouseClusterIT'
-PskipDockerTests=false --console=plain --no-daemon` - passed against
ClickHouse 24.8.14 with 19 tests, 0 skipped, 0 failures, and 0 errors.
- `./gradlew :catalogs-contrib:catalog-jdbc-clickhouse:build -x test
--console=plain` - passed.
- `git diff --check` - passed.
Signed-off-by: jiangxt2 <[email protected]>
Co-authored-by: Qi Yu <[email protected]>
---
.../operations/ClickHouseTableOperations.java | 21 +++++---
.../operations/ClickHouseTableSqlUtils.java | 33 ++++++------
.../integration/test/CatalogClickHouseIT.java | 59 ++++++++++++++++++++++
.../operations/TestClickHouseTableOperations.java | 4 +-
.../TestClickHouseTableOperationsIndexParsing.java | 21 +++++---
.../TestClickHouseTableOperationsUnit.java | 2 +-
6 files changed, 109 insertions(+), 31 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 308f695cf6..e21109f84d 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
@@ -1736,18 +1736,25 @@ public class ClickHouseTableOperations extends
JdbcTableOperations {
String[][] fields;
try {
indexType = getClickHouseIndexType(type);
+ } catch (IllegalArgumentException ignored) {
+ LOG.warn(
+ "Skip unsupported data skipping index {} for {}.{} with
unsupported type {}",
+ name,
+ databaseName,
+ tableName,
+ type);
+ continue;
+ }
+ try {
fields = parseIndexFields(expression);
- } catch (IllegalArgumentException e) {
+ } catch (IllegalArgumentException ignored) {
LOG.warn(
- "Skip unsupported data skipping index {} for {}.{} with type
{} "
- + "(parameter metadata={}) and expression {}",
+ "Skip unsupported data skipping index {} for {}.{} with type
{} because its "
+ + "expression cannot be represented as index field names",
name,
databaseName,
tableName,
- type,
- parameterSource,
- expression,
- e);
+ type);
continue;
}
if (ArrayUtils.isEmpty(fields)) {
diff --git
a/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableSqlUtils.java
b/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableSqlUtils.java
index f90d607f23..9681c0d301 100644
---
a/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableSqlUtils.java
+++
b/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableSqlUtils.java
@@ -84,7 +84,7 @@ final class ClickHouseTableSqlUtils {
}
static List<String> extractShardingKeyColumns(String shardingKey) {
- String normalized = normalizeIndexExpression(shardingKey);
+ String normalized = normalizeShardingKeyExpression(shardingKey);
if (StringUtils.isBlank(normalized)) {
return Collections.emptyList();
}
@@ -136,23 +136,10 @@ final class ClickHouseTableSqlUtils {
}
static String normalizeIndexExpression(String expression) {
- String trimmed = expression.trim();
-
- boolean stripped = true;
- String current = trimmed;
- while (stripped) {
- stripped = false;
- Matcher matcher = FUNCTION_WRAPPER_PATTERN.matcher(current);
- if (matcher.matches()) {
- current = matcher.group(2).trim();
- stripped = true;
- }
- }
+ String current = expression.trim();
if (StringUtils.startsWithIgnoreCase(current, "tuple(") &&
StringUtils.endsWith(current, ")")) {
current = current.substring("tuple(".length(), current.length() -
1).trim();
- } else if (StringUtils.equalsIgnoreCase(current, "tuple()")) {
- current = "";
}
return current;
@@ -175,6 +162,22 @@ final class ClickHouseTableSqlUtils {
return StringUtils.isNotBlank(identifier) &&
identifier.matches("^[a-zA-Z_][a-zA-Z0-9_]*$");
}
+ private static String normalizeShardingKeyExpression(String expression) {
+ String current = expression.trim();
+
+ boolean stripped = true;
+ while (stripped) {
+ stripped = false;
+ Matcher matcher = FUNCTION_WRAPPER_PATTERN.matcher(current);
+ if (matcher.matches()) {
+ current = matcher.group(2).trim();
+ stripped = true;
+ }
+ }
+
+ return normalizeIndexExpression(current);
+ }
+
private static Transform parsePartitionExpression(
String expression, String originalPartitionKey) {
String trimmedExpression = StringUtils.trim(expression);
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 022070db0d..a9ce6f64ea 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
@@ -531,6 +531,65 @@ public class CatalogClickHouseIT extends BaseIT {
&& Arrays.deepEquals(idx.fieldNames(), new String[][]
{{"amount"}})));
}
+ @Test
+ void testLoadExpressionIndexDoesNotFabricateColumnIndex() {
+ String sourceTableName =
GravitinoITUtils.genRandomName("expression_index_source");
+ String recreatedTableName =
GravitinoITUtils.genRandomName("expression_index_recreated");
+ clickhouseService.executeQuery(
+ String.format(
+ "CREATE TABLE `%s`.`%s` ("
+ + "id UInt64, "
+ + "name String, "
+ + "INDEX idx_name name TYPE minmax GRANULARITY 1, "
+ + "INDEX idx_lower lower(name) TYPE minmax GRANULARITY 1"
+ + ") ENGINE = MergeTree ORDER BY id",
+ schemaName, sourceTableName));
+
+ String sourceCreateSql =
+ clickhouseService.executeQueryForResult(
+ String.format("SHOW CREATE TABLE `%s`.`%s`", schemaName,
sourceTableName));
+ String normalizedSourceCreateSql = sourceCreateSql.replace("`",
"").replaceAll("\\s+", "");
+ Assertions.assertTrue(
+ StringUtils.containsIgnoreCase(
+ normalizedSourceCreateSql, "INDEXidx_lowerlower(name)TYPEminmax"),
+ "Source table should retain its expression index: " + sourceCreateSql);
+
+ TableCatalog tableCatalog = catalog.asTableCatalog();
+ Table loaded = tableCatalog.loadTable(NameIdentifier.of(schemaName,
sourceTableName));
+ Index[] loadedIndexes = loaded.index();
+ Index loadedSimpleIndex =
+ Arrays.stream(loadedIndexes)
+ .filter(index -> "idx_name".equals(index.name()))
+ .findFirst()
+ .orElseThrow();
+ Assertions.assertEquals(Index.IndexType.DATA_SKIPPING_MINMAX,
loadedSimpleIndex.type());
+ Assertions.assertArrayEquals(new String[][] {{"name"}},
loadedSimpleIndex.fieldNames());
+ Assertions.assertFalse(
+ Arrays.stream(loadedIndexes).anyMatch(index ->
"idx_lower".equals(index.name())));
+
+ tableCatalog.createTable(
+ NameIdentifier.of(schemaName, recreatedTableName),
+ loaded.columns(),
+ loaded.comment(),
+ loaded.properties(),
+ loaded.partitioning(),
+ loaded.distribution(),
+ loaded.sortOrder(),
+ loaded.index());
+
+ String recreatedCreateSql =
+ clickhouseService.executeQueryForResult(
+ String.format("SHOW CREATE TABLE `%s`.`%s`", schemaName,
recreatedTableName));
+ String normalizedRecreatedCreateSql =
+ recreatedCreateSql.replace("`", "").replaceAll("\\s+", "");
+ Assertions.assertTrue(
+ StringUtils.containsIgnoreCase(normalizedRecreatedCreateSql,
"INDEXidx_namenametypeMINMAX"),
+ "Recreated table should retain the simple index: " +
recreatedCreateSql);
+ Assertions.assertFalse(
+ StringUtils.containsIgnoreCase(normalizedRecreatedCreateSql,
"idx_lower"),
+ "Recreated table must not contain a fabricated replacement index: " +
recreatedCreateSql);
+ }
+
@Test
void testCreateAndLoadCompositePrimaryKey() {
String table = GravitinoITUtils.genRandomName("composite_primary_key");
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 9ff657b9c3..c1b00b36b9 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
@@ -1357,8 +1357,8 @@ public class TestClickHouseTableOperations extends
TestClickHouse {
String[][] fields = ops.parseIndexFields("tuple(`c2`, `c3`)");
Assertions.assertArrayEquals(new String[][] {{"c2"}, {"c3"}}, fields);
- String[][] bloomFields = ops.parseIndexFields("bloom_filter(`c4`)");
- Assertions.assertArrayEquals(new String[][] {{"c4"}}, bloomFields);
+ Assertions.assertThrows(
+ IllegalArgumentException.class, () ->
ops.parseIndexFields("bloom_filter(`c4`)"));
}
@Test
diff --git
a/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperationsIndexParsing.java
b/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperationsIndexParsing.java
index 83ad206a7f..90fc4334ea 100644
---
a/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperationsIndexParsing.java
+++
b/catalogs-contrib/catalog-jdbc-clickhouse/src/test/java/org/apache/gravitino/catalog/clickhouse/operations/TestClickHouseTableOperationsIndexParsing.java
@@ -29,17 +29,23 @@ public class TestClickHouseTableOperationsIndexParsing {
String[][] single = operations.parseIndexFields("col_1");
Assertions.assertArrayEquals(new String[][] {{"col_1"}}, single);
+ String[][] quoted = operations.parseIndexFields("`quoted_col`");
+ Assertions.assertArrayEquals(new String[][] {{"quoted_col"}}, quoted);
+
String[][] tuple = operations.parseIndexFields("tuple(`a`, b)");
Assertions.assertArrayEquals(new String[][] {{"a"}, {"b"}}, tuple);
}
@Test
- public void testParseFunctionWrappedExpression() {
- String[][] bloom =
operations.parseIndexFields("bloom_filter(cityHash64(user_id))");
- Assertions.assertArrayEquals(new String[][] {{"user_id"}}, bloom);
-
- String[][] nested =
operations.parseIndexFields("minmax(lower(`tenant_id`))");
- Assertions.assertArrayEquals(new String[][] {{"tenant_id"}}, nested);
+ public void testRejectFunctionWrappedExpression() {
+ Assertions.assertThrows(
+ IllegalArgumentException.class, () ->
operations.parseIndexFields("lower(name)"));
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
operations.parseIndexFields("bloom_filter(cityHash64(user_id))"));
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> operations.parseIndexFields("minmax(lower(`tenant_id`))"));
}
@Test
@@ -52,5 +58,8 @@ public class TestClickHouseTableOperationsIndexParsing {
public void testUnsupportedExpression() {
Assertions.assertThrows(
IllegalArgumentException.class, () ->
operations.parseIndexFields("cityHash64(id) % 16"));
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> operations.parseIndexFields("tuple(lower(name), id)"));
}
}
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 ce57e0376f..86ee8538fd 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
@@ -762,7 +762,7 @@ public class TestClickHouseTableOperationsUnit {
Mockito.when(secondaryRs.getString("type")).thenReturn("ngrambf_v1",
"tokenbf_v1");
Mockito.when(secondaryRs.getString("type_full"))
.thenReturn("ngrambf_v1(3, 512, 3, 0)", "tokenbf_v1(256, 2, 0)");
- Mockito.when(secondaryRs.getString("expr")).thenReturn("cityHash64(col_1)
% 16", "col_2");
+ Mockito.when(secondaryRs.getString("expr")).thenReturn("lower(col_1)",
"col_2");
Mockito.when(secondaryRs.getLong("granularity")).thenReturn(1L, 1L);
Connection connection = Mockito.mock(Connection.class);
