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 <[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: Tue, 02 Aug 2022 02:56:15 +0000 Gerrit-HasComments: Yes
