impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4163: Add sortby() query hint
Date Wed, 14 Dec 2016 18:49:49 GMT
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 <lv@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message