Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/22339 )
Change subject: IMPALA-13609: Store Iceberg snapshot id for COMPUTE STATS ...................................................................... Patch Set 13: (7 comments) Mostly left nits, otherwise LGTM. http://gerrit.cloudera.org:8080/#/c/22339/13/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/22339/13/be/src/service/client-request-state.cc@1883 PS13, Line 1883: return std::optional<long>() nit: I think 'return {};' would be more clear http://gerrit.cloudera.org:8080/#/c/22339/13/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/22339/13/common/thrift/JniCatalog.thrift@704 PS13, Line 704: 10: optional i64 iceberg_snapshot_id; Please add comment. I wonder if it would make sense to add this field to a parent object so we won't need to add iceberg_snapshot_id to a thrift struct for each use case. But we can do this later, if there is a need. http://gerrit.cloudera.org:8080/#/c/22339/10/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/22339/10/fe/src/main/java/org/apache/impala/service/Frontend.java@2732 PS10, Line 2732: .map(t -> String.join(", ", t.getFullName(), > I removed including the snapshotId here because we don't need it in this pa Can you please file a Jira for this? Btw we have the snapshot id in the plan at the SCAN operators. http://gerrit.cloudera.org:8080/#/c/22339/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/22339/13/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1437 PS13, Line 1437: nit: too many empty lines http://gerrit.cloudera.org:8080/#/c/22339/13/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1440 PS13, Line 1440: nit: needs another 2 spaces http://gerrit.cloudera.org:8080/#/c/22339/13/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1463 PS13, Line 1463: sb_.deleteCharAt(sb_.length() - 1); You could precondition check that it is a comma. Alternatively, you could move the sb_.append(',') to initNewRange(): if (colRangeStart_ != INVALID) sb_.append(","); http://gerrit.cloudera.org:8080/#/c/22339/13/fe/src/test/java/org/apache/impala/util/IcebergUtilTest.java File fe/src/test/java/org/apache/impala/util/IcebergUtilTest.java: http://gerrit.cloudera.org:8080/#/c/22339/13/fe/src/test/java/org/apache/impala/util/IcebergUtilTest.java@502 PS13, Line 502: // spaces are not allowed nit: earlier comments weren't inline -- To view, visit http://gerrit.cloudera.org:8080/22339 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9998b84c4fd20d1cf5e97a34f3553832ec70ae7 Gerrit-Change-Number: 22339 Gerrit-PatchSet: 13 Gerrit-Owner: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 14 Mar 2025 15:55:48 +0000 Gerrit-HasComments: Yes
