Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/19850 )
Change subject: IMPALA-11619: Improve Iceberg V2 reads with a custom Iceberg Position Delete operator ...................................................................... Patch Set 31: (8 comments) Thanks for the update Gergo, marked some nits. Doing another round soon. http://gerrit.cloudera.org:8080/#/c/19850/30//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19850/30//COMMIT_MSG@24 PS30, Line 24: nit: could you add a testing part as well and describe the test that were done in short bullet points? http://gerrit.cloudera.org:8080/#/c/19850/30/be/src/exec/iceberg-delete-builder.h File be/src/exec/iceberg-delete-builder.h: http://gerrit.cloudera.org:8080/#/c/19850/30/be/src/exec/iceberg-delete-builder.h@104 PS30, Line 104: /// data files. nit: indentation http://gerrit.cloudera.org:8080/#/c/19850/30/be/src/exec/iceberg-delete-node.cc File be/src/exec/iceberg-delete-node.cc: http://gerrit.cloudera.org:8080/#/c/19850/30/be/src/exec/iceberg-delete-node.cc@61 PS30, Line 61: // TODO: IMPALA-12265 nit: could you add a sentence or two that describes this Jira? Like in L165. http://gerrit.cloudera.org:8080/#/c/19850/30/be/src/exec/iceberg-delete-node.cc@316 PS30, Line 316: \n nit: std::endl http://gerrit.cloudera.org:8080/#/c/19850/30/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: http://gerrit.cloudera.org:8080/#/c/19850/30/be/src/exec/partitioned-hash-join-node.cc@86 PS30, Line 86: IMPALA-12265 nit: could you add a sentence that describes this Jira? http://gerrit.cloudera.org:8080/#/c/19850/30/fe/src/main/java/org/apache/impala/analysis/JoinOperator.java File fe/src/main/java/org/apache/impala/analysis/JoinOperator.java: http://gerrit.cloudera.org:8080/#/c/19850/30/fe/src/main/java/org/apache/impala/analysis/JoinOperator.java@108 PS30, Line 108: throw new IllegalStateException("Not implemented"); nit: this is not needed http://gerrit.cloudera.org:8080/#/c/19850/30/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java: http://gerrit.cloudera.org:8080/#/c/19850/30/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@23 PS30, Line 23: * nit: we shouldn't import the whole package. http://gerrit.cloudera.org:8080/#/c/19850/30/fe/src/main/java/org/apache/impala/planner/IcebergDeleteNode.java File fe/src/main/java/org/apache/impala/planner/IcebergDeleteNode.java: http://gerrit.cloudera.org:8080/#/c/19850/30/fe/src/main/java/org/apache/impala/planner/IcebergDeleteNode.java@254 PS30, Line 254: TODO nit: missing Jira id -- To view, visit http://gerrit.cloudera.org:8080/19850 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I024a61573c83bda5584f243c879d9ff39dd2dcfa Gerrit-Change-Number: 19850 Gerrit-PatchSet: 31 Gerrit-Owner: Gergely Fürnstáhl <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Gergely Fürnstáhl <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 05 Jul 2023 12:10:46 +0000 Gerrit-HasComments: Yes
