[Impala-ASF-CR] IMPALA-7942: Add query hints for cardinalities and selectivities

["Quanlong Huang (Code Review)" <ge...@cloudera.org>]

Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18023 )

Change subject: IMPALA-7942: Add query hints for cardinalities and selectivities
......................................................................


Patch Set 9:

(7 comments)

This patch looks pretty good for me! I hope we can reduce the test files by not showing the verbose plan. It's a little large for me to go through all the test cases.

http://gerrit.cloudera.org:8080/#/c/18023/9//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/18023/9//COMMIT_MSG@14
PS9, Line 14: HDFS_NUM_ROWS
TABLE_NUM_ROWS?


http://gerrit.cloudera.org:8080/#/c/18023/9//COMMIT_MSG@15
PS9, Line 15: HDFS_NUM_ROWS
TABLE_NUM_ROWS?


http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/main/cup/sql-parser.cup@3772
PS9, Line 3772:     if (p instanceof Predicate) {
              :       if (hint != null) {
nit: "if (p instanceof Predicate && hint != null)"


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:       } else if (hint.is("TABLE_NUM_ROWS")) {
The above hints are all limited to hdfs tables since they are related to hdfs file/replica. But it's not clear to me why we can't support this hint for kudu tables.


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@5074
PS9, Line 5074:     AnalysisError("select * from t1 where (a>1 and b<1) /* +SELECTIVITY(0.1) */",
Isn't this supported now in patch set 9?


http://gerrit.cloudera.org:8080/#/c/18023/9/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/9/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@1294
PS9, Line 1294:     options.setExplain_level(TExplainLevel.VERBOSE);
Why do we need verbose plan? Is there anything else we want to check except cardinality?
I don't see this in similar tests: https://github.com/apache/impala/blob/c0b0875bda59771fb1b5c55a5eaf45f3dcfaa63c/fe/src/test/java/org/apache/impala/planner/PlannerTest.java#L72-L73


http://gerrit.cloudera.org:8080/#/c/18023/9/fe/src/test/java/org/apache/impala/planner/PlannerTest.java@1295
PS9, Line 1295:     runPlannerTestFile("hdfs-cardinality-hint", options);
I think we need PlannerTestOption.VALIDATE_CARDINALITY here.



-- 
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: 9
Gerrit-Owner: wangsheng <sk...@163.com>
Gerrit-Reviewer: Amogh Margoor <am...@gmail.com>
Gerrit-Reviewer: Fucun Chu <ch...@hotmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Tue, 02 Aug 2022 02:56:15 +0000
Gerrit-HasComments: Yes