Gabor Kaszab 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: (15 comments) I just scratched the surface of the patch in overall. I relied on Zoli already given +1 on PS24 so apart from a quick run-through I checked the diffs after that. http://gerrit.cloudera.org:8080/#/c/19850/31/be/src/exec/iceberg-delete-builder.h File be/src/exec/iceberg-delete-builder.h: http://gerrit.cloudera.org:8080/#/c/19850/31/be/src/exec/iceberg-delete-builder.h@16 PS31, Line 16: // under the License. I started checking the includes and forward declarations in this file and apparently a lot of them aren't used here. Could you please re-visit these and get rid of the ones you don't use in this header? http://gerrit.cloudera.org:8080/#/c/19850/31/be/src/exec/iceberg-delete-builder.h@20 PS31, Line 20: #include <deque> This is not used. http://gerrit.cloudera.org:8080/#/c/19850/31/be/src/exec/iceberg-delete-builder.h@42 PS31, Line 42: class CyclicBarrier; This class is not used in the header. http://gerrit.cloudera.org:8080/#/c/19850/31/be/src/exec/iceberg-delete-builder.h@46 PS31, Line 46: class ScalarExpr; This class is not used in the header. http://gerrit.cloudera.org:8080/#/c/19850/31/be/src/exec/iceberg-delete-builder.h@47 PS31, Line 47: class ScalarExprEvaluator; This class is not used in the header. http://gerrit.cloudera.org:8080/#/c/19850/31/be/src/exec/iceberg-delete-builder.h@134 PS31, Line 134: // The following functions are used only by IcebergDeleteNode. I personally don't really like comments like this. Currently I get that these are only used by IcebergDeleteNode, but it's pretty easy to write code in the future that would make this comment obsolete. http://gerrit.cloudera.org:8080/#/c/19850/31/be/src/exec/iceberg-delete-builder.h@137 PS31, Line 137: typo http://gerrit.cloudera.org:8080/#/c/19850/31/be/src/exec/iceberg-delete-builder.cc File be/src/exec/iceberg-delete-builder.cc: http://gerrit.cloudera.org:8080/#/c/19850/31/be/src/exec/iceberg-delete-builder.cc@21 PS31, Line 21: #include <numeric> Is this used? Similarly to the .h file, could you please check these includes if they are used anymore? http://gerrit.cloudera.org:8080/#/c/19850/31/be/src/exec/iceberg-delete-node.h File be/src/exec/iceberg-delete-node.h: http://gerrit.cloudera.org:8080/#/c/19850/31/be/src/exec/iceberg-delete-node.h@21 PS31, Line 21: #include <list> Apparently, from this import block you don't really use any of these in the header. On the other hand, you use std::vector that is not included. http://gerrit.cloudera.org:8080/#/c/19850/31/be/src/exec/iceberg-delete-node.h@28 PS31, Line 28: #include "exec/exec-node.h" If I'm not mistaken, the only use of this header is that you refer to ExecNode** in L44. Can't that be a forward declaration and get rid of this include? http://gerrit.cloudera.org:8080/#/c/19850/31/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/31/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@32 PS31, Line 32: import org.apache.impala.thrift.TJoinDistributionMode; not used http://gerrit.cloudera.org:8080/#/c/19850/31/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/31/fe/src/main/java/org/apache/impala/planner/IcebergDeleteNode.java@30 PS31, Line 30: import org.apache.impala.common.AnalysisException; not used http://gerrit.cloudera.org:8080/#/c/19850/31/fe/src/main/java/org/apache/impala/planner/IcebergDeleteNode.java@32 PS31, Line 32: import org.apache.impala.common.InternalException; not used http://gerrit.cloudera.org:8080/#/c/19850/31/fe/src/main/java/org/apache/impala/planner/IcebergDeleteNode.java@42 PS31, Line 42: import com.google.common.base.Joiner; I don't think this is used either. http://gerrit.cloudera.org:8080/#/c/19850/31/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java: http://gerrit.cloudera.org:8080/#/c/19850/31/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@58 PS31, Line 58: JOIN and operator This part of the sentence isn't meaningful I think. extra 'and'? -- 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 13:38:39 +0000 Gerrit-HasComments: Yes
