Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/24041 )
Change subject: IMPALA-14583: Support partial RPC dispatch for Iceberg tables ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/24041/3/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/24041/3/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@154 PS3, Line 154: new HashMap<>() nit: using Collections.emptyMap() to avoid creating lots of empty maps. I think for delete files this could usually be empty. http://gerrit.cloudera.org:8080/#/c/24041/3/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@162 PS3, Line 162: Map<EncodedFileDescriptor, Hash128> reverseMap = new HashMap<>(); : for (Map.Entry<Hash128, EncodedFileDescriptor> entry : fileDescMap_.entrySet()) { : reverseMap.put(entry.getValue(), entry.getKey()); : } nit: can be simplified as Map<EncodedFileDescriptor, Hash128> reverseMap = fileDescMap_.entrySet().stream() .collect(Collectors.toMap(Map.Entry::getValue, Map.Entry::getKey)); BTW, do we need to build this for every request? It seems after the IcebergTable is loaded, its IcebergContentFileStore won't be updated anymore so we can consider these MapListContainers as unchanged. Or we can use Guava's BiMap like ImmutableBiMap in MapListContainer. http://gerrit.cloudera.org:8080/#/c/24041/3/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@586 PS3, Line 586: long count = 0; : if (contentFiles.isSetPath_hash_to_data_file_without_deletes()) { : count += contentFiles.path_hash_to_data_file_without_deletes.size(); : } : if (contentFiles.isSetPath_hash_to_data_file_with_deletes()) { : count += contentFiles.path_hash_to_data_file_with_deletes.size(); : } : if (contentFiles.isSetPath_hash_to_position_delete_file()) { : count += contentFiles.path_hash_to_position_delete_file.size(); : } : if (contentFiles.isSetPath_hash_to_equality_delete_file()) { : count += contentFiles.path_hash_to_equality_delete_file.size(); : } : return count; nit: this can be simplified as return contentFiles.getPath_hash_to_data_file_without_deletesSize() + contentFiles.getPath_hash_to_data_file_with_deletesSize() + contentFiles.getPath_hash_to_position_delete_fileSize() + contentFiles.getPath_hash_to_equality_delete_fileSize(); http://gerrit.cloudera.org:8080/#/c/24041/3/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergTable.java: http://gerrit.cloudera.org:8080/#/c/24041/3/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@892 PS3, Line 892: if (fileOffset == 0) { : partialContentFiles.setTotal_file_count(totalFiles); : } nit: To be consistent, this can be moved into IcebergContentFileStore.toThriftPartial(). -- To view, visit http://gerrit.cloudera.org:8080/24041 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f2c058b7cc8efc15bac9fe0e91baadbb7b92cbb Gerrit-Change-Number: 24041 Gerrit-PatchSet: 3 Gerrit-Owner: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 11 Mar 2026 10:46:36 +0000 Gerrit-HasComments: Yes
