Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/22353 )
Change subject: IMPALA-13586: Initial support for Iceberg REST Catalogs ...................................................................... Patch Set 2: (20 comments) http://gerrit.cloudera.org:8080/#/c/22353/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22353/1//COMMIT_MSG@43 PS1, Line 43: functionlity typo: functionality http://gerrit.cloudera.org:8080/#/c/22353/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergRESTCatalog.java File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergRESTCatalog.java: http://gerrit.cloudera.org:8080/#/c/22353/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergRESTCatalog.java@48 PS1, Line 48: IcebergRESTCatalog The class name could be IcebergRestCatalog. http://gerrit.cloudera.org:8080/#/c/22353/1/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergRESTCatalog.java@49 PS1, Line 49: private final static Logger LOG = LoggerFactory.getLogger(IcebergRESTCatalog.class); nit: unused http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@189 PS2, Line 189: LOG.info(e.toString()); Could be LOG.error or warning http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@231 PS2, Line 231: "invalid state" It could be something more descriptive http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@277 PS2, Line 277: PartitionRef This cast is not needed http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@300 PS2, Line 300: TableMetaRefImpl This cast is not needed http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@315 PS2, Line 315: List<Partition> parts = Collections.emptyList(); As 'parts' is always empty, the for loop above can be removed http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@346 PS2, Line 346: FileMetadataLoader loader) { nit: formatting http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@597 PS2, Line 597: return false; /* Incremental stats not supported in direct mode */ nit: iceberg? http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@613 PS2, Line 613: DirectMetaProvider nit: IcebergMetaProvider http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@740 PS2, Line 740: Invalid It could be more verbose http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/service/FeCatalogManager.java File fe/src/main/java/org/apache/impala/service/FeCatalogManager.java: http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/service/FeCatalogManager.java@182 PS2, Line 182: return null; It could use an empty response object instead of null http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1126 PS2, Line 1126: != It should be checked with equals() or isEmpty() http://gerrit.cloudera.org:8080/#/c/22353/2/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/22353/2/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1060 PS2, Line 1060: ContentFile nit: indentation http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1137 PS2, Line 1137: __HIVE_DEFAULT_PARTITION__ It could be a new constant or DEFAULT_NULL_PARTITION_KEY_VALUE could be reused. http://gerrit.cloudera.org:8080/#/c/22353/2/java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java File java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java: http://gerrit.cloudera.org:8080/#/c/22353/2/java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java@54 PS2, Line 54: WAREHOUSE_LOCATION This location could be calculated from environment variables, like using DEFAULT_FS http://gerrit.cloudera.org:8080/#/c/22353/2/java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java@66 PS2, Line 66: public void start(boolean join) throws Exception { This assignment hides the member variable, if 'catalog' is not needed elsewhere, the local scoped variable is enough. http://gerrit.cloudera.org:8080/#/c/22353/2/java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java@115 PS2, Line 115: warn Could be trace with a conditional before http://gerrit.cloudera.org:8080/#/c/22353/2/testdata/bin/run-iceberg-rest-server.sh File testdata/bin/run-iceberg-rest-server.sh: http://gerrit.cloudera.org:8080/#/c/22353/2/testdata/bin/run-iceberg-rest-server.sh@24 PS2, Line 24: mvn -f "java/iceberg-rest-catalog-test/pom.xml" exec:java \ It could redirect its output to the logs/ folder -- To view, visit http://gerrit.cloudera.org:8080/22353 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1722b898b568d2f5689002f2b9bef59320cb088c Gerrit-Change-Number: 22353 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Comment-Date: Mon, 10 Feb 2025 12:28:40 +0000 Gerrit-HasComments: Yes
