Quanlong Huang 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 19: (25 comments) Thanks for your continous work on this, Sheng Wang! There are some merge conflicts. Could you rebase the patch to the latest master branch? The only issue I found is we need to copy hasValidSelectivityHint_ in the constructor of Predicate. Also left some tiny 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: // If set, Impala will use this value to replace original selectiviy : // computed in sql analysis phase. nit: I think we don't need to mention Impala since these are already Impala codes. We can make it shorter, e.g. "It's used to replace the selectivity estimated by the planner." 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: 0 is make no sense for a query nit: "0 makes no sense for a query" http://gerrit.cloudera.org:8080/#/c/18023/19/fe/src/main/java/org/apache/impala/analysis/Predicate.java@39 PS19, Line 39: true if selectivity predicate been set as a valid value nit: "true if the selectivity hint is set to a valid value" http://gerrit.cloudera.org:8080/#/c/18023/19/fe/src/main/java/org/apache/impala/analysis/Predicate.java@57 PS19, Line 57: } We should copy hasValidSelectivityHint_ as well. http://gerrit.cloudera.org:8080/#/c/18023/19/fe/src/main/java/org/apache/impala/analysis/Predicate.java@69 PS19, Line 69: analyzeSelectivityHint(analyzer); nit: Any reason we put this here instead of put it inside analyzeHints() ? http://gerrit.cloudera.org:8080/#/c/18023/19/fe/src/main/java/org/apache/impala/analysis/Predicate.java@79 PS19, Line 79: bigger nit: "larger" http://gerrit.cloudera.org:8080/#/c/18023/19/fe/src/main/java/org/apache/impala/analysis/Predicate.java@177 PS19, Line 177: return selectivityHint_ > 0 && selectivityHint_ <= 1.0; Can we return hasValidSelectivityHint_ directly? Also the method name can change to hasValidSelectivityHint(). 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 like "0.3333333333333333333333333333333333". 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 new hint of 'SELECTIVITY' nit: "new" will be stale. Might reword to "Test SELECTIVITY hints" 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' records count is 6001215, so cardinality is 6.00M, nit: "Table 'tpch.lineitem' has 6001215 rows, so the scan on it has cardinality as 6.00M." 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" http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@4 PS19, Line 4: This predicate cannot compute selectivity, default value is 0.1 nit: "Planner assigns the default selectivity (0.1) to this predicate" http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@15 PS19, Line 15: 98% values nit: "98% of the values" http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@50 PS19, Line 50: numRows - getStats().getNumNulls() I'm confused with this. Shouldn't the selectivity a value lower than 1? This seems the cardinality of the scan with "l_shipdate IS NOT NULL". 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" http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@62 PS19, Line 62: We assume that this predicate actual selectivity is 0.5 for testing, and set hint manually nit: "Assuming the predicate has 0.5 as the selectivity by using the hint" http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@73 PS19, Line 73: This predicate cannot compute selectivity nit: It's the planner instead of "this predicate" that computes the selectivity. Might reword to "Planner will assign the default selectivity (0.1) on this predicate" http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@84 PS19, Line 84: # This predicate's actual selectivity is almost 11.5%, we set hint manually nit: "The actual selectivity of this predicate is around 11.5%. Set it by the hint manually." http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@95 PS19, Line 95: # This predicate cannot compute selectivity, default value is 0.1 nit: reword this as well http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@106 PS19, Line 106: # This like predicate actual selectivity is almost 11.5% as above, so not like : # predicate actual selectivity is 88.5%. nit: "The actual selectivity of this LIKE predicate is around 11.5% (same as the above one). So the selectivity of the corresponding NOT LIKE predicate is 88.5%" http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@118 PS19, Line 118: # This predicate cannot compute selectivity, default value is 0.1 nit: reword this as well http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@129 PS19, Line 129: # We assume that this predicate actual selectivity is 0.5 for testing, and set hint manually nit: reword or remove this http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@141 PS19, Line 141: # This predicate cannot compute selectivity, default value is 0.1 nit: reword this as well http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@209 PS19, Line 209: Impala can not compute selectivity : # Impala can not compute selectivity and assumes the default value of 0.1 nit: "the planner assigns the default selectivity (0.1) to it" http://gerrit.cloudera.org:8080/#/c/18023/19/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@212 PS19, Line 212: and nit: remove "and" -- 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: 19 Gerrit-Owner: wangsheng <[email protected]> Gerrit-Reviewer: Amogh Margoor <[email protected]> Gerrit-Reviewer: Fucun Chu <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: wangsheng <[email protected]> Gerrit-Comment-Date: Wed, 22 Mar 2023 03:00:15 +0000 Gerrit-HasComments: Yes
