Copilot commented on code in PR #11018:
URL: https://github.com/apache/gravitino/pull/11018#discussion_r3213151781
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableSqlUtils.java:
##########
@@ -68,17 +68,18 @@ static Transform[] parsePartitioning(String partitionKey) {
static String toPartitionExpression(Transform transform) {
Preconditions.checkArgument(transform != null, "Partition transform cannot
be null");
- Preconditions.checkArgument(
- StringUtils.equalsIgnoreCase(transform.name(),
Transforms.NAME_OF_IDENTITY),
- "Unsupported partition transform: " + transform.name());
- Preconditions.checkArgument(
- transform.arguments().length == 1
- && transform.arguments()[0] instanceof NamedReference
- && ((NamedReference) transform.arguments()[0]).fieldName().length
== 1,
- "ClickHouse only supports single column identity partitioning");
-
- String fieldName = ((NamedReference)
transform.arguments()[0]).fieldName()[0];
- return quoteIdentifier(fieldName);
+ String fieldName = partitionFieldName(transform);
+
+ if (StringUtils.equalsIgnoreCase(transform.name(),
Transforms.NAME_OF_IDENTITY)) {
+ return quoteIdentifier(fieldName);
+ } else if (StringUtils.equalsIgnoreCase(transform.name(),
Transforms.NAME_OF_YEAR)) {
+ return "toYear(%s)".formatted(quoteIdentifier(fieldName));
+ } else if (StringUtils.equalsIgnoreCase(transform.name(),
Transforms.NAME_OF_MONTH)) {
+ return "toYYYYMM(%s)".formatted(quoteIdentifier(fieldName));
+ } else if (StringUtils.equalsIgnoreCase(transform.name(),
Transforms.NAME_OF_DAY)) {
Review Comment:
`toPartitionExpression` calls `partitionFieldName(transform)` before
checking whether the transform name is supported. This changes the validation
behavior vs the previous implementation: for an unsupported transform (or any
transform with non-single-column args), the method can now throw "ClickHouse
only supports single column partitioning" instead of the intended "Unsupported
partition transform: ...". If you want to preserve the previous semantics (and
the issue’s stated behavior), validate `transform.name()` against the supported
set first, then validate/extract the field name only for supported transforms.
--
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]
