Noemi Pap-Takacs has posted comments on this change. ( http://gerrit.cloudera.org:8080/22458 )
Change subject: IMPALA-13737: Directly load file metadata via IcebergFileMetadataLoader ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/22458/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: http://gerrit.cloudera.org:8080/#/c/22458/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@246 PS2, Line 246: Whether to skip file metadata loading. (only used when HdfsTable is part of : // an Iceberg table Could you explain what determines whether this flag is set to true or false, and if it always has the same value for Iceberg tables? As far as I understood, it is always true for Iceberg tables, otherwise false. e.g. '(true iff this HdfsTable is part of an Iceberg table...)' http://gerrit.cloudera.org:8080/#/c/22458/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@247 PS2, Line 247: metadta nit: metadata http://gerrit.cloudera.org:8080/#/c/22458/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java@340 PS2, Line 340: public void setSkipFileMetadataLoading( : boolean skipFileMetadataLoading) { nit: could be in one line http://gerrit.cloudera.org:8080/#/c/22458/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java File fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java: http://gerrit.cloudera.org:8080/#/c/22458/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@195 PS2, Line 195: Preconditions.checkNotNull(fileDescriptors); Why was the 'Preconditions.checkNotNull(iceApiTable)' check removed? http://gerrit.cloudera.org:8080/#/c/22458/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@340 PS2, Line 340: // TODO IMPALA-11265: After converting to/from thrift the byte arrays representing the : // file descriptors won't be shared between the HdfsTable and IcebergContentFileStore. : // This redundancy causes unnecessary JVM heap memory usage on the coordinator. This TODO was left here because it was moved out of the scope of IMPALA-11265. This comment should be removed once this issue is solved. http://gerrit.cloudera.org:8080/#/c/22458/2/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java File fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java: http://gerrit.cloudera.org:8080/#/c/22458/2/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@135 PS2, Line 135: not Delete 'not'. The flag changed, so the comment now means the opposite. If all files must be in the table location, the file system must be the same, otherwise we cannot guarantee that. -- To view, visit http://gerrit.cloudera.org:8080/22458 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaf7e23ec21b65036b47edadcb4cbe4b64be3baee Gerrit-Change-Number: 22458 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Comment-Date: Mon, 10 Feb 2025 16:27:23 +0000 Gerrit-HasComments: Yes
