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 20: (25 comments) Thanks for review, Quanlong, already modified related code according to your comments. http://gerrit.cloudera.org:8080/#/c/18023/19/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/18023/19/fe/src/main/cup/sql-parser.cup@3407 PS19, Line 3407: // It's used to replace the selectivity estimated by the planner. : selectivity_hint ::= > nit: I think we don't need to mention Impala since these are already Impala Done http://gerrit.cloudera.org:8080/#/c/18023/19/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/19/fe/src/main/java/org/apache/impala/analysis/Predicate.java@37 PS19, Line 37: s no sense for a query. > nit: "0 makes no sense for a query" Done http://gerrit.cloudera.org:8080/#/c/18023/19/fe/src/main/java/org/apache/impala/analysis/Predicate.java@39 PS19, Line 39: true if the selectivity hint is set to a valid value. > nit: "true if the selectivity hint is set to a valid value" Done http://gerrit.cloudera.org:8080/#/c/18023/19/fe/src/main/java/org/apache/impala/analysis/Predicate.java@57 PS19, Line 57: hasValidSelectivityHint_ = other.hasValidSelectivityHint_; > We should copy hasValidSelectivityHint_ as well. Done http://gerrit.cloudera.org:8080/#/c/18023/19/fe/src/main/java/org/apache/impala/analysis/Predicate.java@69 PS19, Line 69: > nit: Any reason we put this here instead of put it inside analyzeHints() ? I think selectivity hint is valid for Predicate, instead of Expr, so I implements 'analyzeSelectivityHint()' in Predicate instead of Expr. But 'analyzeHints()' is in Expr, so I didn't put it inside analyzeHints(). Otherwise, we need to move 'analyzeSelectivityHint()' to Expr http://gerrit.cloudera.org:8080/#/c/18023/19/fe/src/main/java/org/apache/impala/analysis/Predicate.java@79 PS19, Line 79: > nit: "larger" Done http://gerrit.cloudera.org:8080/#/c/18023/19/fe/src/main/java/org/apache/impala/analysis/Predicate.java@177 PS19, Line 177: public boolean selectivityValidHintSet() { > Can we return hasValidSelectivityHint_ directly? Also the method name can c Done Yes, of course. I forgot modify this method in previous patch. http://gerrit.cloudera.org:8080/#/c/18023/19/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/19/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5163 PS19, Line 5163: "Syntax error in line 1"); > Let's also test some expressions like "1/3" and very long decimal values li Done http://gerrit.cloudera.org:8080/#/c/18023/19/fe/src/test/java/org/apache/impala/planner/PlannerTest.java File fe/src/test/java/org/apache/impala/planner/PlannerTest.java: http://gerrit.cloudera.org:8080/#/c/18023/19/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@1380 PS19, Line 1380: Test SELECTIVITY hints > nit: "new" will be stale. Might reword to "Test SELECTIVITY hints" Done http://gerrit.cloudera.org:8080/#/c/18023/19/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/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@1 PS19, Line 1: # Table 'tpch.lineitem' has 6001215 rows, so the scan on it has cardinality as 6.00M. > nit: "Table 'tpch.lineitem' has 6001215 rows, so the scan on it has cardina Done http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@2 PS19, Line 2: If > nit: start a new sentence with "If the selectivity of the predicate is 0.1" Done http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@4 PS19, Line 4: Planner assigns the default selectivity (0.1) to this predicate > nit: "Planner assigns the default selectivity (0.1) to this predicate" Done http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@15 PS19, Line 15: 98% of the > nit: "98% of the values" Done http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@50 PS19, Line 50: getStats().getNumNulls() / numRows > I'm confused with this. Shouldn't the selectivity a value lower than 1? Thi Done This comment seems not correct, for IS NULL predicate, selectivity is: getStats().getNumNulls() / numRows. I already update this comment, in this way, selectivity is 0 / rows = 0. http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@51 PS19, Line 51: value > nit: "values" Done http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@62 PS19, Line 62: Assuming the predicate has 0.5 as the selectivity by using the hint > nit: "Assuming the predicate has 0.5 as the selectivity by using the hint" Done http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@73 PS19, Line 73: Planner will assign the default selectivi > nit: It's the planner instead of "this predicate" that computes the selecti Done http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@84 PS19, Line 84: # The actual selectivity of this predicate is around 11.5%. Set it by the hint manually. > nit: "The actual selectivity of this predicate is around 11.5%. Set it by t Done http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@95 PS19, Line 95: # Planner will assign the default selectivity (0.1) on this predicate > nit: reword this as well Done http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@106 PS19, Line 106: # The actual selectivity of this LIKE predicate is around 11.5% (same as the above one). : # So the selectivity of the correspondin > nit: "The actual selectivity of this LIKE predicate is around 11.5% (same a Done http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@118 PS19, Line 118: # Planner will assign the default selectivity (0.1) on this predicate > nit: reword this as well Done http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@129 PS19, Line 129: # Planner will assign the default selectivity (0.1) on this predicate > nit: reword or remove this Done http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@141 PS19, Line 141: select * from tpch.lineitem where l_shipdate <= '1998-09-02' and l_shipdate >= '1997-09-02' > nit: reword this as well Done http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@209 PS19, Line 209: d add selectivity : # hint manually, the new join becomes: > nit: "the planner assigns the default selectivity (0.1) to it" Done http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@212 PS19, Line 212: > nit: remove "and" 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: 20 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: Qifan Chen <qfc...@hotmail.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.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: Wed, 22 Mar 2023 15:35:49 +0000 Gerrit-HasComments: Yes
