wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18023 )
Change subject: IMPALA-7942 (part 2): Add query hints for predicate selectivities ...................................................................... Patch Set 23: (16 comments) Hi Kurt Deschler and Xiang Yang, thanks for review. I already modify the patch as possible. http://gerrit.cloudera.org:8080/#/c/18023/23//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18023/23//COMMIT_MSG@11 PS23, Line 11: Hence, we add new hints to reduce such errors. Maybe in the future, > No need to mention histograms here. Instead just say this is giving a qay t Done http://gerrit.cloudera.org:8080/#/c/18023/23//COMMIT_MSG@14 PS23, Line 14: another > Don't say another unless you want to qualify that means Done http://gerrit.cloudera.org:8080/#/c/18023/23//COMMIT_MSG@15 PS23, Line 15: hint to original selectivity computing. > Describe the syntax additions here at a high level. i.e. The parser will in Done http://gerrit.cloudera.org:8080/#/c/18023/23//COMMIT_MSG@17 PS23, Line 17: Format like this: > Single predicate example: Done http://gerrit.cloudera.org:8080/#/c/18023/23//COMMIT_MSG@21 PS23, Line 21: Besides, this hint is also valid for compound predicate like this: > Compound Predicate Example: Done http://gerrit.cloudera.org:8080/#/c/18023/23//COMMIT_MSG@25 PS23, Line 25: But pay attention, if we want to use 'SELECTIVITY' hint for predicate, > State this first when you are describing the general syntax. Done http://gerrit.cloudera.org:8080/#/c/18023/23//COMMIT_MSG@26 PS23, Line 26: braket > nit: brackets Done http://gerrit.cloudera.org:8080/#/c/18023/23/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/18023/23/fe/src/main/java/org/apache/impala/analysis/Expr.java@1068 PS23, Line 1068: && !((CompoundPredicate) this).selectivityValidHintSet()) { > Skipping child conjuncts here based on a hint does not seem legal Why? This patch only add a selectivity hint check in original if-condition, this is illegal? As you can see, the gerrit-verify-dryrun task success. I think this additional check seem ok. http://gerrit.cloudera.org:8080/#/c/18023/23/fe/src/main/java/org/apache/impala/analysis/Predicate.java File fe/src/main/java/org/apache/impala/analysis/Predicate.java: http://gerrit.cloudera.org:8080/#/c/18023/23/fe/src/main/java/org/apache/impala/analysis/Predicate.java@38 PS23, Line 38: protected double selectivityHint_; : // true if the selectivity hint is set to a valid value. : protected boolean hasValidSelectivityHint_; > From my point of view, it's not necessary to use the redundant variable 'ha I think you are right. I remove 'hasValidSelectivityHint_' in latest patch, only reserve 'selectivityHint_', and use 'hasValidSelectivityHint()' in each kinds of Predicate. http://gerrit.cloudera.org:8080/#/c/18023/23/fe/src/main/java/org/apache/impala/analysis/Predicate.java@183 PS23, Line 183: public boolean selectivityValidHintSet() { > nit: rename to hasValidSelectivityHint() ? Done http://gerrit.cloudera.org:8080/#/c/18023/23/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java File fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java: http://gerrit.cloudera.org:8080/#/c/18023/23/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java@70 PS23, Line 70: result.setSelectivityHint(bp.getSelectivity()); > Probably better to no propagate the selectivity. If user want it to apply a This rule is to transform BetweenPredicate to CompoundPredicate, which is transparent to the user. If we don't assign BetweenPredicate selectivity hint to new created CompoundPredicate, this hint value will missing due to this transformation, which means user set selectivity hint for BetweenPredicate are always invalid. I think this maybe inappropriate. http://gerrit.cloudera.org:8080/#/c/18023/21/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/18023/21/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5144 PS21, Line 5144: > > nit: add space on both sides of the '>'. same the followings. Done http://gerrit.cloudera.org:8080/#/c/18023/21/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5144 PS21, Line 5144: > > nit: add space on both sides of the '>'. same the followings. Done http://gerrit.cloudera.org:8080/#/c/18023/21/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5181 PS21, Line 5181: > > nit: same as above. Done http://gerrit.cloudera.org:8080/#/c/18023/21/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5181 PS21, Line 5181: > > nit: same as above. Done http://gerrit.cloudera.org:8080/#/c/18023/21/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test File testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test: http://gerrit.cloudera.org:8080/#/c/18023/21/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@161 PS21, Line 161: ==== > Does there need a compound 'OR' predicate without hint test case? Done -- To view, visit http://gerrit.cloudera.org:8080/18023 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2776b9bbd878b8a21d9c866b400140a454f59e1b Gerrit-Change-Number: 18023 Gerrit-PatchSet: 23 Gerrit-Owner: wangsheng <sky...@163.com> Gerrit-Reviewer: Amogh Margoor <amarg...@gmail.com> Gerrit-Reviewer: Fucun Chu <chufu...@hotmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qfc...@hotmail.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Xiang Yang <yx91...@126.com> Gerrit-Reviewer: Yifan Zhang <chinazhangyi...@163.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: wangsheng <sky...@163.com> Gerrit-Comment-Date: Thu, 30 Mar 2023 08:54:16 +0000 Gerrit-HasComments: Yes
