Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14859 )
Change subject: IMPALA-4224: execute separate join builds fragments ...................................................................... Patch Set 44: (6 comments) PS44 was a rebase, PS43 addressed comments http://gerrit.cloudera.org:8080/#/c/14859/42/be/src/exec/blocking-join-node.h File be/src/exec/blocking-join-node.h: http://gerrit.cloudera.org:8080/#/c/14859/42/be/src/exec/blocking-join-node.h@159 PS42, Line 159: /// Acquire resources for this ExecNode required for the build phase. : /// Called by BlockingJoinNode after opening child(1) succeeds and before > update comment to accommodate separate builder case. Done http://gerrit.cloudera.org:8080/#/c/14859/42/be/src/exec/partitioned-hash-join-builder.h File be/src/exec/partitioned-hash-join-builder.h: http://gerrit.cloudera.org:8080/#/c/14859/42/be/src/exec/partitioned-hash-join-builder.h@292 PS42, Line 292: > nit: might be worth mentioning here and in the method below, that the probe Done http://gerrit.cloudera.org:8080/#/c/14859/42/be/src/exec/partitioned-hash-join-builder.h@570 PS42, Line 570: Tr > nit: to state? I tried rewording a different way. The original phrasing made sense to me TBH. http://gerrit.cloudera.org:8080/#/c/14859/42/be/src/exec/partitioned-hash-join-builder.cc File be/src/exec/partitioned-hash-join-builder.cc: http://gerrit.cloudera.org:8080/#/c/14859/42/be/src/exec/partitioned-hash-join-builder.cc@607 PS42, Line 607: int64_t saved_reservation = probe_stream_reservation_.GetReservation(); : DCHECK_GE(saved_reservation, probe_reservation); > nit: no need for this, instead add a comment in CalcProbeStreamReservation Done http://gerrit.cloudera.org:8080/#/c/14859/42/be/src/exec/partitioned-hash-join-builder.cc@623 PS42, Line 623: need_probe_buffer; > nit: add comment: thanks for the suggestion http://gerrit.cloudera.org:8080/#/c/14859/42/be/src/exec/partitioned-hash-join-builder.cc@955 PS42, Line 955: Status status = : probe_client->TransferReservationTo(buffer_pool_client_, bytes, &success); > should we dcheck here whether we got all the memory that was lent to the pr I think it's messy to do it, so might not be worth doing (there isn't a particularly obvious invariant). * asserting on buffer_pool_client_.GetReservation() is messy for the shared build, because you may not have gotten back all of the reservations yet * If we're checking that 'bytes' is at least the probe servation, then the error case in PHJNode::Close() may be problematic. It's hard to reason through what that might be in the various error cases. -- To view, visit http://gerrit.cloudera.org:8080/14859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4403c8e62d9c13854e7830602ee613f8efc80c58 Gerrit-Change-Number: 14859 Gerrit-PatchSet: 44 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 19 Feb 2020 21:26:35 +0000 Gerrit-HasComments: Yes
