Lars Volker has posted comments on this change. Change subject: IMPALA-4163: Add sortby() query hint ......................................................................
Patch Set 3: (18 comments) Thanks for the review. Please see PS4. http://gerrit.cloudera.org:8080/#/c/5051/3/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: Line 370: nonterminal List<PlanHint> opt_plan_hints, plan_hint_list; > do you really need both? why not have a non-existing hint = empty hint list I'm not sure I understood your comment. I think we need three separate classes here, since we need to 1) remove/handle the opening and closing comment characters ("/* */" and "-- \n" respectively), 2) be able to parse a list of uniform tokens, and 3) parse the individual hints. Together with the quirk that "straight_join" may exists as a single keyword outside them all I couldn't find a way to get rid of either. Do you have a suggestion? Line 2397: | /* empty */ > what's the point of this? We support empty hint tokens, tested in ParserTest.java#L512 . Should we remove that support? If so, I'd suggest opening a Jira to track removal with the next compat-breaking release. http://gerrit.cloudera.org:8080/#/c/5051/3/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: Line 127: // Contains the result exprs corresponding to the columns of the target table, that are > what do you mean by result exprs? are these basetbl exprs? It contains the same exprs as resultExprs_ below. I added that to the comment. Line 774: private void analyzeSortByColumns(List<String> columnNames) throws AnalysisException { > use line breaks where it makes sense. Added line breaks, described rules in the field comment above, renamed the method. Line 787: } else { > you'd also take this branch for hbase hbase tables don't support insert hints, which is enforced in L740. I added a comment. http://gerrit.cloudera.org:8080/#/c/5051/3/fe/src/main/java/org/apache/impala/analysis/PlanHint.java File fe/src/main/java/org/apache/impala/analysis/PlanHint.java: Line 34: private final String hintName_; > remove 'hint' from members, this class already contains 'hint' in its name. Done Line 62: public String toString() { > toSql test missing ToSqlTest.java already tests plan hints. I added a sortby() hint there, to make sure that the parentheses also get printed as expected. http://gerrit.cloudera.org:8080/#/c/5051/3/fe/src/main/java/org/apache/impala/analysis/TableRef.java File fe/src/main/java/org/apache/impala/analysis/TableRef.java: Line 87: // ArrayList initialization each. > yes, please do. Done http://gerrit.cloudera.org:8080/#/c/5051/3/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: PS3, Line 511: clustered/noclustered plan : * hint and the sortby() > "on the clustered/noclustered/sortby plan hint" is shorter Done Line 515: * will also sort by all columns specified in the sortby() hint. > rewrite comment, instead of just tacking on Done http://gerrit.cloudera.org:8080/#/c/5051/3/fe/src/main/jflex/sql-scanner.flex File fe/src/main/jflex/sql-scanner.flex: Line 331: HintContent = [ ]* "+" [^\r\n*]* > isn't [ ]* the same as " "*? Yes, they are the same. I went with [ ]* because it was like that before. Should I change it to " "*? Line 336: ContainsCommentEnd = [^]* "*/" [^]* > what is [^]*? No. The dot does not match newlines. From: http://jflex.de/manual.html Dot (.) matches [^\r\n\u2028\u2029\u000B\u000C\u0085]. Line 342: TraditionalComment = "/*" !({HintContent}|{ContainsCommentEnd}) "*/" > why do you need the ContainsCommentEnd here? wouldn't /* */ */ simply be a Jflex greedily processes the longest match it can find for any regular expression. Thus it would strip /* */ */ as a single comment and accept it, instead of it being a parser error. Similarly, without this clause, multiple query hints spanning multiple lines will be discarded as a comment: select * from T join U /* +shuffle */ \n join V /* +broadcast */... will discard all hints as one comment (since !{HintContent} doesn't apply, due to the newline. Line 434: yybegin(HINT); > shouldn't this only apply to end-of-line hints? Done http://gerrit.cloudera.org:8080/#/c/5051/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: Line 1776: "partition (year, month) %sshuffle,clustered,sortby(int_col,bool_col)%s " + > mix up the order a bit Done http://gerrit.cloudera.org:8080/#/c/5051/3/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: Line 216: ParserError("select 1 /*+ sortby(() */"); > also check that this is illegal: Done Line 477: "insert into t %sSoRtBy( a , , ,,, b )%s select * from t", prefix, suffix)); > is the capitalization necessary? No, removed it. http://gerrit.cloudera.org:8080/#/c/5051/3/testdata/workloads/functional-planner/queries/PlannerTest/insert.test File testdata/workloads/functional-planner/queries/PlannerTest/insert.test: Line 758: /*+ clustered,sortby(int_col, bool_col) */ > move clustered to end Done -- To view, visit http://gerrit.cloudera.org:8080/5051 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I37a3ffab99aaa5d5a4fd1ac674b3e8b394a3c4c0 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
