impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4163: Add sortby() query hint
Date Fri, 11 Nov 2016 21:23:31 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-4163: Add sortby() query hint
......................................................................


Patch Set 1:

(15 comments)

Add planner and end-to-end tests

http://gerrit.cloudera.org:8080/#/c/5051/1/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

Line 2248:   {: RESULT = PlanHint.parsePlanHintsString(l); :}
Isn't it easier overall (testing, etc.) to allow the legacy hint style as well? I though that's
what we settled on.


http://gerrit.cloudera.org:8080/#/c/5051/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java:

Line 128:   // that will be written into the columns referenced in that hint. The list is
populated
The comment says it but maybe more explicit that the hint references columns of the target
table (and not e.g., the column labels of the feeding select stmt)


http://gerrit.cloudera.org:8080/#/c/5051/1/fe/src/main/java/org/apache/impala/analysis/PlanHint.java
File fe/src/main/java/org/apache/impala/analysis/PlanHint.java:

Line 32:  * query statements. Plan consist of a name and an optional list of arguments.
A hint consists of ...


Line 34: // TODO: Should we make this class a proper ParseNode?
Don't think so, unless it's really directly produced by the parser.


Line 42:   /// TODO: This is code that parses parts of the query (the sortby hint). It would
be
Agree. We'll need to change the lexer as well. Might be a tricky. Let's investigate this option
together.

We could also generate a new plan hint parser, but let's first try the regex solution. Seems
like that should be easy and readable enough.


Line 45:       throws AnalysisException {
weird to throw an AnalysisException from the parser, consider just throwing Exception


Line 46:     ArrayList<PlanHint> hints = Lists.newArrayList();
This code looks a little scary. Could it be simplified with a regex? I understand the error
reporting may not be as nice as with this custom solution, but I think code simplicity may
trump that in this case.


Line 112:   /// Check wether this hint equals to a given string, ignoring case.
typo: whether


Line 113:   public boolean is(String s) { return hintName_.equals(s.toLowerCase()); }
equalsIgnoreCase


http://gerrit.cloudera.org:8080/#/c/5051/1/fe/src/main/java/org/apache/impala/planner/Planner.java
File fe/src/main/java/org/apache/impala/planner/Planner.java:

Line 495:    * Insert a sort node on top of the plan, depending on the clustered/noclustered
plan
update comment


Line 510:     orderingExprs.addAll(insertStmt.getSortByExprs());
nice!


http://gerrit.cloudera.org:8080/#/c/5051/1/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java:

Line 1723:   public void TestInsertHints() throws AnalysisException {
don't we test the clustered hint here?


Line 1777:           prefix, suffix), "Could not find SORTBY hint column foo in table.");
in target table

but 'foo' in single quotes


Line 1782:           "SORTBY hint column list must not contain Hdfs partition columns.");
mention the offending column name


Line 1786:           "SORTBY hint column list must not contain Kudu primary key columns.");
mention the offending column


-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message