Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14787 )
Change subject: IMPALA-9126: part 3: move more logic to PhjBuilder ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/14787/5/be/src/exec/partitioned-hash-join-builder.h File be/src/exec/partitioned-hash-join-builder.h: http://gerrit.cloudera.org:8080/#/c/14787/5/be/src/exec/partitioned-hash-join-builder.h@183 PS5, Line 183: ) > typo: I think you intended the ')' to go after true Done http://gerrit.cloudera.org:8080/#/c/14787/5/be/src/exec/partitioned-hash-join-builder.h@185 PS5, Line 185: instead prepares build rows for reading > This confused me a bit, as it makes it sound like we don't prepare build ro I thought about this and realised that this isn't really part of the visible behaviour of BeginSpilledProbe(), it's just a precursor to returning the partition in DoneProbingSinglePartition(). So I removed this paragraph and augmented the other comment. http://gerrit.cloudera.org:8080/#/c/14787/5/be/src/exec/partitioned-hash-join-builder.h@502 PS5, Line 502: HashJoinState state_ = HashJoinState::PARTITIONING_BUILD; > Might want to put this in the section below noting things that need to be R Done http://gerrit.cloudera.org:8080/#/c/14787/5/be/src/exec/partitioned-hash-join-builder.h@524 PS5, Line 524: /// Set in FlushFinal() and constant thereafter. > Also gets Reset() Good point. Done http://gerrit.cloudera.org:8080/#/c/14787/5/be/src/exec/partitioned-hash-join-builder.cc File be/src/exec/partitioned-hash-join-builder.cc: http://gerrit.cloudera.org:8080/#/c/14787/5/be/src/exec/partitioned-hash-join-builder.cc@68 PS5, Line 68: partitions_created_(nullptr), > Maybe do all of these in the header instead Done http://gerrit.cloudera.org:8080/#/c/14787/5/be/src/exec/partitioned-hash-join-builder.cc@496 PS5, Line 496: state_ == HashJoinState::PARTITIONING_PROBE ? 0 : 1 > Brief comment what the extra buffer is for Done -- To view, visit http://gerrit.cloudera.org:8080/14787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e233468de1eeae86651ab96df207de19e091053 Gerrit-Change-Number: 14787 Gerrit-PatchSet: 5 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: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 27 Nov 2019 21:51:46 +0000 Gerrit-HasComments: Yes
