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 1: (16 comments) I can't say I understand the change, but here are some comments. http://gerrit.cloudera.org:8080/#/c/20498/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20498/1//COMMIT_MSG@18 PS1, Line 18: select Nit: selects http://gerrit.cloudera.org:8080/#/c/20498/1//COMMIT_MSG@20 PS1, Line 20: produce Nit: produces. http://gerrit.cloudera.org:8080/#/c/20498/1//COMMIT_MSG@25 PS1, Line 25: POCESSING Typo: PROCESSING http://gerrit.cloudera.org:8080/#/c/20498/1//COMMIT_MSG@25 PS1, Line 25: This is because It's not clear to me how it explains that the cardinality reduction is only applied if the query option is on (as opposed to always being applied). http://gerrit.cloudera.org:8080/#/c/20498/1/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/1/fe/src/main/java/org/apache/impala/planner/PlanNode.java@128 PS1, Line 128: protected long cardinality_; Mention in the comment that it can also be replaced in replaceCardinality(). http://gerrit.cloudera.org:8080/#/c/20498/1/fe/src/main/java/org/apache/impala/planner/PlanNode.java@132 PS1, Line 132: Long Does it need to be a Long? Can't we use -1 as the value indicating an invalid/unset state, like in the case of 'cardinality_'? http://gerrit.cloudera.org:8080/#/c/20498/1/fe/src/main/java/org/apache/impala/planner/PlanNode.java@386 PS1, Line 386: expBuilder.append("(") Shouldn't we add some explanation here about what the value in parentheses means? Or would it be too much here? http://gerrit.cloudera.org:8080/#/c/20498/1/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: http://gerrit.cloudera.org:8080/#/c/20498/1/fe/src/main/java/org/apache/impala/planner/Planner.java@534 PS1, Line 534: refine Nit: refines. http://gerrit.cloudera.org:8080/#/c/20498/1/fe/src/main/java/org/apache/impala/planner/Planner.java@547 PS1, Line 547: hand Nit: do we need 'hand' here? http://gerrit.cloudera.org:8080/#/c/20498/1/fe/src/main/java/org/apache/impala/planner/Planner.java@585 PS1, Line 585: nodeStack_.get(0) Could add a precondition check that 'nodeStack_' is not empty. http://gerrit.cloudera.org:8080/#/c/20498/1/fe/src/main/java/org/apache/impala/planner/Planner.java@592 PS1, Line 592: filter.getSrc().getId() Could extract it into a variable. http://gerrit.cloudera.org:8080/#/c/20498/1/fe/src/main/java/org/apache/impala/planner/Planner.java@592 PS1, Line 592: containsKey Could use Map::computeIfAbsent() here. http://gerrit.cloudera.org:8080/#/c/20498/1/fe/src/main/java/org/apache/impala/planner/Planner.java@621 PS1, Line 621: consider Nit: considers. http://gerrit.cloudera.org:8080/#/c/20498/1/fe/src/main/java/org/apache/impala/planner/Planner.java@658 PS1, Line 658: require Nit: requires. http://gerrit.cloudera.org:8080/#/c/20498/1/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/1/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@640 PS1, Line 640: for (RuntimeFilterTarget target : targets_) { I think it would be cleaner if we inverted the if and returned from there - 'continue' would not be needed. http://gerrit.cloudera.org:8080/#/c/20498/1/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@658 PS1, Line 658: only valid for bloom filter. Can we check if it is a Bloom filter? We could add a Precondition check. -- 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: 1 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: David Rorke <dro...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Tue, 17 Oct 2023 09:27:31 +0000 Gerrit-HasComments: Yes
