Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19547 )
Change subject: IMPALA-11950: Planner change for Iceberg metadata querying ...................................................................... Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/19547/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19547/4//COMMIT_MSG@7 PS4, Line 7: IMPALA-11950: Planner change for Iceberg metadata querying I wonder if we should add something about the changes in processing cost calculation and scan parallelism? http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/PlanNode.java@413 PS4, Line 413: == Shouldn't it be !=? Do we skip appending info at the highest explain level? http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/PlanNode.java@937 PS4, Line 937: ) Nit: no need to enclose the first two conjuncts in parentheses. http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/ScanNode.java File fe/src/main/java/org/apache/impala/planner/ScanNode.java: http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/ScanNode.java@368 PS4, Line 368: else Nit: no need for else because the THEN branch returns. http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/ScanNode.java@390 PS4, Line 390: reflect Nit: reflects. http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@866 PS4, Line 866: enclosingTupleDescs This variable is not used. This part was added here because of a rebase, right? We can remove this line now but if you think it's too unrelated we can do it in another patch too. http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/19547/4/fe/src/main/java/org/apache/impala/service/Frontend.java@294 PS4, Line 294: that Remove 'that'? -- To view, visit http://gerrit.cloudera.org:8080/19547 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3675d7a57ca570bfec306798589b5ef6aa34b5c6 Gerrit-Change-Number: 19547 Gerrit-PatchSet: 4 Gerrit-Owner: Tamas Mate <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Daniel Becker <[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: Tamas Mate <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 13 Apr 2023 12:35:56 +0000 Gerrit-HasComments: Yes
