Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3567 Part 2, IMPALA-3899: factor out PHJ builder

Patch Set 14:


File be/src/exec/blocking-join-node.cc:

PS14, Line 297: SCOPED_TIMER(build_timer_);
> I am mostly concerned about preserving "BuildPartitionTime" and "BuildTime"
Ah, you're right, the older code for some reason didn't include the 
partitioning time in BuildTime. This doesn't make a lot of sense to me, since 
build_timer_ is a BlockingJoinNode-level concept, where we shouldn't care about 
the implementation details of the build phase. With the old code, I don't think 
someone would guess that meaning of "BuildTime" without looking at the code.

I think the new decomposition of BuildTime covering the whole initial build 
phase, then BuildRowsPartitionTime and HashTablesBuildTime covering the time 
spent in those operations is more intuitive.

I guess it could cause some confusion comparing profiles from different 
versions, but I think it would be better to clean things up here instead of 
trying to preserve the profile counters exactly. The profile layout is changing 
anyway with the patch so probably a good time to do it.

File be/src/exec/partitioned-hash-join-builder.cc:

PS19, Line 377: ;
> if

File be/src/exec/partitioned-hash-join-node.cc:

Line 77:   const vector<TEqJoinCondition>& eq_join_conjuncts =
> It helps to add a comment like:
I added a different comment with the same intent - just pointing out that 
allocating PhjBuidler like this is a temporary measure.

PS19, Line 479:   while (true) {
              :     DCHECK(status.ok());
              :     DCHECK_NE(state_, PARTITIONING_BUILD) << "Should not be in 
              :     RETURN_IF_CANCELLED(state);
              :     RETURN_IF_ERROR(QueryMaintenance(state));
              :     if (!output_build_partitions_.empty()) {
> It seems unfortunate that we cannot remove this given the change at line 58
Exactly, it's necessary for that case.

My feeling is that we shouldn't try to refactor this loop piecemeal, rather we 
should just figure out the right abstraction/organisation (maybe a state 
machine plus some logical decomposition into functions?) and completely clean 
it up. Not in this patch though.

Line 554:       if (out_batch->AtCapacity() || ReachedLimit()) break;
> DCHECK(input_partition_ == NULL);

PS19, Line 599:     if (got_partition) continue; // Probe the spilled partition.
> Not your change but I find this duplicated check with line 501 a bit confus
Done. Should be equivalent, since the null_aware_partition is only non-NULL for 
NAAJ, and there are DCHECKs in PrepareNullAwarePartition() that both this 
partitions are non-NULL (it's not reachable otherwise because of the checks up 
the top of the loop).

PS19, Line 724: 
> I suppose it's good to avoid small buffers on all probe partitions for cons
Yes potentially, it could increase it by up to 16MB but I think it's worth 
accepting that increase for a rarely-used join type in order to avoid the 
possibility of spilling while probing.

Line 740:       state, this, builder_->null_aware_partition(), 
> Is there an error path in which probe_rows == NULL ? Looks like line 725 im
Should be ok to remove it.

PS19, Line 751: 
> Same question as above.
See response above.

PS19, Line 968: ore
> nit: newly created

File be/src/util/runtime-profile.h:

PS19, Line 129: AddChildFirst
> Feel free to ignore but PrependChild() seems more appropriate.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e02ea9c7a7d1a0f373b11aa06c3237e1c7bd4cb
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to