Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201611141850.uAEIoRHA032005@ip-10-146-233-104.ec2.internal>
Date: Mon, 14 Nov 2016 18:50:27 +0000
From: "Lars Volker (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Alex Behm <alex.behm@cloudera.com>
Reply-To: lv@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-4163=3A_Add_sortby=28=29_query_hint=0A?=
In-Reply-To: <gerrit.1478859315000.I37a3ffab99aaa5d5a4fd1ac674b3e8b394a3c4c0@gerrit.cloudera.org>
References: <gerrit.1478859315000.I37a3ffab99aaa5d5a4fd1ac674b3e8b394a3c4c0@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.2
archived-at: Mon, 14 Nov 2016 18:50:33 -0000

Lars Volker has posted comments on this change.

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


Patch Set 1:

(14 comments)

Thank you for the review. Please see my comments and PS2.

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 w
So far it was. However, legacy hints [foo,bar] are handled by the parser, and changing those to allow hints with arguments seems to much trouble.

Let's work on moving hint parsing into the parser and see whether it is easier to allow legacy hints afterwards.


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 column
Done


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 ...
Done


Line 34: // TODO: Should we make this class a proper ParseNode?
> Don't think so, unless it's really directly produced by the parser.
Ok, I'll keep this TODO for now until we have decided how to parse plan hints.


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 inv
I'm afraid I'm not following completely. The options I understood are:
- Move parsing of plan hints to the lexer/parser
- Split out a completely new plan hint parser based on CUP
- Use regular expressions below to simplify the code, but keep parsing code within this class

Which one should we try first? I have already tried option 1 (lexer/parser) last week and couldn't get it to work easily. Eventually I gave up and wrote the code below to make progress on the overall change. I'll happily try again if you're confident it should work.


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


Line 46:     ArrayList<PlanHint> hints = Lists.newArrayList();
> This code looks a little scary. Could it be simplified with a regex? I unde
See my comment above. Which way should we try out first?


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


Line 113:   public boolean is(String s) { return hintName_.equals(s.toLowerCase()); }
> equalsIgnoreCase
I've used toLowerCase during PlanHint creation, hoping that storing all hints and args in lower case by convention would make the code easier to reason about. Especially comparing the hintArgs_ list to that of another PlanHint becomes easier since we can use hintArgs_.equals(). Should I lift that restriction?

I also added comments to the member variables explaining that we store them in lowercase.


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
Done


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?
We hadn't so far. I added them to the change for IMPALA-2523 and will eventually rebase this one.


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


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


Line 1786:           "SORTBY hint column list must not contain Kudu primary key columns.");
> mention the offending column
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: 1
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-HasComments: Yes