Copilot commented on code in PR #10383:
URL: https://github.com/apache/gravitino/pull/10383#discussion_r2938694969
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -768,6 +790,36 @@ protected String generateAlterTableSql(
return result;
}
+ private String addIndexDefinition(JdbcTable table, TableChange.AddIndex
addIndex) {
+ Preconditions.checkArgument(
+ StringUtils.isNotBlank(addIndex.getName()), "Index name is required");
+ Preconditions.checkArgument(
+ ArrayUtils.isNotEmpty(addIndex.getFieldNames()), "Index field names
are required");
+
+ boolean indexExists =
+ Arrays.stream(table.index()).anyMatch(index ->
index.name().equals(addIndex.getName()));
+ Preconditions.checkArgument(!indexExists, "Index '%s' already exists",
addIndex.getName());
+
+ String fieldStr = getIndexFieldStr(addIndex.getFieldNames());
+ switch (addIndex.getType()) {
+ case DATA_SKIPPING_MINMAX:
+ return "ADD INDEX %s %s TYPE minmax GRANULARITY 1"
+ .formatted(quoteIdentifier(addIndex.getName()), fieldStr);
+
+ case DATA_SKIPPING_BLOOM_FILTER:
+ return "ADD INDEX %s %s TYPE bloom_filter GRANULARITY 3"
+ .formatted(quoteIdentifier(addIndex.getName()), fieldStr);
+
+ case PRIMARY_KEY:
+ throw new UnsupportedOperationException(
+ "ClickHouse does not support adding primary key via ALTER TABLE");
+
+ default:
+ throw new IllegalArgumentException(
+ "Gravitino Clickhouse doesn't support index : " +
addIndex.getType());
Review Comment:
This error message is inconsistent in naming/capitalization (“Clickhouse”),
contains awkward spacing (`index :`), and isn’t very actionable. Consider
standardizing terminology (“ClickHouse”) and making the message explicit about
the unsupported type (e.g., “Unsupported index type for ClickHouse: <type>”).
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -165,6 +167,8 @@ protected String generateCreateTableSql(
Map<String, String> notNullProperties =
MapUtils.isNotEmpty(properties) ? properties : Collections.emptyMap();
+ validateNoAutoIncrementColumns(columns);
Review Comment:
The PR description says “test coverage only” and “No user-facing behavior
change”, but this PR also changes production behavior (CREATE TABLE now rejects
auto-increment columns; ALTER TABLE now supports `AddIndex`). Please update the
PR description (and the “user-facing change” section) to reflect these
functional changes, or split production changes into a separate PR if that’s
the project’s convention.
##########
docs/jdbc-clickhouse-catalog.md:
##########
@@ -156,15 +156,15 @@ See [Manage Relational Metadata Using
Gravitino](./manage-relational-metadata-us
### Table capabilities
-| Area | Details
|
-|---------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
-| Mapping | Gravitino table maps to a ClickHouse table
|
-| Engines | Local engines: MergeTree family (`MergeTree` default,
`ReplacingMergeTree`, `SummingMergeTree`, `AggregatingMergeTree`,
`CollapsingMergeTree`, `VersionedCollapsingMergeTree`, `GraphiteMergeTree`),
Tiny/Stripe/Log, Memory, File, Null, Set, Join, View, Buffer, KeeperMap, etc.
Distributed engine supports cluster mode with remote database/table and
sharding key. |
-| 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` and `DATA_SKIPPING_BLOOM_FILTER` (fixed granularities).
|
-| Distribution | Gravitino enforces `Distributions.NONE`; no custom
distribution strategies.
|
-| Column defaults | Supported.
|
-| Unsupported | Engine change after creation; removing table
properties; auto-increment columns.
|
+| Area | Details
|
+|---------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| Mapping | Gravitino table maps to a ClickHouse table
|
+| Engines | Local engines: MergeTree family (`MergeTree` default,
`ReplacingMergeTree`, `SummingMergeTree`, `AggregatingMergeTree`,
`CollapsingMergeTree`, `VersionedCollapsingMergeTree`, `GraphiteMergeTree`),
Distributed engine supports cluster mode with remote database/table and
sharding key. |
+| 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` and `DATA_SKIPPING_BLOOM_FILTER` (fixed granularities).
|
+| Distribution | Gravitino enforces `Distributions.NONE`; no custom
distribution strategies.
|
+| Column defaults | Supported.
|
+| Unsupported | Engine change after creation; removing table
properties; auto-increment columns.
|
Review Comment:
The added markdown includes what appear to be literal line numbers (`159`,
`160`, … and `325`) embedded in the document, and the table rows start with
`||` rather than a standard markdown table `| ... |`. This will render
incorrectly and likely breaks the doc formatting. Please remove the numeric
prefixes and ensure the table/list use valid markdown syntax (single leading
`|` for tables; no prefixed numbers for list items).
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -719,6 +737,10 @@ protected String generateAlterTableSql(
updateColumnNullabilityDefinition(
(TableChange.UpdateColumnNullability) change, lazyLoadTable));
+ } else if (change instanceof TableChange.AddIndex addIndex) {
+ lazyLoadTable = getOrCreateTable(databaseName, tableName,
lazyLoadTable);
+ alterSql.add(addIndexDefinition(lazyLoadTable, addIndex));
Review Comment:
The PR description says “test coverage only” and “No user-facing behavior
change”, but this PR also changes production behavior (CREATE TABLE now rejects
auto-increment columns; ALTER TABLE now supports `AddIndex`). Please update the
PR description (and the “user-facing change” section) to reflect these
functional changes, or split production changes into a separate PR if that’s
the project’s convention.
##########
docs/jdbc-clickhouse-catalog.md:
##########
@@ -322,7 +322,7 @@ Supported:
- Rename column.
- Update column type/comment/default/position/nullability.
- Delete columns (with `IF EXISTS` support).
-- Add primary or data-skipping indexes; drop data-skipping indexes.
+- Add data-skipping indexes; drop data-skipping indexes. Adding/dropping
primary key is not supported.
Review Comment:
The added markdown includes what appear to be literal line numbers (`159`,
`160`, … and `325`) embedded in the document, and the table rows start with
`||` rather than a standard markdown table `| ... |`. This will render
incorrectly and likely breaks the doc formatting. Please remove the numeric
prefixes and ensure the table/list use valid markdown syntax (single leading
`|` for tables; no prefixed numbers for list items).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
