Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201610182215.u9IMFpJu029081@ip-10-146-233-104.ec2.internal>
Date: Tue, 18 Oct 2016 22:15:50 +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>, Marcel Kornacker <marcel@cloudera.com>
Reply-To: lv@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-2521=3A_Add_clustered_hint_to_insert_statements=0A?=
In-Reply-To: <gerrit.1476752309000.I412153bd8435d792bd61dea268d7a3b884048f14@gerrit.cloudera.org>
References: <gerrit.1476752309000.I412153bd8435d792bd61dea268d7a3b884048f14@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: Tue, 18 Oct 2016 22:15:57 -0000

Lars Volker has posted comments on this change.

Change subject: IMPALA-2521: Add clustered hint to insert statements
......................................................................


Patch Set 1:

(12 comments)

Thanks for the reviews, please see PS2

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

Line 121:   private boolean hasClusteredHint_ = false;
> might be better to use ternary logic here with a single Boolean (which can 
We actually only need the ternary logic to detect conflicting hints, so I made it a boolean and used another local variable in analyzePlanHints() to detect that error.


Line 631:         if (hasNoShuffleHint_) {
> did you mean noclustered?
Yes, done.


Line 632:           throw new AnalysisException("Conflicting INSERT hint: " + hint);
> list both conflicting hints (otherwise it's hard to tell how to fix the pro
Done


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

Line 68:    * sorted. Its source exprs are update to those in tupleSlotExprs.
> second sentence incomprehensible.
Done


Line 123:    * the exprs used by the SortNode refer to the materialized tuples instead of the
> don't refer to the sort node here, SortInfo shouldn't have to know about th
Done


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

Line 269:    * clustered/noclustered plan hint. The clustering re-order the data in 'inputFragment'
> Better to be specific: Say that it adds a sort node that orders on the part
Done


Line 272:   // TODO: this function shares some code with QueryStmt::createSortTupleInfo(). However
> You could move most of the code into SortInfo.createSortTupleInfo(List<Expr
Done, however I couldn't see how that signature would fit in, so now I'm passing the set of source slot refs instead. I also had to modify the StatementBase class to make the factored-out code work with both QueryStmt and InsertStmt objects.


Line 275:   public PlanFragment addClusteringFragment(PlanFragment inputFragment,
> you should move the logic (minus the createSortTupleInfo part that alex bro
createInsertFragment has several early exits (return inputFragment) and it isn't that clear to me, how adding this orthogonal functionality would fit in nicely. We need to support both cases, adding shuffling and/or clustering independently. Should I replace them with nested if-else-clauses?

For now I moved it to the Planner as suggested by Alex and enabled it for single-node-plans, too.


Line 275:   public PlanFragment addClusteringFragment(PlanFragment inputFragment,
> createInsertFragment() is only called for distributed plans. Should cluster
Done


Line 309:     sortTupleDesc.setIsMaterialized(true);
> To indicate that this tuple is a physical tuple needed during runtime execu
Done


Line 324:     // TODO Why is this needed here but not in QueryStmt.java? If it is omitted, the query
> Before plan generation we call QueryStmt.materializeRequiredSlots() to make
Done


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

Line 144:         rootFragment = distributedPlanner.addClusteringFragment(
> 1. we should also do this for single-node plans, so adding the clustering d
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4745
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I412153bd8435d792bd61dea268d7a3b884048f14
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-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes