impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3567 Part 2, IMPALA-3899: factor out PHJ builder
Date Fri, 26 Aug 2016 23:48:01 GMT
Alex Behm has posted comments on this change.

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


Patch Set 10:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/3873/10/be/src/exec/partitioned-hash-join-builder.h
File be/src/exec/partitioned-hash-join-builder.h:

Line 45: /// The builder owns the hash tables and build row partitions. The initial partitioning
Let's follow your suggestion of finishing level0 completely in this builder and handing off
the allocated probe buffers to the join node. That seems pretty clean to me.


http://gerrit.cloudera.org:8080/#/c/3873/9/be/src/exec/partitioned-hash-join-node.cc
File be/src/exec/partitioned-hash-join-node.cc:

Line 865: Status PartitionedHashJoinNode::BuildHashTablesAndPrepareProbePartitions() {
> I agree that should be the end-goal.
I like your idea of allocating probe blocks in the builder, that way we can finish level0
completely in the builder and the handoff is clean.


Line 959:   }
> Yeah, if we know how much memory we have to work with we can precompute the
Agree.


http://gerrit.cloudera.org:8080/#/c/3873/10/be/src/exec/partitioned-hash-join-node.cc
File be/src/exec/partitioned-hash-join-node.cc:

Line 150:     builder_->Codegen(&build_codegen_status, &insert_codegen_status);
Shouldn't this be done in builder_->Prepare() or similar?

How would codegen work if the builder is executed in a separate fragment?

Or are you planning a follow-on patch to enable running the builder in a separate fragment?


http://gerrit.cloudera.org:8080/#/c/3873/10/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:

Line 408:   /// Same as AddChild, except 'children_lock_' must already be held.
AddChild()


-- 
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: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message