Zoltan Borok-Nagy 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: (35 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/22353/2/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/22353/2/be/src/common/global-flags.cc@437 PS2, Line 437: DEFINE_string(iceberg_rest_warehouse_location, "", "Iceberg REST catalog warehouse location."); > line too long (95 > 90) Done http://gerrit.cloudera.org:8080/#/c/22353/2/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/2/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergRESTCatalog.java@116 PS2, Line 116: Map<String, String> properties) throws IcebergTableLoadingException { > line too long (94 > 90) Done 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@103 PS2, Line 103: import static org.apache.hadoop.hive.metastore.api.hive_metastoreConstants.ACCESSTYPE_READONLY; > line too long (95 > 90) Done 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 Done 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 Done 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 Done 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 Most of this method is unneded. 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 Most of this method is unneded. 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 Done http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@489 PS2, Line 489: String partName, Partition msPartition, ListMap<TNetworkAddress> hostIndex) > line too long (121 > 90) Done http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@541 PS2, Line 541: ImmutableList<FileDescriptor> fds, ImmutableList<FileDescriptor> insertFds, > line too long (108 > 90) Done 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? Done 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 Done http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@634 PS2, Line 634: msTable.getPartitionKeysSize(), Lists.newArrayList(msTable.getSd().getLocation())); > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@691 PS2, Line 691: TableMetaRef table, Map<PartitionRef, PartitionMetadata> metas) throws TException { > line too long (133 > 90) Done http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@718 PS2, Line 718: TableParams params, Table msTable) throws TException { > line too long (108 > 90) Done http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@727 PS2, Line 727: org.apache.iceberg.Table apiTable, ListMap<TNetworkAddress> hostIndex) throws TException { > line too long (96 > 90) Done 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 Done 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 I'm leaning towards null as it expresses better that nothing happened. 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() Added null-check and 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@132 PS2, Line 132: public static final String ICEBERG_REST_WAREHOUSE_LOCATION = "iceberg_rest_warehouse_location"; > line too long (97 > 90) Done 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 Done http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1070 PS2, Line 1070: private static int createIcebergMetadata(org.apache.iceberg.Table iceTbl, FlatBufferBuilder fbb, > line too long (98 > 90) Done http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1115 PS2, Line 1115: private static int createPartitionKeys(org.apache.iceberg.Table iceTbl, FlatBufferBuilder fbb, > line too long (96 > 90) Done 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 reu Done http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1236 PS2, Line 1236: ICEBERG_REST_URI, ICEBERG_REST_USER_ID, ICEBERG_REST_USER_SECRET, ICEBERG_REST_WAREHOUSE_LOCATION)); > line too long (108 > 90) Done 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: static final String WAREHOUSE_LOCATION = "hdfs://localhost:20500/test-warehouse/iceberg_test/hadoop_catalog"; > line too long (111 > 90) Done 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, That's a good idea, I also removed the SkipIf from the python test, so we can try this test on multiple storage systems. 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 elsew Yeah, seems like the member is not needed. 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 Done http://gerrit.cloudera.org:8080/#/c/22353/2/java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java@121 PS2, Line 121: // use Map so that Jackson doesn't try to instantiate ImmutableMap from payload.getClass() > line too long (99 > 90) Done http://gerrit.cloudera.org:8080/#/c/22353/2/java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java@128 PS2, Line 128: String.format("Failed to serialize and deserialize %s: %s", description, payload), e); > line too long (97 > 90) Done 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 This script can be invoked from the command line for manual testing. But in the python test I switched from PIPE to stdout/stderr. http://gerrit.cloudera.org:8080/#/c/22353/2/tests/custom_cluster/test_iceberg_rest_catalog.py File tests/custom_cluster/test_iceberg_rest_catalog.py: http://gerrit.cloudera.org:8080/#/c/22353/2/tests/custom_cluster/test_iceberg_rest_catalog.py@23 PS2, Line 23: import sys > flake8: F401 'sys' imported but unused Done http://gerrit.cloudera.org:8080/#/c/22353/2/tests/custom_cluster/test_iceberg_rest_catalog.py@28 PS2, Line 28: " > flake8: E225 missing whitespace around operator Done -- 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: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 04 Mar 2025 16:54:44 +0000 Gerrit-HasComments: Yes
