lipeng...@apache.org has posted comments on this change. ( http://gerrit.cloudera.org:8080/19419 )
Change subject: IMPALA-11826: Avoid calling planFiles() on Iceberg V2 tables when there are no predicates ...................................................................... Patch Set 2: (9 comments) Left some minor comments. http://gerrit.cloudera.org:8080/#/c/19419/2/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java File fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java: http://gerrit.cloudera.org:8080/#/c/19419/2/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@754 PS2, Line 754: Iterables.concat( : icebergFiles.dataFilesWithoutDeletes, : icebergFiles.dataFilesWithDeletes, : icebergFiles.deleteFiles) I prefer to put this logic in GroupedContentFiles, which could be nameed like 'getAllContentFiles()'? http://gerrit.cloudera.org:8080/#/c/19419/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/19419/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@49 PS2, Line 49: files nit: file? http://gerrit.cloudera.org:8080/#/c/19419/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@149 PS2, Line 149: public FileDescriptor getFileDescriptor(String pathHash) { > This method appears to be unused +1 indeed. http://gerrit.cloudera.org:8080/#/c/19419/2/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/19419/2/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@32 PS2, Line 32: import org.apache.iceberg.DataFile; : import org.apache.iceberg.DeleteFile; nit: unused import. http://gerrit.cloudera.org:8080/#/c/19419/2/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@37 PS2, Line 37: FileDescriptor ditto http://gerrit.cloudera.org:8080/#/c/19419/2/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@39 PS2, Line 39: import org.apache.impala.common.Pair; ditto http://gerrit.cloudera.org:8080/#/c/19419/2/fe/src/main/java/org/apache/impala/catalog/iceberg/GroupedContentFiles.java File fe/src/main/java/org/apache/impala/catalog/iceberg/GroupedContentFiles.java: http://gerrit.cloudera.org:8080/#/c/19419/2/fe/src/main/java/org/apache/impala/catalog/iceberg/GroupedContentFiles.java@36 PS2, Line 36: GroupedContentFiles This is a nice encapsulation, the class name is explicit, and it's more elegant than 'Pair<List<DataFile>, Set<DeleteFile>>'. I think we should try not to have this kind of nesting(Pair<Collection<?>, Collection<?>) in the future, and try to wrap it into a meaningful class? http://gerrit.cloudera.org:8080/#/c/19419/2/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java File fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java: http://gerrit.cloudera.org:8080/#/c/19419/2/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@162 PS2, Line 162: private void setFileDescriptorsBasedOnFileStore() throws ImpalaException { > Can delete "throws ImpalaException" +1 indeed http://gerrit.cloudera.org:8080/#/c/19419/2/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@166 PS2, Line 166: deleteFiles_ = new HashSet<FileDescriptor>(fileStore.getDeleteFiles()); > Can be "HashSet<>" +1 indeed -- To view, visit http://gerrit.cloudera.org:8080/19419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia46bd2dce248a9e096fc1c0bd914fc3fa4686fb0 Gerrit-Change-Number: 19419 Gerrit-PatchSet: 2 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Anonymous Coward <lipeng...@apache.org> Gerrit-Reviewer: Gergely Fürnstáhl <gfurnst...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tma...@apache.org> Gerrit-Comment-Date: Fri, 13 Jan 2023 07:01:18 +0000 Gerrit-HasComments: Yes
