Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/20903 )
Change subject: IMPALA-12708: An UPDATE creates 2 new snapshots in Iceberg tables ...................................................................... Patch Set 1: (2 comments) Thanks for this fix Zoltan, LGTM! I left a suggestion on the modifyRows method, this could be just a matter of preference. http://gerrit.cloudera.org:8080/#/c/20903/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/20903/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@354 PS1, Line 354: case DELETE: : case UPDATE: modifyRows(feIcebergTable, txn, icebergOp); break; The two operations that modifyRows manages with the two conditions makes it a bit difficult to read. I think it would be more readable to keep these two operations separate. ie.: deleteRows/updateRows. http://gerrit.cloudera.org:8080/#/c/20903/1/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@379 PS1, Line 379: if (dataFilesFb != null) { Following up on the previous comment, if I am not mistaken this can only happen with UPDATEs. -- To view, visit http://gerrit.cloudera.org:8080/20903 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2ceb80b939c644388707b21061bf55451234dcd3 Gerrit-Change-Number: 20903 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tma...@apache.org> Gerrit-Comment-Date: Tue, 16 Jan 2024 09:49:52 +0000 Gerrit-HasComments: Yes
