Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20204 )
Change subject: IMPALA-12072: Include snapshot id of Iceberg tables in query plan / profile ...................................................................... Patch Set 9: (3 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/20204/9/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java File fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java: http://gerrit.cloudera.org:8080/#/c/20204/9/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java@74 PS9, Line 74: private long snapshotId_; > This could be final. Done http://gerrit.cloudera.org:8080/#/c/20204/9/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java@277 PS9, Line 277: > nit: indentation Done http://gerrit.cloudera.org:8080/#/c/20204/9/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/20204/9/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@145 PS9, Line 145: IcebergUtil.getSnapshotId(getIceTable(), tblRef_.getTimeTravelSpec()); > Shouldn't we put this inside the IcebergScanNode's constructor? I think we An IcebergScanPlanner can create up to 3 IcebergScanNode objects, so we can avoiding calling IcebergUtil.getSnapshotId() multiple times. The table ref of the DELETE scan node currently doesn't get a time travel spec, so we would also need to overload TableRef.newTableRef(). https://gerrit.cloudera.org/#/c/20866/ will add another overload to IcebergUtil.planFiles() that will take a 'long snapshotId', so we can also use 'snapshotId_' for it in filterFileDescriptors(). That being said, I don't have a too strong opinion about this, as the typical non-time travel case should be fairly efficient. Even in the case when we have time travel it shouldn't be too expensive, as we are only creating Scan objects without invoking planFiles() on them. -- To view, visit http://gerrit.cloudera.org:8080/20204 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iee0b4967429ea733729ad8e44df32e3b24b88525 Gerrit-Change-Number: 20204 Gerrit-PatchSet: 9 Gerrit-Owner: Gergely Fürnstáhl <g.furnst...@gmail.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Gergely Fürnstáhl <g.furnst...@gmail.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: Thu, 01 Feb 2024 17:33:36 +0000 Gerrit-HasComments: Yes
