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 13: (10 comments) http://gerrit.cloudera.org:8080/#/c/22339/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22339/3//COMMIT_MSG@41 PS3, Line 41: : Note that this change does not yet modify how Impala ch > I think this can only happen in two cases: Ok, then maybe we can leave it as it is. There is a limit on property length anyway. http://gerrit.cloudera.org:8080/#/c/22339/10/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/22339/10/be/src/service/client-request-state.cc@1913 PS10, Line 1913: alogTimeline(); > TExecRequest is created by the Frontend(.java). We could set the snapshot i Done http://gerrit.cloudera.org:8080/#/c/22339/5/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/22339/5/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@817 PS5, Line 817: > Added another suggestion in client-request-state.cc Done http://gerrit.cloudera.org:8080/#/c/22339/10/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/22339/10/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@790 PS10, Line 790: TAlterTableUpdateStatsParams params) { > Can we have unit tests for this method? Done http://gerrit.cloudera.org:8080/#/c/22339/10/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@790 PS10, Line 790: rams) { > Can we move this method to IcebergUtil? Done http://gerrit.cloudera.org:8080/#/c/22339/10/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@831 PS10, Line 831: > nit: too many spaces Done http://gerrit.cloudera.org:8080/#/c/22339/10/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@856 PS10, Line 856: > Can we move this class to IcebergUtil? Done http://gerrit.cloudera.org:8080/#/c/22339/10/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@856 PS10, Line 856: > Could you please add unit tests for this class? Done 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 patch, but otherwise I think it may make sense to add it. http://gerrit.cloudera.org:8080/#/c/22339/10/testdata/workloads/functional-query/queries/QueryTest/iceberg-compute-stats.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-compute-stats.test: http://gerrit.cloudera.org:8080/#/c/22339/10/testdata/workloads/functional-query/queries/QueryTest/iceberg-compute-stats.test@417 PS10, Line 417: \s > Do we need the .* here? If it's for whitespaces, can we use \s* instead? 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: 13 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 10 Mar 2025 15:40:23 +0000 Gerrit-HasComments: Yes
