Zoltan Borok-Nagy 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 3: (13 comments) Thanks for the 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: ble, contentFile); : nameToStats.put(name, mergePartitionStats(nameToStats, contentFile, name)); : } : return nameToStats; > I prefer to put this logic in GroupedContentFiles, which could be nameed li Done 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: file > nit: file? Done http://gerrit.cloudera.org:8080/#/c/19419/2/fe/src/main/java/org/apache/impala/catalog/IcebergContentFileStore.java@149 PS2, Line 149: public List<FileDescriptor> getDataFilesWithoutDeletes() { > +1 indeed. Done 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.impala.analysis.IcebergPartitionField; : import org.apache.impala.analysis.Ice > nit: unused import. Done http://gerrit.cloudera.org:8080/#/c/19419/2/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@37 PS2, Line 37: ec; > ditto Done 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.thrift.TGetP > ditto Done 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@34 PS2, Line 34: * Struct-like object to group different Iceberg content files: > line has trailing whitespace Done http://gerrit.cloudera.org:8080/#/c/19419/2/fe/src/main/java/org/apache/impala/catalog/iceberg/GroupedContentFiles.java@36 PS2, Line 36: es with deleted row > This is a nice encapsulation, the class name is explicit, and it's more ele Yeah, I agree. Sometimes a Pair is a handy lazy solution when we only need to group two objects, but I agree that it's nicer to introduce a new class instead. http://gerrit.cloudera.org:8080/#/c/19419/2/fe/src/main/java/org/apache/impala/catalog/iceberg/GroupedContentFiles.java@42 PS2, Line 42: public Set<DeleteFile> deleteFiles = new HashSet<>(); > This is a style thing, but it can be neater to do these initializations in Done 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() { > +1 indeed Done 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<>(fileStore.getDeleteFiles()); > +1 indeed Done http://gerrit.cloudera.org:8080/#/c/19419/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/19419/2/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@558 PS2, Line 558: try (CloseableIterable<FileScanTask> fileScanTasks = planFiles( > dataFilesWithoutDeletes, dataFilesWithDeletes, deleteFiles are unused now. Done http://gerrit.cloudera.org:8080/#/c/19419/2/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java File fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java: http://gerrit.cloudera.org:8080/#/c/19419/2/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java@287 PS2, Line 287: tblInfo.getIceberg_table().getContent_files(), > Can add Good point, thanks. -- 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: 3 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-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Mon, 16 Jan 2023 17:11:28 +0000 Gerrit-HasComments: Yes
