Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/18995 )
Change subject: IMPALA-11583: Use Iceberg API to update stats ...................................................................... Patch Set 1: (5 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/18995/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/18995/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1698 PS1, Line 1698: Preconditions.checkState(msTbl.getParameters().containsKey(COMPUTE_STATS_TIME)); > Are you sure that this is always set? Is it possible for the condition at l Looking at catalog-op-executor.cc I think this should be always set, but removed from the precondition check anyway. Manually setting stats doesn't go through this code. http://gerrit.cloudera.org:8080/#/c/18995/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1707 PS1, Line 1707: IcebergCatalogOpExecutor.unsetTblProperties(iceTxn, Lists.newArrayList( : StatsSetupConst.COLUMN_STATS_ACCURATE)); > Do you know whether COLUMN_STATS_ACCURATE is used with Iceberg tables? It m I didn't want to change behavior in this CR. But I agree that we could remove this, as table-level stats are not modified and column stats are separate. http://gerrit.cloudera.org:8080/#/c/18995/1/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/18995/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-compute-stats.test@33 PS1, Line 33: compute stats ice_alltypes; > What happens if we run COMPUTE INCREMENTAL STATS for Iceberg? It shouldn't It's supported, works similarly to non-partitioned tables. Though the output can be confusing, extended IMPALA-11538 with COMPUTE STATS. http://gerrit.cloudera.org:8080/#/c/18995/1/testdata/workloads/functional-query/queries/QueryTest/iceberg-compute-stats.test@57 PS1, Line 57: ==== > Can you also add a test for setting stats manually? See https://docs.cloude Sure, done. http://gerrit.cloudera.org:8080/#/c/18995/1/tests/query_test/test_iceberg.py File tests/query_test/test_iceberg.py: http://gerrit.cloudera.org:8080/#/c/18995/1/tests/query_test/test_iceberg.py@795 PS1, Line 795: unique_database) > nit: would fit to one line Done -- To view, visit http://gerrit.cloudera.org:8080/18995 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I46b6e0a5a65e18e5aaf2a007ec0242b28e0fed92 Gerrit-Change-Number: 18995 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Gergely Fürnstáhl <gfurnst...@cloudera.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: Mon, 19 Sep 2022 13:51:29 +0000 Gerrit-HasComments: Yes
