Daniel Becker 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 14: (7 comments) 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 {}; > nit: I think 'return {};' would be more clear Done 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: // If this is an Iceberg table, stores the snapshot id for which stats are computed. > Please add comment. I wonder if it would make sense to add this field to a Added comment. I think it could make sense to extract the snapshot-id fields from child structs. I'd prefer it to be done in a separate patch. 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(), > Can you please file a Jira for this? Btw we have the snapshot id in the pla Opened IMPALA-13871. 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: private boolean canContinueRange(long col, long snapshotId) { > nit: too many empty lines Done 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 Done http://gerrit.cloudera.org:8080/#/c/22339/13/fe/src/main/java/org/apache/impala/util/IcebergUtil.java@1463 PS13, Line 1463: } > You could precondition check that it is a comma. Alternatively, you could m I went with the second approach. 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: > nit: earlier comments weren't inline Done -- 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: 14 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: Tue, 18 Mar 2025 13:55:44 +0000 Gerrit-HasComments: Yes
