Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17918 )
Change subject: WIP IMPALA-10569: Determine Iceberg file format from Icebert metadata ...................................................................... Patch Set 1: (5 comments) The change looks good, left some minor comments. Looking forward for the tests. http://gerrit.cloudera.org:8080/#/c/17918/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17918/1//COMMIT_MSG@7 PS1, Line 7: Icebert typo: Iceberg http://gerrit.cloudera.org:8080/#/c/17918/1/common/fbs/IcebergObjects.fbs File common/fbs/IcebergObjects.fbs: http://gerrit.cloudera.org:8080/#/c/17918/1/common/fbs/IcebergObjects.fbs@20 PS1, Line 20: enum FbFileFormat: byte { : UNKNOWN, : PARQUET, : ORC : } Now it might make sense to move this enum to a more generic place. http://gerrit.cloudera.org:8080/#/c/17918/1/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java File fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java: http://gerrit.cloudera.org:8080/#/c/17918/1/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@479 PS1, Line 479: fileFormats There is no insertion to this set in the current code. Also, as discussed offline, since we only support a single file format for the Iceberg table for now, we don't really need a set here. http://gerrit.cloudera.org:8080/#/c/17918/1/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java File fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java: http://gerrit.cloudera.org:8080/#/c/17918/1/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java@205 PS1, Line 205: && hasFileDescriptors() We'll need some tests with empty tables. http://gerrit.cloudera.org:8080/#/c/17918/1/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/17918/1/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@591 PS1, Line 591: nit: We usually don't indent multiple times when the expression only needs a single extra line. See e.g. L575. -- To view, visit http://gerrit.cloudera.org:8080/17918 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I74a583ffe91694a647a37846cd58a99e37d6ce72 Gerrit-Change-Number: 17918 Gerrit-PatchSet: 1 Gerrit-Owner: Tamas Mate <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 12 Oct 2021 15:36:37 +0000 Gerrit-HasComments: Yes
