Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20498 )
Change subject: IMPALA-12018: Consider runtime filter for cardinality reduction ...................................................................... Patch Set 5: (18 comments) Thanks Riza http://gerrit.cloudera.org:8080/#/c/20498/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20498/5//COMMIT_MSG@18 PS5, Line 18: contigous Nit: contiguous http://gerrit.cloudera.org:8080/#/c/20498/5//COMMIT_MSG@26 PS5, Line 26: mode Nit: modes. http://gerrit.cloudera.org:8080/#/c/20498/5//COMMIT_MSG@26 PS5, Line 26: reduction in "... reduction is present/available in all ..." http://gerrit.cloudera.org:8080/#/c/20498/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java File fe/src/main/java/org/apache/impala/planner/JoinNode.java: http://gerrit.cloudera.org:8080/#/c/20498/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java@454 PS5, Line 454: internal Now that it's public, is it still internal? http://gerrit.cloudera.org:8080/#/c/20498/5/fe/src/main/java/org/apache/impala/planner/JoinNode.java@475 PS5, Line 475: getGenericJoinCardinality This method was renamed. http://gerrit.cloudera.org:8080/#/c/20498/5/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/20498/5/fe/src/main/java/org/apache/impala/planner/PlanNode.java@1180 PS5, Line 1180: ScanNode and JoinNodes Aren't also ExchangeNodes included? http://gerrit.cloudera.org:8080/#/c/20498/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java File fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java: http://gerrit.cloudera.org:8080/#/c/20498/5/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@710 PS5, Line 710: || buildSlotRef.getDesc().getParent().getTable() == null Is it possible that buildSlotRef.getDesc().getParent() is null? http://gerrit.cloudera.org:8080/#/c/20498/5/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/20498/5/fe/src/main/java/org/apache/impala/planner/ScanNode.java@22 PS5, Line 22: import java.util.HashSet; Seems to be unused. http://gerrit.cloudera.org:8080/#/c/20498/5/fe/src/main/java/org/apache/impala/planner/ScanNode.java@25 PS5, Line 25: import java.util.Set; Seems to be unused. http://gerrit.cloudera.org:8080/#/c/20498/5/fe/src/main/java/org/apache/impala/planner/ScanNode.java@102 PS5, Line 102: Set in Planner.applyRuntimeFilterSelectivity() It is not set there, and I can't find any other place where it's set. http://gerrit.cloudera.org:8080/#/c/20498/5/fe/src/main/java/org/apache/impala/planner/ScanNode.java@456 PS5, Line 456: protected void reduceCardinalityByRuntimeFilter(Stack<PlanNode> nodeStack) { Could you describe the algorithm also here? http://gerrit.cloudera.org:8080/#/c/20498/5/fe/src/main/java/org/apache/impala/planner/ScanNode.java@470 PS5, Line 470: for (RuntimeFilterGenerator.RuntimeFilter filter : getRuntimeFilters()) { Could extract this loop into a function. http://gerrit.cloudera.org:8080/#/c/20498/5/fe/src/main/java/org/apache/impala/planner/ScanNode.java@474 PS5, Line 474: aplied Nit: applies http://gerrit.cloudera.org:8080/#/c/20498/5/fe/src/main/java/org/apache/impala/planner/ScanNode.java@475 PS5, Line 475: applied Nit: applies (present tense) would be better. http://gerrit.cloudera.org:8080/#/c/20498/5/fe/src/main/java/org/apache/impala/planner/ScanNode.java@487 PS5, Line 487: for (int i = nodeStack.size() - 1; i >= 0; i--) { Could extract this loop into a function. http://gerrit.cloudera.org:8080/#/c/20498/5/fe/src/main/java/org/apache/impala/planner/ScanNode.java@510 PS5, Line 510: applies Nit: apply http://gerrit.cloudera.org:8080/#/c/20498/5/fe/src/main/java/org/apache/impala/planner/ScanNode.java@517 PS5, Line 517: colName = targetSlot.getDesc().getColumn().getName(); If 'colName' isn't updated here for more than one column, those columns will be handled if they were the same. http://gerrit.cloudera.org:8080/#/c/20498/5/fe/src/main/java/org/apache/impala/planner/ScanNode.java@528 PS5, Line 528: reducedCardinality = Math.max(reducedCardinality, highestJoinCard); I don't understand why we choose the max here, we've been reducing 'reducedCardinality' so far and 'nodeStack.get(0).getCardinality()' was not yet updated. Could you explain? Thanks. -- To view, visit http://gerrit.cloudera.org:8080/20498 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I033789c9b63a8188484e3afde8e646563918b3e1 Gerrit-Change-Number: 20498 Gerrit-PatchSet: 5 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: David Rorke <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Thu, 09 Nov 2023 13:08:54 +0000 Gerrit-HasComments: Yes
