Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4163: Add sortby() query hint ......................................................................
Patch Set 3: (18 comments) 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? Line 2397: | /* empty */ what's the point of this? 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? Line 774: private void analyzeSortByColumns(List<String> columnNames) throws AnalysisException { use line breaks where it makes sense. also, describe the rules for sort-by columns somewhere. analyzeSortByHint is more general (and correct) Line 787: } else { you'd also take this branch for hbase 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. Line 62: public String toString() { toSql test missing 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. 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 Line 515: * will also sort by all columns specified in the sortby() hint. rewrite comment, instead of just tacking on 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 " "*? Line 336: ContainsCommentEnd = [^]* "*/" [^]* what is [^]*? shouldn't 'anything' be simply ".*"? Line 342: TraditionalComment = "/*" !({HintContent}|{ContainsCommentEnd}) "*/" why do you need the ContainsCommentEnd here? wouldn't /* */ */ simply be a parse error? Line 434: yybegin(HINT); shouldn't this only apply to end-of-line hints? 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 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: /* +sortby()\n and -- +sortby() */ \n Line 477: "insert into t %sSoRtBy( a , , ,,, b )%s select * from t", prefix, suffix)); is the capitalization necessary? 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 -- 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
