Copilot commented on code in PR #11806:
URL: https://github.com/apache/gravitino/pull/11806#discussion_r3510006157
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -502,6 +519,42 @@ private void appendIndexesSql(Index[] indexes,
StringBuilder sqlBuilder) {
}
}
+ /**
+ * Resolves the GRANULARITY value for a data skipping index from its
properties, falling back to
+ * the ClickHouse default. The value is interpolated directly into DDL, so
it must be a positive
+ * integer (≥ 1) to avoid generating malformed SQL.
+ *
+ * @param index the index whose {@code granularity} property is read
+ * @return the validated GRANULARITY value as a string
+ */
+ private static String resolveGranularity(Index index) {
+ String granularity =
+ index.properties().getOrDefault(INDEX_GRANULARITY_KEY,
DEFAULT_INDEX_GRANULARITY);
+ Preconditions.checkArgument(
+ StringUtils.isNumeric(granularity) && Long.parseLong(granularity) >= 1,
+ "Index %s granularity must be a positive integer (>= 1), but got %s",
+ index.name(),
+ granularity);
+ return granularity;
Review Comment:
`resolveGranularity()` can throw a raw `NumberFormatException` when
`granularity` is a digit-only string that doesn’t fit in a signed `long` (or
contains non-ASCII digits that `StringUtils.isNumeric` may accept). That
bypasses the intended `IllegalArgumentException` with a clear message and makes
validation error handling inconsistent.
--
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]
