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 13: -Code-Review (5 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/20204/13/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/13/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@132 PS13, Line 132: private long > nit: Is this require mutability? Can this changed to "private final long", Done http://gerrit.cloudera.org:8080/#/c/20204/13/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/20204/13/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@649 PS13, Line 649: public static long getSnapshotId(FeIcebergTable table, TimeTravelSpec timeTravelSpec) { : if (timeTravelSpec == null) return table.snapshotId(); : TableScan scan = createScanAsOf(table, timeTravelSpec); : return scan.snapshot().snapshotId(); : } > nit: Add precondition before returning to verify that snapshot id is always Iceberg currently only genereates positive snapshot IDs: https://github.com/apache/iceberg/blob/e61b0e51e151fb64cb5e39b49ea47c0809c3f84a/core/src/main/java/org/apache/iceberg/SnapshotIdGeneratorUtil.java#L36 But I don't see that restriction in the spec, so I'm not sure if we want to have that check. http://gerrit.cloudera.org:8080/#/c/20204/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java File fe/src/test/java/org/apache/impala/planner/PlannerTest.java: http://gerrit.cloudera.org:8080/#/c/20204/13/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@1327 PS13, Line 1327: runPlannerTestFile("iceberg-v2-update", "functional_parquet", > I see you added the snapshot ids to the .test file. Shouldn't you also add These tests refer to Iceberg tables that are not part of the Impala repo with already existing snapshots and files, but created during data loading, so they have random snapshot ids. http://gerrit.cloudera.org:8080/#/c/20204/13/testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test File testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test: http://gerrit.cloudera.org:8080/#/c/20204/13/testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test@172 PS13, Line 172: | Per-Host Resources: mem-estimate=20.00MB mem-reservation=4.01MB thread-reservation=2 > nit: I see some changes in this test that are irrelevant for this patch. Done http://gerrit.cloudera.org:8080/#/c/20204/13/testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test@356 PS13, Line 356: | tuple-ids=0 row-size=36B cardinality=2 > Same as above with these cardinality numbers Cardinality numbers are coming from a fix (IMPALA-12371) and they should be quite deterministic, so I think I'm keeping those. -- 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: 13 Gerrit-Owner: Gergely Fürnstáhl <g.furnst...@gmail.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Gergely Fürnstáhl <g.furnst...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tma...@apache.org> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 06 Feb 2024 17:29:54 +0000 Gerrit-HasComments: Yes
