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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
