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 13: (29 comments) > Can you please provide feedback on all comments? > > For example, for the following comment, a feedback can be DONE, a > reply etc. > > Commit Message > Line 14: > TABLE_NUM_ROWS? > > The feedbacks are useful to judge the rework. Thanks! Ok, Qifan, thanks for reply. I've already answer all comments. http://gerrit.cloudera.org:8080/#/c/18023/9/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/9/fe/src/main/java/org/apache/impala/analysis/Predicate.java@78 PS9, Line 78: ti > nit. should be a double value in (0, 1]. Done http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/main/java/org/apache/impala/analysis/TableRef.java File fe/src/main/java/org/apache/impala/analysis/TableRef.java: http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/main/java/org/apache/impala/analysis/TableRef.java@547 PS9, Line 547: return; > The above hints are all limited to hdfs tables since they are related to hd Done http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/main/java/org/apache/impala/analysis/TableRef.java@554 PS9, Line 554: } > nit. for this HDFS table reference. On paper, one can assign different # ro Done http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/main/java/org/apache/impala/analysis/TableRef.java@555 PS9, Line 555: > It seems we need to handle NumberFormatException thrown from this method. F Done http://gerrit.cloudera.org:8080/#/c/18023/9/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/18023/9/fe/src/main/java/org/apache/impala/planner/ScanNode.java@81 PS9, Line 81: H > nit. "." Done http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/main/java/org/apache/impala/planner/ScanNode.java@82 PS9, Line 82: ng tableNumRowsHint_ = -1; : > I wonder if this is still correct. I thought the hint can be used to overri Done http://gerrit.cloudera.org:8080/#/c/18023/9/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/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5053 PS9, Line 5053: void testCreatePartitio > This error may not be user friendly for super long SQL query. Done http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5056 PS9, Line 5056: "PARTITIONED BY SPEC (BUC > same comment as above. Done http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5060 PS9, Line 5060: "STORED AS ICEBERG" + tb > same comment as above. Done http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5064 PS9, Line 5064: int, > nit. may add a qualifier "non-HDFS" before 'table' to make it clear. Done http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5071 PS9, Line 5071: "PARTITIONED BY SPEC (BU > See the comment on TABLE_NUM_ROWS for "syntax error in line 1". Done http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5074 PS9, Line 5074: "PARTITIONED BY SPEC (TRUNCATE(0, p1), DAY(p2)) STORED AS ICEBERG" + > Isn't this supported now in patch set 9? Done http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5085 PS9, Line 5085: Legal hint return correct > is this a mistake? 0.1 is a perfect selectivity value. Done http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5087 PS9, Line 5087: zesOk("select * from tp > Same as above. 0 is okay. Done http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5093 PS9, Line 5093: > should be [0,1], right? Done http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5098 PS9, Line 5098: gal > See the comment about. Should be allowed. Done http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5102 PS9, Line 5102: : // Multiple illegal hints wil > From the updated parser, it seems this hint should be accepted in that the Done http://gerrit.cloudera.org:8080/#/c/18023/9/testdata/workloads/functional-planner/queries/PlannerTest/hdfs-cardinality-hint.test File testdata/workloads/functional-planner/queries/PlannerTest/hdfs-cardinality-hint.test: http://gerrit.cloudera.org:8080/#/c/18023/9/testdata/workloads/functional-planner/queries/PlannerTest/hdfs-cardinality-hint.test@90 PS9, Line 90: > same argument as for selectivity hints. We probably do not need to test PAR Done http://gerrit.cloudera.org:8080/#/c/18023/9/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/9/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@2 PS9, Line 2: CAN HDFS' is 6001215 > Please add a comment after it to indicate the cardinality clause in the sca Done http://gerrit.cloudera.org:8080/#/c/18023/9/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@70 PS9, Line 70: ty=3.0 > nit. Since almost 98% values are less than '1998-09-02', we set Done http://gerrit.cloudera.org:8080/#/c/18023/9/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@95 PS9, Line 95: y, de > nice. Done http://gerrit.cloudera.org:8080/#/c/18023/9/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@344 PS9, Line 344: > nit. Delete this sentence and add "Here we assume ...". Done http://gerrit.cloudera.org:8080/#/c/18023/9/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@481 PS9, Line 481: > nit. predicate's Done http://gerrit.cloudera.org:8080/#/c/18023/9/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@643 PS9, Line 643: > nice! Done http://gerrit.cloudera.org:8080/#/c/18023/9/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@754 PS9, Line 754: > nit. Delete this sentence and add "Here we assume ...". Done http://gerrit.cloudera.org:8080/#/c/18023/9/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@780 PS9, Line 780: > Can you double check? It seems to me 3M should be the right number. Done http://gerrit.cloudera.org:8080/#/c/18023/9/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@822 PS9, Line 822: > nit. A simple example to show that selectivity hint can help change join mo Done http://gerrit.cloudera.org:8080/#/c/18023/9/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@826 PS9, Line 826: > nit. Impala can not compute selectivity and assumes the default value of 0. Done http://gerrit.cloudera.org:8080/#/c/18023/9/testdata/workloads/functional-planner/queries/PlannerTest/predicate-selectivity-hint.test@827 PS9, Line 827: > nit. becomes 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: 13 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: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: wangsheng <[email protected]> Gerrit-Comment-Date: Sat, 25 Feb 2023 04:03:55 +0000 Gerrit-HasComments: Yes
