wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18829 )
Change subject: IMPALA-7942 (part 1): Add query hints for table cardinalities ...................................................................... Patch Set 7: (10 comments) http://gerrit.cloudera.org:8080/#/c/18829/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18829/5//COMMIT_MSG@9 PS5, Line 9: Currently, we run 'COMPUTE STATS' command to compute table stats : which is very useful for query planning. Without these stats, a : query plan may not be optimal. However, these stats may not be : available, up to date, or valid. To workaround this problem, > nit. I guess 'valid' expresses the idea of 'invalid', due to the negation i Done http://gerrit.cloudera.org:8080/#/c/18829/5/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/18829/5/fe/src/main/java/org/apache/impala/analysis/TableRef.java@176 PS5, Line 176: tableNumRowsHint_ > yes. Yes, this situation would not throw exception or error according my test. http://gerrit.cloudera.org:8080/#/c/18829/6/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/18829/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5046 PS6, Line 5046: Syntax error in line 1 > Is it possible to include the invalid hint value in error message. In this This is a good idea, but not easy to achieve. Here are two mainly exception: 1. For '/* +TABLE_NUM_ROWS(aa) */', thrown NumberFormatException exception in 'TableRef.java', which means syntax parse success, but check failed in 'TableRef.java'; 2. For '/* +TABLE_NUM_ROWS(-1) */' or '/* +TABLE_NUM_ROWS(1.0) */', we get ParseException, which means syntax parse failed in 'sql-parser.cup'. These two test cases failed in different phase, main reason is that: plan_hint ::= KW_STRAIGHT_JOIN {: RESULT = new PlanHint("straight_join"); :} | IDENT:name {: RESULT = new PlanHint(name); :} | IDENT:name LPAREN ident_list:args RPAREN {: RESULT = new PlanHint(name, args); :} | IDENT:name LPAREN INTEGER_LITERAL:value RPAREN {: if (value != null) { RESULT = new PlanHint(name, new ArrayList(Arrays.asList(value.toString()))); } else { RESULT = new PlanHint(name); } :} | /* empty */ {: RESULT = null; :} ; ident_list ::= ident_or_default:ident {: List<String> list = new ArrayList<>(); list.add(ident); RESULT = list; :} | ident_list:list COMMA ident_or_default:ident {: list.add(ident); RESULT = list; :} ; As you can see, 'sql-parser.cup' define hint wrtie format. We can use string, string list or INTEGER_LITERAL as hint args after hint name. If we want to include the invalid hint value in error message, only two ways: 1. Implement error message in 'sql-parser.cup', this maybe not suitable, since we need to check each hint format in 'sql-parser.cup'; 2. Allow all kinds of args in 'sql-parser.cup', to ensure sql parse success, and then add hint format check in corresponding class, for example, 'TABLE_NUM_ROWS' check in 'TableRef.java'. In this way, we may need to modify 'ident_list' to allowed all kinds of type, currently is only 'ident_or_default' or 'INTEGER_LITERAL', I'm not sure this is good, since this change will influence other hint behavior. Do you have any suggestion? http://gerrit.cloudera.org:8080/#/c/18829/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@5056 PS6, Line 5056: // Cannot set cardinality hint without parameter : AnalyzesOk("select * from functional.alltypes /* +TABLE_NUM_ROWS */", > It does not appear these two belong to this section. Move them to positive Done http://gerrit.cloudera.org:8080/#/c/18829/6/testdata/workloads/functional-planner/queries/PlannerTest/table-cardinality-hint.test File testdata/workloads/functional-planner/queries/PlannerTest/table-cardinality-hint.test: http://gerrit.cloudera.org:8080/#/c/18829/6/testdata/workloads/functional-planner/queries/PlannerTest/table-cardinality-hint.test@1 PS6, Line 1: > you meant to say "and no hint is applied"? Done http://gerrit.cloudera.org:8080/#/c/18829/6/testdata/workloads/functional-planner/queries/PlannerTest/table-cardinality-hint.test@10 PS6, Line 10: d the > nit. "and the hint will be taken". Done http://gerrit.cloudera.org:8080/#/c/18829/6/testdata/workloads/functional-planner/queries/PlannerTest/table-cardinality-hint.test@28 PS6, Line 28: > nit. the hint value will be ignored. Done http://gerrit.cloudera.org:8080/#/c/18829/6/testdata/workloads/functional-planner/queries/PlannerTest/table-cardinality-hint.test@37 PS6, Line 37: the hint value will be > nit. the hint value will be ignored. Done http://gerrit.cloudera.org:8080/#/c/18829/6/testdata/workloads/functional-planner/queries/PlannerTest/table-cardinality-hint.test@46 PS6, Line 46: lue will be > nit. the hint value will be ignored. Done http://gerrit.cloudera.org:8080/#/c/18829/6/testdata/workloads/functional-planner/queries/PlannerTest/table-cardinality-hint.test@54 PS6, Line 54: the hint value will be > nit. the hint value will be ignored. Done -- To view, visit http://gerrit.cloudera.org:8080/18829 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f0c773f4e67782a1428db64062f68afbd257af7 Gerrit-Change-Number: 18829 Gerrit-PatchSet: 7 Gerrit-Owner: wangsheng <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[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: wangsheng <[email protected]> Gerrit-Comment-Date: Thu, 15 Sep 2022 07:31:30 +0000 Gerrit-HasComments: Yes
