Copilot commented on code in PR #11916:
URL: https://github.com/apache/gravitino/pull/11916#discussion_r3536138631
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -858,12 +857,14 @@ private String addIndexDefinition(JdbcTable table,
TableChange.AddIndex addIndex
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);
+ return "ADD INDEX " + buildDataSkippingIndexDdl(addIndex.getName(),
fieldStr, "minmax", 1);
case DATA_SKIPPING_BLOOM_FILTER:
- return "ADD INDEX %s %s TYPE bloom_filter GRANULARITY 3"
- .formatted(quoteIdentifier(addIndex.getName()), fieldStr);
+ return "ADD INDEX "
+ + buildDataSkippingIndexDdl(addIndex.getName(), fieldStr,
"bloom_filter", 3);
+
Review Comment:
Same double-"INDEX" issue as above: this branch will emit "ADD INDEX INDEX
..." because buildDataSkippingIndexDdl() already includes the leading "INDEX"
keyword.
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -858,12 +857,14 @@ private String addIndexDefinition(JdbcTable table,
TableChange.AddIndex addIndex
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);
+ return "ADD INDEX " + buildDataSkippingIndexDdl(addIndex.getName(),
fieldStr, "minmax", 1);
case DATA_SKIPPING_BLOOM_FILTER:
- return "ADD INDEX %s %s TYPE bloom_filter GRANULARITY 3"
- .formatted(quoteIdentifier(addIndex.getName()), fieldStr);
+ return "ADD INDEX "
+ + buildDataSkippingIndexDdl(addIndex.getName(), fieldStr,
"bloom_filter", 3);
+
+ case DATA_SKIPPING_SET:
+ return "ADD INDEX " + buildDataSkippingIndexDdl(addIndex.getName(),
fieldStr, "set", 1);
Review Comment:
Same double-"INDEX" issue as above: this branch will emit "ADD INDEX INDEX
..." because buildDataSkippingIndexDdl() already includes the leading "INDEX"
keyword.
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -858,12 +857,14 @@ private String addIndexDefinition(JdbcTable table,
TableChange.AddIndex addIndex
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);
+ return "ADD INDEX " + buildDataSkippingIndexDdl(addIndex.getName(),
fieldStr, "minmax", 1);
Review Comment:
ALTER TABLE index DDL is currently generated as "ADD INDEX " +
buildDataSkippingIndexDdl(...), but buildDataSkippingIndexDdl() already
prefixes the clause with "INDEX". This produces invalid SQL like "ADD INDEX
INDEX `idx` ..." for DATA_SKIPPING_MINMAX.
--
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]
