Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17575 )
Change subject: IMPALA-10732: Use consistent DDL for specifying Iceberg partitions ...................................................................... Patch Set 3: (4 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/17575/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17575/3//COMMIT_MSG@35 PS3, Line 35: We might later consider making 'SPEC' optional. > I think making 'SPEC' optional maybe confused users. It's better to removed Hive uses PARTITIONED BY SPEC, Spark uses PARTITIONED BY only, so it would make sense to support both in the future. Anyway, I removed this sentence because in the near future we'll only support the Hive-like PARTITIONED BY SPEC. Supporting PARTITIONED BY would require bigger changes in the SQL parser. http://gerrit.cloudera.org:8080/#/c/17575/3/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionTransform.java File fe/src/main/java/org/apache/impala/analysis/IcebergPartitionTransform.java: http://gerrit.cloudera.org:8080/#/c/17575/3/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionTransform.java@97 PS3, Line 97: builder.append(transformType_.toString() + "("); > Maybe this is better: Done http://gerrit.cloudera.org:8080/#/c/17575/3/fe/src/main/java/org/apache/impala/analysis/IcebergPartitionTransform.java@99 PS3, Line 99: builder.append(transformParam_.toString() + ", "); > Same as above Done http://gerrit.cloudera.org:8080/#/c/17575/3/fe/src/main/java/org/apache/impala/util/IcebergUtil.java File fe/src/main/java/org/apache/impala/util/IcebergUtil.java: http://gerrit.cloudera.org:8080/#/c/17575/3/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@297 PS3, Line 297: case "HOUR": case "HOURS": return TIcebergPartitionTransformType.HOUR; : case "DAY": case "DAYS": return TIcebergPartitionTransformType.DAY; : case "MONTH": case "MONTHS": return TIcebergPartitionTransformType.MONTH; : case "YEAR": case "YEARS": return TIcebergPartitionTransformType.YEAR; > I see that both Hive and Spark use HOURS/DAYS/MONTHS/YEARS in your comment I contacted the Hive team, and actually Hive supports both: https://github.com/apache/hive/blob/8ef538c6d84d0c9d7b610ca446bdc1083d62fa1b/iceberg/iceberg-handler/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergStorageHandlerNoScan.java#L182 -- To view, visit http://gerrit.cloudera.org:8080/17575 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib72ae445fd68fb0ab75d87b34779dbab922bbc62 Gerrit-Change-Number: 17575 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: wangsheng <[email protected]> Gerrit-Comment-Date: Mon, 21 Jun 2021 12:39:57 +0000 Gerrit-HasComments: Yes
