Lars Volker has posted comments on this change.

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

Patch Set 1:


Thanks for the reviews, please see PS2
File fe/src/main/java/org/apache/impala/analysis/

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: " + 
> list both conflicting hints (otherwise it's hard to tell how to fix the pro
File fe/src/main/java/org/apache/impala/analysis/

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

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
File fe/src/main/java/org/apache/impala/planner/

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

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 
> 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 
> createInsertFragment() is only called for distributed plans. Should cluster

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

Line 324:     // TODO Why is this needed here but not in If it 
is omitted, the query
> Before plan generation we call QueryStmt.materializeRequiredSlots() to make
File fe/src/main/java/org/apache/impala/planner/

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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I412153bd8435d792bd61dea268d7a3b884048f14
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-HasComments: Yes

Reply via email to