Mihaly Szjatinya has posted comments on this change. ( http://gerrit.cloudera.org:8080/22474 )
Change subject: IMPALA-11597: Unset impala.lastComputeStatsTime during DROP STATS ...................................................................... Patch Set 8: (6 comments) Thanks for corrections, pls check. http://gerrit.cloudera.org:8080/#/c/22474/7/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/22474/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2742 PS7, Line 2742: iceTxn, catalog_.getCatalogServiceId(), modification.newVersionNumber()); > This should be always true because of L2737 Fixed http://gerrit.cloudera.org:8080/#/c/22474/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2743 PS7, Line 2743: eTxn.commitTransaction(); > This is already called at L2729. Fixed http://gerrit.cloudera.org:8080/#/c/22474/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2830 PS7, Line 2830: int numTargetedPartitions = 0; > Can we have a Precondition check that the table is not an integrated Iceber Sure, also adding the opposite to dropIcebergTableStats. http://gerrit.cloudera.org:8080/#/c/22474/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2890 PS7, Line 2890: olean isIntegratedIcebergTbl > Unused parameter Fixed http://gerrit.cloudera.org:8080/#/c/22474/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2891 PS7, Line 2891: IcebergTable.isIcebergTable(msTbl) && isIcebergHmsIntegrationEnabled(msTbl); : Preconditions.checkState(isIntegrated > I think these are redundant, as these are also called by the callers. Ok, in that case no need to pass modification. In fact even table isn't needed, but I'd keep it for precondition. http://gerrit.cloudera.org:8080/#/c/22474/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3555 PS7, Line 3555: if (isIcebergHmsInteg > IIUC, now when we issue TRUNCATE TABLE on a non-integrated Iceberg table (i I see, we probably should also rename 'dropIcebergTableStats' to 'dropIntegratedIcebergTableStats' -- To view, visit http://gerrit.cloudera.org:8080/22474 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id743bc9779141df7a26a95a0ea201ef6fc6aeff9 Gerrit-Change-Number: 22474 Gerrit-PatchSet: 8 Gerrit-Owner: Mihaly Szjatinya <msz...@pm.me> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Mihaly Szjatinya <msz...@pm.me> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 07 Mar 2025 21:31:49 +0000 Gerrit-HasComments: Yes
