Tim Armstrong has posted comments on this change. Change subject: IMPALA-3567 Part 2, IMPALA-3899: factor out PHJ builder ......................................................................
Patch Set 12: (5 comments) http://gerrit.cloudera.org:8080/#/c/3873/12/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 builder partitions > remove last "partitions" Done Line 46: /// first partitions the build rows and builds hash tables for in-memory partitions, > I think it's worth going into more detail here: My understanding is that th Done Line 47: /// driven by the DataSink interface methods. After FlushFinal() is called, the join > the join node drives level1-N re-partitioning, calling into the builder for Done, but worded quite differently. Line 84: /// partitions in FlushFinal(). > are they indexed by partition index? They're all identical so there's no need to make that distinction. Clarified the comment a bit that they're empty and that there's one per spilled partition. I think the new comment implies strongly enough that they are non-NULL and therefore not indexed by partition - let me know if you think it needs more explanation. Line 303: void Codegen(RuntimeState* state); > Can codegen not fail? Should return a Status? It can, but that's handled inside of the function. Elsewhere we use a pattern where a Status is returned from Codegen*() and logged in Prepare() but this makes more sense to me. -- 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: 12 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
