Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/24513 )
Change subject: IMPALA-15061: Refactor and Improve IcebergFileMetadataLoader ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/24513/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24513/3//COMMIT_MSG@46 PS3, Line 46: Besides some refactoring, this patch introduced the : IcebergFileMetadataLoader.LoadingMode enum and added the : NO_REMOVED_PARTITIONS case as an optimization compared to : the previous FULL_LOAD logic. It would be nice to do some benchmarks, both to see gains from the new optimizations and to so if there are other perf changes. http://gerrit.cloudera.org:8080/#/c/24513/3/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/24513/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@82 PS3, Line 82: INCREMENTAL wouldn't be APPEND_ONLY clearer? incremental can mean many things http://gerrit.cloudera.org:8080/#/c/24513/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@184 PS3, Line 184: private void initPartitionId() { : if (mode_ == LoadingMode.INCREMENTAL || mode_ == LoadingMode.NO_REMOVED_PARTITIONS) { : loadedIcebergPartitions_.putAll(oldIcebergPartitions_); naming: initPartitions? loadedIcebergPartitions_ is also modified http://gerrit.cloudera.org:8080/#/c/24513/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@193 PS3, Line 193: convertToFdsAndFilterFilesForLoading naming: handleFilesWithoutBlockLocations? this functions is kind if noop for files with block locations, just copied from one list to another http://gerrit.cloudera.org:8080/#/c/24513/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@210 PS3, Line 210: supportsStorageIds Can you do an optimization here? If requiresDataFilesInTableLocation_ is true, which is the case most of the time, then FileSystemUtil.supportsStorageIds(fsForPath) will always return the same result. We could save the result for the table, and only calculate this per file if requiresDataFilesInTableLocation_ is false. If all files need block locations then we could simply return newContentFiles before the loop. http://gerrit.cloudera.org:8080/#/c/24513/3/fe/src/main/java/org/apache/impala/catalog/IcebergFileMetadataLoader.java@214 PS3, Line 214: numUnknownDiskIds I think that this can be null, as we know that there are no block locations in this branch, we won't deal with diskIds. -- To view, visit http://gerrit.cloudera.org:8080/24513 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3faa45a311385c65a7e2ec7f46e3305c5de2ee0b Gerrit-Change-Number: 24513 Gerrit-PatchSet: 3 Gerrit-Owner: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 07 Jul 2026 14:45:35 +0000 Gerrit-HasComments: Yes
