Thomas Tauber-Marshall 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 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 rows for reading in other (non-empty_probe) cases, which I don't think is correct. 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 Reset() 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() 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 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 -- 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 19:36:52 +0000 Gerrit-HasComments: Yes
