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 13: Code-Review+1 (7 comments) Looks good, I left some nits. http://gerrit.cloudera.org:8080/#/c/22353/13/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/22353/13/be/src/service/impala-http-handler.cc@1001 PS13, Line 1001: Value error(status.GetDetail().c_str(), document->GetAllocator()); nit: this error reporting code piece is used multiple times through this file, it could be extracted as a function 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. This comment lost in PS1 :) http://gerrit.cloudera.org:8080/#/c/22353/13/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/13/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@98 PS13, Line 98: DirectMetaProvider nit: IcebergMetaProvider http://gerrit.cloudera.org:8080/#/c/22353/13/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@128 PS13, Line 128: DirectMetaProvider nit: IcebergMetaProvider http://gerrit.cloudera.org:8080/#/c/22353/13/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@216 PS13, Line 216: {}.{} it should be %s.%s http://gerrit.cloudera.org:8080/#/c/22353/13/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/13/fe/src/main/java/org/apache/impala/service/FeCatalogManager.java@200 PS13, Line 200: Preconditions.checkState(catalogConfigDir != null && It could be checked whether the provided path exists or not http://gerrit.cloudera.org:8080/#/c/22353/13/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/13/java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java@29 PS13, Line 29: import java.net.URLClassLoader; nit: unused imports -- 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: 13 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 14 Mar 2025 11:50:40 +0000 Gerrit-HasComments: Yes
