This is an automated email from the ASF dual-hosted git repository. yuqi1129 pushed a commit to branch branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit 6d20ca0c30ef78ae707b04a97c5e4263e8dfc227 Author: StormSpirit <[email protected]> AuthorDate: Wed Jul 15 19:29:40 2026 +0800 [#11946] improvement(api): Add properties field to TableChange.AddIndex for custom index parameters (#12013) ### What changes were proposed in this pull request? Add a `Map<String, String> properties` field to `TableChange.AddIndex`, enabling catalog implementations to accept custom index parameters (e.g., GRANULARITY for ClickHouse data-skipping indexes) through the ALTER TABLE ADD INDEX path. The change touches four layers: - **API** (`TableChange.java`): New `addIndex(type, name, fieldNames, properties)` factory method; existing no-properties method delegates with `Collections.emptyMap()`. Defensive copy + null safety in constructor. `equals`/`hashCode` updated. - **REST DTO** (`TableUpdateRequest.java`): Pass `index.properties()` through deserialization. - **Core** (`ManagedTableOperations.java`): Pass `addIndex.getProperties()` when building the new `Index` object. - **ClickHouse catalog**: Both CREATE TABLE and ALTER TABLE paths resolve GRANULARITY and `set(N)` max values from `index.properties()`, falling back to sensible defaults when absent. Constants defined for property keys (`granularity`, `set_max_values`). ### Why are the changes needed? `TableChange.AddIndex` currently carries only `type`, `name`, and `fieldNames` — no `properties`. This means the ALTER TABLE ADD INDEX path cannot pass index-specific parameters to catalog implementations, forcing them to use hardcoded defaults. Concrete example: the ClickHouse data-skipping index support (see #11806) allows custom GRANULARITY via `Index.properties()`. CREATE TABLE respects user-specified GRANULARITY, but ALTER TABLE ADD INDEX was stuck at `GRANULARITY 1` because `AddIndex` had no way to carry the value. This PR closes that gap for both CREATE TABLE and ALTER TABLE paths. Fix: #11946 ### Does this PR introduce _any_ user-facing change? Yes. - New public API: `TableChange.addIndex(IndexType, String, String[][], Map<String, String>)`. The existing 3-parameter overload is unchanged and fully backward compatible. - **Behavior change (ClickHouse catalog)**: bloom_filter default GRANULARITY is unified to `1` (previously `3` in CREATE TABLE, `1` in ALTER TABLE). This aligns with the ClickHouse server default. ### How was this patch tested? - **Unit tests**: `TestTableChange` — 6 new tests covering properties, null safety, equals/hashCode. `TestClickHouseTableOperations` — SQL generation for custom properties in both CREATE TABLE and ALTER TABLE paths, plus negative tests for invalid input. - **Docker integration test**: `CatalogClickHouseIT.testAlterTableAddIndexWithCustomGranularity` — end-to-end ALTER TABLE ADD INDEX with GRANULARITY=5, verified via `SHOW CREATE TABLE`. Run commands: - `./gradlew :catalogs-contrib:catalog-jdbc-clickhouse:test -PskipITs` (unit tests) - `./gradlew :catalogs-contrib:catalog-jdbc-clickhouse:test --tests "CatalogClickHouseIT" -PskipDockerTests=false` (Docker integration test) --------- Signed-off-by: jiangxt2 <[email protected]> --- .../java/org/apache/gravitino/rel/TableChange.java | 51 ++++- .../java/org/apache/gravitino/TestTableChange.java | 101 ++++++++++ .../catalog/clickhouse/ClickHouseConstants.java | 6 + .../operations/ClickHouseTableOperations.java | 95 +++++++-- .../integration/test/CatalogClickHouseIT.java | 46 +++++ .../operations/TestClickHouseTableOperations.java | 212 ++++++++++++++++++++- .../org/apache/gravitino/client/DTOConverters.java | 3 +- .../gravitino/dto/requests/TableUpdateRequest.java | 20 +- .../gravitino/catalog/ManagedTableOperations.java | 1 + docs/jdbc-clickhouse-catalog.md | 12 +- docs/open-api/indexes.yaml | 8 + ...partitioning-distribution-sort-order-indexes.md | 13 +- 12 files changed, 533 insertions(+), 35 deletions(-) diff --git a/api/src/main/java/org/apache/gravitino/rel/TableChange.java b/api/src/main/java/org/apache/gravitino/rel/TableChange.java index 136f01c8a4..719a6f3f7a 100644 --- a/api/src/main/java/org/apache/gravitino/rel/TableChange.java +++ b/api/src/main/java/org/apache/gravitino/rel/TableChange.java @@ -22,6 +22,9 @@ package org.apache.gravitino.rel; import com.google.common.base.Preconditions; import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; import java.util.Objects; import java.util.Optional; import org.apache.gravitino.annotation.Evolving; @@ -450,7 +453,22 @@ public interface TableChange { * @return A TableChange for the add index. */ static TableChange addIndex(IndexType type, String name, String[][] fieldNames) { - return new AddIndex(type, name, fieldNames); + return addIndex(type, name, fieldNames, Collections.emptyMap()); + } + + /** + * Create a TableChange for adding an index with extra properties. + * + * @param type The type of the index. + * @param name The name of the index. + * @param fieldNames The field names of the index. + * @param properties Extra properties for index configuration (e.g., granularity for ClickHouse + * data skipping indexes). If {@code null}, an empty map is used. + * @return A TableChange for the add index. + */ + static TableChange addIndex( + IndexType type, String name, String[][] fieldNames, Map<String, String> properties) { + return new AddIndex(type, name, fieldNames, properties); } /** @@ -746,6 +764,7 @@ public interface TableChange { private final String name; private final String[][] fieldNames; + private final Map<String, String> properties; /** * @param type The type of the index. @@ -753,9 +772,25 @@ public interface TableChange { * @param fieldNames The field names of the index. */ public AddIndex(IndexType type, String name, String[][] fieldNames) { + this(type, name, fieldNames, Collections.emptyMap()); + } + + /** + * @param type The type of the index. + * @param name The name of the index. + * @param fieldNames The field names of the index. + * @param properties Extra properties for index configuration. A defensive copy is made; if + * {@code null}, an empty map is used. + */ + public AddIndex( + IndexType type, String name, String[][] fieldNames, Map<String, String> properties) { this.type = type; this.name = name; this.fieldNames = fieldNames; + this.properties = + properties == null + ? Collections.emptyMap() + : Collections.unmodifiableMap(new HashMap<>(properties)); } /** @@ -779,6 +814,13 @@ public interface TableChange { return fieldNames; } + /** + * @return Extra properties for index configuration. An unmodifiable map; never {@code null}. + */ + public Map<String, String> getProperties() { + return properties; + } + @Override public boolean equals(Object o) { if (this == o) return true; @@ -786,13 +828,14 @@ public interface TableChange { AddIndex addIndex = (AddIndex) o; return type == addIndex.type && Objects.equals(name, addIndex.name) - && Arrays.deepEquals(fieldNames, addIndex.fieldNames); + && Arrays.deepEquals(fieldNames, addIndex.fieldNames) + && Objects.equals(properties, addIndex.properties); } @Override public int hashCode() { - int result = Objects.hash(type, name); - result = 31 * result + Arrays.hashCode(fieldNames); + int result = Objects.hash(type, name, properties); + result = 31 * result + Arrays.deepHashCode(fieldNames); return result; } } diff --git a/api/src/test/java/org/apache/gravitino/TestTableChange.java b/api/src/test/java/org/apache/gravitino/TestTableChange.java index 47a0e23c35..67ab3e3978 100644 --- a/api/src/test/java/org/apache/gravitino/TestTableChange.java +++ b/api/src/test/java/org/apache/gravitino/TestTableChange.java @@ -25,9 +25,13 @@ import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; import org.apache.gravitino.rel.Column; import org.apache.gravitino.rel.TableChange; import org.apache.gravitino.rel.TableChange.AddColumn; +import org.apache.gravitino.rel.TableChange.AddIndex; import org.apache.gravitino.rel.TableChange.ColumnPosition; import org.apache.gravitino.rel.TableChange.DeleteColumn; import org.apache.gravitino.rel.TableChange.RemoveProperty; @@ -41,6 +45,7 @@ import org.apache.gravitino.rel.TableChange.UpdateColumnType; import org.apache.gravitino.rel.TableChange.UpdateComment; import org.apache.gravitino.rel.expressions.Expression; import org.apache.gravitino.rel.expressions.literals.Literals; +import org.apache.gravitino.rel.indexes.Index.IndexType; import org.apache.gravitino.rel.types.Type; import org.apache.gravitino.rel.types.Types; import org.junit.jupiter.api.Assertions; @@ -626,4 +631,100 @@ public class TestTableChange { assertFalse(columnB.equals(columnA)); assertNotEquals(columnA.hashCode(), columnB.hashCode()); } + + @Test + void testAddIndexWithoutProperties() { + IndexType type = IndexType.PRIMARY_KEY; + String name = "pk_index"; + String[][] fieldNames = new String[][] {{"col1"}}; + AddIndex addIndex = (AddIndex) TableChange.addIndex(type, name, fieldNames); + + assertEquals(type, addIndex.getType()); + assertEquals(name, addIndex.getName()); + assertArrayEquals(fieldNames, addIndex.getFieldNames()); + assertEquals(Collections.emptyMap(), addIndex.getProperties()); + } + + @Test + void testAddIndexWithProperties() { + IndexType type = IndexType.DATA_SKIPPING_BLOOM_FILTER; + String name = "bf_idx"; + String[][] fieldNames = new String[][] {{"col_a"}}; + Map<String, String> properties = new HashMap<>(); + properties.put("granularity", "5"); + properties.put("bloom_filter_bytes", "64"); + + AddIndex addIndex = (AddIndex) TableChange.addIndex(type, name, fieldNames, properties); + + assertEquals(type, addIndex.getType()); + assertEquals(name, addIndex.getName()); + assertArrayEquals(fieldNames, addIndex.getFieldNames()); + assertEquals(properties, addIndex.getProperties()); + } + + @Test + void testAddIndexWithNullProperties() { + AddIndex addIndex = + (AddIndex) + TableChange.addIndex( + IndexType.DATA_SKIPPING_MINMAX, "mm_idx", new String[][] {{"col1"}}, null); + + assertEquals(Collections.emptyMap(), addIndex.getProperties()); + // null safety: calling getProperties() should never throw + assertEquals(0, addIndex.getProperties().size()); + } + + @Test + void testAddIndexWithEmptyProperties() { + AddIndex addIndex = + (AddIndex) + TableChange.addIndex( + IndexType.DATA_SKIPPING_SET, + "set_idx", + new String[][] {{"col1"}}, + Collections.emptyMap()); + + assertEquals(Collections.emptyMap(), addIndex.getProperties()); + } + + @Test + void testAddIndexEqualsHashCode() { + String[][] fields = new String[][] {{"col1"}}; + Map<String, String> props1 = Collections.singletonMap("granularity", "1"); + Map<String, String> props5 = Collections.singletonMap("granularity", "5"); + + AddIndex a1 = (AddIndex) TableChange.addIndex(IndexType.DATA_SKIPPING_MINMAX, "a", fields); + AddIndex a2 = (AddIndex) TableChange.addIndex(IndexType.DATA_SKIPPING_MINMAX, "a", fields); + AddIndex a3 = + (AddIndex) TableChange.addIndex(IndexType.DATA_SKIPPING_MINMAX, "a", fields, props1); + AddIndex a4 = + (AddIndex) TableChange.addIndex(IndexType.DATA_SKIPPING_MINMAX, "a", fields, props1); + AddIndex a5 = + (AddIndex) TableChange.addIndex(IndexType.DATA_SKIPPING_MINMAX, "a", fields, props5); + AddIndex b = (AddIndex) TableChange.addIndex(IndexType.DATA_SKIPPING_MINMAX, "b", fields); + + // Reflexivity + assertEquals(a1, a1); + assertEquals(a1.hashCode(), a1.hashCode()); + + // Equality: same params (no props) → equal + assertEquals(a1, a2); + assertEquals(a1.hashCode(), a2.hashCode()); + + // Equality: same params + same props → equal + assertEquals(a3, a4); + assertEquals(a3.hashCode(), a4.hashCode()); + + // Inequality: same params, different props → not equal + assertNotEquals(a3, a5); + + // Inequality: no props vs with props → not equal + assertNotEquals(a1, a3); + + // Inequality: different name → not equal + assertNotEquals(a1, b); + + // Null safety + assertFalse(a1.equals(null)); + } } diff --git a/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/ClickHouseConstants.java b/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/ClickHouseConstants.java index a2fca9f7b7..008de7b2aa 100644 --- a/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/ClickHouseConstants.java +++ b/catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/ClickHouseConstants.java @@ -60,5 +60,11 @@ public class ClickHouseConstants { // The name of the data skipping index type for set index in clickhouse. public static final String DATA_SKIPPING_SET = "set"; + + // Key for GRANULARITY in index properties (data-skipping index granularity). + public static final String GRANULARITY = "granularity"; + + // Key for max unique values (N) in set(N) data-skipping index properties. + public static final String SET_MAX_VALUES = "set_max_values"; } } 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 688483a9ce..befc2bc761 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 @@ -21,6 +21,8 @@ package org.apache.gravitino.catalog.clickhouse.operations; import static org.apache.gravitino.catalog.clickhouse.ClickHouseConstants.IndexConstants.DATA_SKIPPING_BLOOM_FILTER; import static org.apache.gravitino.catalog.clickhouse.ClickHouseConstants.IndexConstants.DATA_SKIPPING_MINMAX_VALUE; import static org.apache.gravitino.catalog.clickhouse.ClickHouseConstants.IndexConstants.DATA_SKIPPING_SET; +import static org.apache.gravitino.catalog.clickhouse.ClickHouseConstants.IndexConstants.GRANULARITY; +import static org.apache.gravitino.catalog.clickhouse.ClickHouseConstants.IndexConstants.SET_MAX_VALUES; import static org.apache.gravitino.catalog.clickhouse.ClickHouseTablePropertiesMetadata.CLICKHOUSE_ENGINE_KEY; import static org.apache.gravitino.catalog.clickhouse.ClickHouseTablePropertiesMetadata.ENGINE_PROPERTY_ENTRY; import static org.apache.gravitino.catalog.clickhouse.ClickHouseTablePropertiesMetadata.GRAVITINO_ENGINE_KEY; @@ -484,27 +486,34 @@ public class ClickHouseTableOperations extends JdbcTableOperations { sqlBuilder.append(" PRIMARY KEY (").append(fieldStr).append(")"); break; case DATA_SKIPPING_MINMAX: - // The GRANULARITY value is always 1 here currently as we can't set it by Index: there is - // no field for it. - // TODO(yuqi) add a properties field to Index to support user defined GRANULARITY value. sqlBuilder .append(" ") - .append(buildDataSkippingIndexDdl(index.name(), fieldStr, "minmax", 1)); + .append( + buildDataSkippingIndexDdl( + index.name(), + fieldStr, + DATA_SKIPPING_MINMAX_VALUE, + resolveGranularity(index.properties(), 1))); break; case DATA_SKIPPING_BLOOM_FILTER: - // The GRANULARITY value is always 3 here currently. - // TODO(yuqi) add a properties field to Index to support user defined GRANULARITY value. sqlBuilder .append(" ") - .append(buildDataSkippingIndexDdl(index.name(), fieldStr, "bloom_filter", 3)); + .append( + buildDataSkippingIndexDdl( + index.name(), + fieldStr, + DATA_SKIPPING_BLOOM_FILTER, + resolveGranularity(index.properties(), 1))); break; case DATA_SKIPPING_SET: - // The max unique values (N) is always 0 (unlimited) here currently as we can't set it - // by Index: there is no field for it. ClickHouse requires set(N) syntax. - // TODO(yuqi) add a properties field to Index to support user defined max unique values. sqlBuilder .append(" ") - .append(buildDataSkippingIndexDdl(index.name(), fieldStr, "set(0)", 1)); + .append( + buildDataSkippingIndexDdl( + index.name(), + fieldStr, + "set(" + resolveSetMaxValues(index.properties()) + ")", + resolveGranularity(index.properties(), 1))); break; default: throw new IllegalArgumentException( @@ -874,18 +883,31 @@ public class ClickHouseTableOperations extends JdbcTableOperations { Preconditions.checkArgument(!indexExists, "Index '%s' already exists", addIndex.getName()); String fieldStr = getIndexFieldStr(addIndex.getFieldNames()); + Map<String, String> properties = addIndex.getProperties(); switch (addIndex.getType()) { case DATA_SKIPPING_MINMAX: - return "ADD " + buildDataSkippingIndexDdl(addIndex.getName(), fieldStr, "minmax", 1); + return "ADD " + + buildDataSkippingIndexDdl( + addIndex.getName(), + fieldStr, + DATA_SKIPPING_MINMAX_VALUE, + resolveGranularity(properties, 1)); case DATA_SKIPPING_BLOOM_FILTER: - return "ADD " + buildDataSkippingIndexDdl(addIndex.getName(), fieldStr, "bloom_filter", 3); + return "ADD " + + buildDataSkippingIndexDdl( + addIndex.getName(), + fieldStr, + DATA_SKIPPING_BLOOM_FILTER, + resolveGranularity(properties, 1)); case DATA_SKIPPING_SET: - // The max unique values (N) is always 0 (unlimited) here currently as we can't set it - // by Index: there is no field for it. ClickHouse requires set(N) syntax. - // TODO(yuqi) add a properties field to Index to support user defined max unique values. - return "ADD " + buildDataSkippingIndexDdl(addIndex.getName(), fieldStr, "set(0)", 1); + return "ADD " + + buildDataSkippingIndexDdl( + addIndex.getName(), + fieldStr, + "set(" + resolveSetMaxValues(properties) + ")", + resolveGranularity(properties, 1)); case PRIMARY_KEY: throw new UnsupportedOperationException( @@ -897,6 +919,45 @@ public class ClickHouseTableOperations extends JdbcTableOperations { } } + /** + * Resolves an integer property from the index properties map. + * + * @param properties the index properties map + * @param key the property key (e.g. {@link ClickHouseConstants.IndexConstants#GRANULARITY}) + * @param defaultValue the value returned when the key is absent + * @param minValue the minimum allowed value (inclusive) + * @return the resolved integer value + * @throws IllegalArgumentException if the value is present but not a valid integer within bounds + */ + private int resolveIntProperty( + Map<String, String> properties, String key, int defaultValue, int minValue) { + if (properties == null) { + return defaultValue; + } + String raw = properties.get(key); + if (raw == null) { + return defaultValue; + } + raw = raw.trim(); + int value; + try { + value = Integer.parseInt(raw); + } catch (NumberFormatException e) { + throw new IllegalArgumentException(key + " must be a valid integer, but got: " + raw, e); + } + Preconditions.checkArgument( + value >= minValue, "%s must be >= %s, but got: %s", key, minValue, value); + return value; + } + + private int resolveGranularity(Map<String, String> properties, int defaultValue) { + return resolveIntProperty(properties, GRANULARITY, defaultValue, 1); + } + + private int resolveSetMaxValues(Map<String, String> properties) { + return resolveIntProperty(properties, SET_MAX_VALUES, 0, 0); + } + @VisibleForTesting private String deleteIndexDefinition( JdbcTable lazyLoadTable, TableChange.DeleteIndex deleteIndex) { 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 a1f64dacda..6fa6f470ca 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 @@ -1509,6 +1509,52 @@ public class CatalogClickHouseIT extends BaseIT { autoIncrementFalseException.getMessage().contains("auto increment is not supported")); } + @Test + void testAlterTableAddIndexWithCustomGranularity() { + String tableName = GravitinoITUtils.genRandomName("alter_idx_gran"); + NameIdentifier tableIdentifier = NameIdentifier.of(schemaName, tableName); + Column[] columns = + new Column[] { + Column.of("id", Types.IntegerType.get(), "id", false, false, DEFAULT_VALUE_NOT_SET), + Column.of("val", Types.IntegerType.get(), "val", false, false, DEFAULT_VALUE_NOT_SET) + }; + TableCatalog tableCatalog = catalog.asTableCatalog(); + tableCatalog.createTable( + tableIdentifier, + columns, + table_comment, + createProperties(), + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + getSortOrders("id")); + + // ALTER TABLE ADD INDEX with custom GRANULARITY via AddIndex properties + tableCatalog.alterTable( + tableIdentifier, + TableChange.addIndex( + Index.IndexType.DATA_SKIPPING_MINMAX, + "idx_val_minmax", + new String[][] {{"val"}}, + Collections.singletonMap("granularity", "5"))); + + Table loaded = tableCatalog.loadTable(tableIdentifier); + Assertions.assertTrue( + Arrays.stream(loaded.index()) + .anyMatch( + index -> + Objects.equals(index.name(), "idx_val_minmax") + && index.type() == Index.IndexType.DATA_SKIPPING_MINMAX)); + + // Verify DDL-level: SHOW CREATE TABLE should contain GRANULARITY 5 + String createSql = + clickhouseService.executeQueryForResult( + String.format("SHOW CREATE TABLE `%s`.`%s`", schemaName, tableName)); + Assertions.assertNotNull(createSql); + Assertions.assertTrue( + createSql.contains("GRANULARITY 5"), + "SHOW CREATE TABLE output should contain GRANULARITY 5: " + createSql); + } + @Test void testCreateTableWithAutoIncrementUnsupported() { String tableName = GravitinoITUtils.genRandomName("create_auto_inc"); 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 3a54b45812..635c41f021 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 @@ -1115,10 +1115,74 @@ public class TestClickHouseTableOperations extends TestClickHouse { Assertions.assertTrue(sql.contains("PARTITION BY `c1`")); Assertions.assertTrue(sql.contains("INDEX `idx_c2` `c2` TYPE minmax GRANULARITY 1")); - Assertions.assertTrue(sql.contains("INDEX `idx_c3` `c3` TYPE bloom_filter GRANULARITY 3")); + Assertions.assertTrue(sql.contains("INDEX `idx_c3` `c3` TYPE bloom_filter GRANULARITY 1")); Assertions.assertTrue(sql.contains("INDEX `idx_c4` `c2` TYPE set(0) GRANULARITY 1")); } + @Test + void testGenerateCreateTableSqlWithCustomIndexProperties() { + TestableClickHouseTableOperations ops = new TestableClickHouseTableOperations(); + ops.initialize( + null, + new ClickHouseExceptionConverter(), + new ClickHouseTypeConverter(), + new ClickHouseColumnDefaultValueConverter(), + new HashMap<>()); + + JdbcColumn[] cols = + new JdbcColumn[] { + JdbcColumn.builder() + .withName("c1") + .withType(Types.IntegerType.get()) + .withNullable(true) + .build(), + JdbcColumn.builder() + .withName("c2") + .withType(Types.StringType.get()) + .withNullable(true) + .build(), + }; + + Index[] indexes = + new Index[] { + Indexes.of( + IndexType.DATA_SKIPPING_MINMAX, + "idx_mm", + new String[][] {{"c1"}}, + Collections.singletonMap("granularity", "5")), + Indexes.of( + IndexType.DATA_SKIPPING_BLOOM_FILTER, + "idx_bf", + new String[][] {{"c2"}}, + Collections.singletonMap("granularity", "10")), + Indexes.of( + IndexType.DATA_SKIPPING_SET, + "idx_set", + new String[][] {{"c2"}}, + new HashMap<String, String>() { + { + put("set_max_values", "100"); + put("granularity", "3"); + } + }), + }; + + String sql = + ops.buildCreateSql( + "t_idx_custom", + cols, + "comment", + new HashMap<>(), + new Transform[] {}, + Distributions.NONE, + indexes, + ClickHouseUtils.getSortOrders("c1")); + + Assertions.assertTrue(sql.contains("INDEX `idx_mm` `c1` TYPE minmax GRANULARITY 5")); + Assertions.assertTrue(sql.contains("INDEX `idx_bf` `c2` TYPE bloom_filter GRANULARITY 10")); + Assertions.assertTrue(sql.contains("INDEX `idx_set` `c2` TYPE set(100) GRANULARITY 3")); + } + @Test void testGenerateCreateTableSqlWithAutoIncrementColumnUnsupported() { TestableClickHouseTableOperations ops = new TestableClickHouseTableOperations(); @@ -1380,7 +1444,7 @@ public class TestClickHouseTableOperations extends TestClickHouse { IndexType.DATA_SKIPPING_BLOOM_FILTER, "idx_bf", new String[][] {{"c2"}}) }); Assertions.assertTrue( - bloomSql.contains("ADD INDEX `idx_bf` `c2` TYPE bloom_filter GRANULARITY 3")); + bloomSql.contains("ADD INDEX `idx_bf` `c2` TYPE bloom_filter GRANULARITY 1")); String setSql = ops.buildAlterSql( @@ -1413,6 +1477,150 @@ public class TestClickHouseTableOperations extends TestClickHouse { })); } + @Test + public void testAlterTableAddIndexWithCustomGranularity() { + StubClickHouseTableOperations ops = new StubClickHouseTableOperations(); + ops.initialize( + null, + new ClickHouseExceptionConverter(), + new ClickHouseTypeConverter(), + new ClickHouseColumnDefaultValueConverter(), + new HashMap<>()); + ops.setTable(buildStubTable()); + + // Custom GRANULARITY for minmax + String minmaxSql = + ops.buildAlterSql( + "db", + "tbl", + new TableChange[] { + TableChange.addIndex( + IndexType.DATA_SKIPPING_MINMAX, + "idx_mm", + new String[][] {{"c2"}}, + Collections.singletonMap("granularity", "5")) + }); + Assertions.assertTrue(minmaxSql.contains("ADD INDEX `idx_mm` `c2` TYPE minmax GRANULARITY 5")); + + // Custom GRANULARITY for bloom_filter + String bloomSql = + ops.buildAlterSql( + "db", + "tbl", + new TableChange[] { + TableChange.addIndex( + IndexType.DATA_SKIPPING_BLOOM_FILTER, + "idx_bf", + new String[][] {{"c2"}}, + Collections.singletonMap("granularity", "10")) + }); + Assertions.assertTrue( + bloomSql.contains("ADD INDEX `idx_bf` `c2` TYPE bloom_filter GRANULARITY 10")); + + // Custom set_max_values for set index + String setSql = + ops.buildAlterSql( + "db", + "tbl", + new TableChange[] { + TableChange.addIndex( + IndexType.DATA_SKIPPING_SET, + "idx_set", + new String[][] {{"c2"}}, + new HashMap<>() { + { + put("set_max_values", "100"); + put("granularity", "3"); + } + }) + }); + Assertions.assertTrue(setSql.contains("ADD INDEX `idx_set` `c2` TYPE set(100) GRANULARITY 3")); + } + + @Test + public void testAlterTableAddIndexInvalidGranularity() { + StubClickHouseTableOperations ops = new StubClickHouseTableOperations(); + ops.initialize( + null, + new ClickHouseExceptionConverter(), + new ClickHouseTypeConverter(), + new ClickHouseColumnDefaultValueConverter(), + new HashMap<>()); + ops.setTable(buildStubTable()); + + // Non-numeric granularity + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + ops.buildAlterSql( + "db", + "tbl", + new TableChange[] { + TableChange.addIndex( + IndexType.DATA_SKIPPING_MINMAX, + "idx1", + new String[][] {{"c2"}}, + Collections.singletonMap("granularity", "abc")) + })); + + // Zero granularity + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + ops.buildAlterSql( + "db", + "tbl", + new TableChange[] { + TableChange.addIndex( + IndexType.DATA_SKIPPING_BLOOM_FILTER, + "idx2", + new String[][] {{"c2"}}, + Collections.singletonMap("granularity", "0")) + })); + } + + @Test + public void testAlterTableAddIndexInvalidSetMaxValues() { + StubClickHouseTableOperations ops = new StubClickHouseTableOperations(); + ops.initialize( + null, + new ClickHouseExceptionConverter(), + new ClickHouseTypeConverter(), + new ClickHouseColumnDefaultValueConverter(), + new HashMap<>()); + ops.setTable(buildStubTable()); + + // Non-numeric set_max_values + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + ops.buildAlterSql( + "db", + "tbl", + new TableChange[] { + TableChange.addIndex( + IndexType.DATA_SKIPPING_SET, + "idx1", + new String[][] {{"c2"}}, + Collections.singletonMap("set_max_values", "abc")) + })); + + // Negative set_max_values + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + ops.buildAlterSql( + "db", + "tbl", + new TableChange[] { + TableChange.addIndex( + IndexType.DATA_SKIPPING_SET, + "idx2", + new String[][] {{"c2"}}, + Collections.singletonMap("set_max_values", "-1")) + })); + } + @Test public void testGetClickHouseIndexType() { StubClickHouseTableOperations ops = new StubClickHouseTableOperations(); diff --git a/clients/client-java/src/main/java/org/apache/gravitino/client/DTOConverters.java b/clients/client-java/src/main/java/org/apache/gravitino/client/DTOConverters.java index 781331a553..b681bb6dea 100644 --- a/clients/client-java/src/main/java/org/apache/gravitino/client/DTOConverters.java +++ b/clients/client-java/src/main/java/org/apache/gravitino/client/DTOConverters.java @@ -235,7 +235,8 @@ class DTOConverters { return new TableUpdateRequest.AddTableIndexRequest( ((TableChange.AddIndex) change).getType(), ((TableChange.AddIndex) change).getName(), - ((TableChange.AddIndex) change).getFieldNames()); + ((TableChange.AddIndex) change).getFieldNames(), + ((TableChange.AddIndex) change).getProperties()); } else if (change instanceof TableChange.DeleteIndex) { return new TableUpdateRequest.DeleteTableIndexRequest( ((TableChange.DeleteIndex) change).getName(), diff --git a/common/src/main/java/org/apache/gravitino/dto/requests/TableUpdateRequest.java b/common/src/main/java/org/apache/gravitino/dto/requests/TableUpdateRequest.java index eb02c4cfe8..300c15c3cb 100644 --- a/common/src/main/java/org/apache/gravitino/dto/requests/TableUpdateRequest.java +++ b/common/src/main/java/org/apache/gravitino/dto/requests/TableUpdateRequest.java @@ -29,6 +29,8 @@ import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.fasterxml.jackson.databind.annotation.JsonSerialize; import com.google.common.base.Preconditions; import java.util.Arrays; +import java.util.Collections; +import java.util.Map; import javax.annotation.Nullable; import lombok.EqualsAndHashCode; import lombok.Getter; @@ -818,7 +820,20 @@ public interface TableUpdateRequest extends RESTRequest { * @param fieldNames The field names under the table contained in the index. */ public AddTableIndexRequest(Index.IndexType type, String name, String[][] fieldNames) { - this.index = Indexes.of(type, name, fieldNames); + this(type, name, fieldNames, Collections.emptyMap()); + } + + /** + * The constructor of the add table index request with extra properties. + * + * @param type The type of the index + * @param name The name of the index + * @param fieldNames The field names under the table contained in the index. + * @param properties Extra properties for index configuration + */ + public AddTableIndexRequest( + Index.IndexType type, String name, String[][] fieldNames, Map<String, String> properties) { + this.index = Indexes.of(type, name, fieldNames, properties); } /** @@ -840,7 +855,8 @@ public interface TableUpdateRequest extends RESTRequest { */ @Override public TableChange tableChange() { - return TableChange.addIndex(index.type(), index.name(), index.fieldNames()); + return TableChange.addIndex( + index.type(), index.name(), index.fieldNames(), index.properties()); } } diff --git a/core/src/main/java/org/apache/gravitino/catalog/ManagedTableOperations.java b/core/src/main/java/org/apache/gravitino/catalog/ManagedTableOperations.java index fe761e7962..0e226abcbb 100644 --- a/core/src/main/java/org/apache/gravitino/catalog/ManagedTableOperations.java +++ b/core/src/main/java/org/apache/gravitino/catalog/ManagedTableOperations.java @@ -250,6 +250,7 @@ public abstract class ManagedTableOperations implements TableCatalog { .withName(addIndex.getName()) .withFieldNames(addIndex.getFieldNames()) .withIndexType(addIndex.getType()) + .withProperties(addIndex.getProperties()) .build(); newIndexes.add(newIndex); diff --git a/docs/jdbc-clickhouse-catalog.md b/docs/jdbc-clickhouse-catalog.md index 4075ff2b38..84964e2e13 100644 --- a/docs/jdbc-clickhouse-catalog.md +++ b/docs/jdbc-clickhouse-catalog.md @@ -172,7 +172,7 @@ See [Manage Relational Metadata Using Gravitino](./manage-relational-metadata-us | Mapping | Gravitino table maps to a ClickHouse table | | Engines | **MergeTree family** (`MergeTree` default, `ReplacingMergeTree`, `SummingMergeTree`, `AggregatingMergeTree`, `CollapsingMergeTree`, `VersionedCollapsingMergeTree`, `GraphiteMergeTree`): fully supported, data persists across restarts. **Log family** (`TinyLog`, `StripeLog`, `Log`): supported, data and table definition persist across restarts. **`Null`**: supported, table persists, data is always discarded by design. **`Set`**: supported, table definition persists. [...] | Ordering/Partition | MergeTree-family requires exactly one `ORDER BY` column; only single-column identity `PARTITION BY` is supported on MergeTree engines. Other engines reject `ORDER BY`/`PARTITION BY`. | -| Indexes | Primary key; data-skipping indexes `DATA_SKIPPING_MINMAX`, `DATA_SKIPPING_BLOOM_FILTER`, and `DATA_SKIPPING_SET` (fixed granularities). | +| Indexes | Primary key; data-skipping indexes `DATA_SKIPPING_MINMAX`, `DATA_SKIPPING_BLOOM_FILTER`, and `DATA_SKIPPING_SET` (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. | @@ -240,9 +240,11 @@ If you need Gravitino to manage an existing cluster database or table, recreate - `PRIMARY_KEY` - Data-skipping indexes: - - `DATA_SKIPPING_MINMAX` (`GRANULARITY` fixed to 1) - - `DATA_SKIPPING_BLOOM_FILTER` (`GRANULARITY` fixed to 3) - - `DATA_SKIPPING_SET` (`GRANULARITY` fixed to 1) + - `DATA_SKIPPING_MINMAX` (default `GRANULARITY 1`) + - `DATA_SKIPPING_BLOOM_FILTER` (default `GRANULARITY 1`) + - `DATA_SKIPPING_SET` (default `GRANULARITY 1`, plus configurable `set(N)` max values) + + Custom `GRANULARITY` can be specified via the `Index.properties()` API (key `granularity`, value must be a positive integer). For `DATA_SKIPPING_SET`, the max unique values can be configured via `set_max_values` (non-negative integer). If not specified, the defaults above apply. ### Partitioning, Sorting, and Distribution @@ -348,7 +350,7 @@ Supported: - Rename column. - Update column type/comment/default/position/nullability. - Delete columns (with `IF EXISTS` support). -- Add data-skipping indexes; drop data-skipping indexes. Adding/dropping primary key is not supported. +- Add data-skipping indexes with custom `GRANULARITY` and `set(N)` via `Index.properties()`; drop data-skipping indexes. Adding/dropping primary key is not supported. - Update table comment. Unsupported: diff --git a/docs/open-api/indexes.yaml b/docs/open-api/indexes.yaml index ea2fe90edf..c4e1a00cf2 100644 --- a/docs/open-api/indexes.yaml +++ b/docs/open-api/indexes.yaml @@ -35,9 +35,17 @@ components: enum: - "primary_key" - "unique_key" + - "data_skipping_minmax" + - "data_skipping_bloom_filter" + - "data_skipping_set" name: type: string description: The name of the index nullable: true fieldNames: $ref: "./tables.yaml#/components/schemas/FieldNames" + properties: + type: object + additionalProperties: + type: string + description: Extra index properties (e.g., granularity for ClickHouse data-skipping indexes) diff --git a/docs/table-partitioning-distribution-sort-order-indexes.md b/docs/table-partitioning-distribution-sort-order-indexes.md index 022ef204f7..65025ec836 100644 --- a/docs/table-partitioning-distribution-sort-order-indexes.md +++ b/docs/table-partitioning-distribution-sort-order-indexes.md @@ -245,7 +245,7 @@ tableCatalog.createTable( ## Indexes -To define an indexed table, you should utilize the following three components to construct a valid indexed table: +To define an indexed table, you should utilize the following four components to construct a valid indexed table: - IndexType. Represents the type of index, such as primary key or unique key. @@ -258,6 +258,8 @@ To define an indexed table, you should utilize the following three components to - FieldNames. It defines which table fields Gravitino uses to index the table. +- Properties (optional). A map of extra index configuration properties (e.g., `granularity` for ClickHouse data-skipping indexes). If omitted, an empty map is used. + <Tabs groupId='language' queryString> <TabItem value="Json" label="JSON"> @@ -265,7 +267,8 @@ To define an indexed table, you should utilize the following three components to { "indexType": "PRIMARY_KEY", "name": "PRIMARY", - "fieldNames": [["col_1"],["col_2"]] + "fieldNames": [["col_1"],["col_2"]], + "properties": {} } ``` @@ -319,12 +322,14 @@ curl -X POST -H "Accept: application/vnd.gravitino.v1+json" \ { "indexType": "PRIMARY_KEY", "name": "PRIMARY", - "fieldNames": [["id"]] + "fieldNames": [["id"]], + "properties": {} }, { "indexType": "UNIQUE_KEY", "name": "name_age_score_uk", - "fieldNames": [["name"],["age"],["score]] + "fieldNames": [["name"],["age"],["score"]], + "properties": {} } ] }' http://localhost:8090/api/metalakes/metalake/catalogs/catalog/schemas/schema/tables
