impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-2521: Add clustered hint to insert statements
Date Wed, 19 Oct 2016 21:38:32 GMT
Lars Volker has posted comments on this change.

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

Patch Set 4:


Thanks for the comments, please see PS4. As pointed out in one of the replies I will look
at Kudu support next.
Commit Message:

Line 11: plan, just before the table sink. This has the effect that data will be
> -distributed (also adds to the single-node plan)
File fe/src/main/java/org/apache/impala/analysis/

Line 121:   private boolean hasClusteredHint_ = false;
> but how do you capture 'noclustered'? that's not the same as the absence of
Right now the default is 'noclustered' and is not captured explicitly. Once the default changes
to 'clustered' we can change the code to reflect that.
File fe/src/main/java/org/apache/impala/analysis/

Line 238:    */
> move this TODO into SortInfo.createSortTupleInfo()

Line 253:       Preconditions.checkState(smap.getLhs().get(i) instanceof SlotRef);
> these few lines can also be moved into createSortTupleInfo() right?
Excellent, I didn't see this.
File fe/src/main/java/org/apache/impala/analysis/

Line 125: 
> refer to the new tuple materialized by this sort instead of the original in

Line 145:   public SortInfo clone() { return new SortInfo(this); }
> shorter: by the sort node

Line 146: 
> comment about result expressions only makes sense if we pass in the result 
True, now it's aligned with the resultExprs actually being passed in.

Line 147:   /**
> we also do this for top-n

Line 152:    * sorts. The substitution map is returned.
> I'd suggest removing the 'stmt' parameter and instead returning the ExprEub

Line 153:    * TODO: We could do something more sophisticated than simply copying input slot
refs -
> Isn't this the tuple descriptor for the sort output? The secnd sentence is 
I think it is both, since the sort node operates only on these. However output seems to be
more intuitive, so I changed it.

Line 163: 
> builds up the tuple operated on and returned by sort node

Line 171:     // substOrderBy is the mapping from slot refs in the sort node's input to slot
refs in
> I think the else case may not be needed/possible, but the explanation is th
Yes, thanks for the explanation.

Line 173:     // the tuple operated on and returned by the sort node.
> Can we also move this to the caller? The returned smap can be used to creat

Line 181:       sortTupleExprs.add(origSlotRef);
> let's let the caller make this decision without passing a flag
File fe/src/main/java/org/apache/impala/planner/

Line 275:   private PlanFragment createScanFragment(PlanNode node) {
> never mind, the structure looks good as it is.
File fe/src/main/java/org/apache/impala/planner/

Line 149:       // Add optional sort node to the plan, based on clustered/noclustered plan
> Let's keep it concrete: we're adding a sort node

Line 151:       // set up table sink for root fragment
> there's no ClusteringNode. something like 'clusterInsertInput'/ 'createClus

Line 497:    * This will sort the data produced by 'inputFragment' by the clustering columns,
> Mention the ordering of the sort

Line 498:    * that partitions can be written sequentially in the table sink.
> Second sentence is kind of vague. I understand that sorting is just an inst

Line 503:     if (!insertStmt.hasClusteredHint()) return;
> single line (and elsewhere)

Line 508: 
> partitioning?
I took that comment from DistributedPlanner::createInsertFragment() and I thought it made
sense to remove all constants during the clustering. Changed the word.
File testdata/workloads/functional-planner/queries/PlannerTest/insert.test:

Line 573: # IMPALA-2521: clustered insert into partitioned table adds sort node.
> Let's test these combinations:

Line 575: select * from functional.alltypes
> Let's also test Kudu tables because there we want to sort on the PKs. Let's
I'll try to address this in a subsequent patchset, so we can make progress on the rest.

Line 576: ---- PLAN
> also add ---- PLAN section

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I412153bd8435d792bd61dea268d7a3b884048f14
Gerrit-PatchSet: 4
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

View raw message