Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14632 )
Change subject: IMPALA-9126: part 1: hash join build partition cleanup ...................................................................... Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/14632/4/be/src/exec/partitioned-hash-join-builder.h File be/src/exec/partitioned-hash-join-builder.h: http://gerrit.cloudera.org:8080/#/c/14632/4/be/src/exec/partitioned-hash-join-builder.h@119 PS4, Line 119: > nit: formatting Done http://gerrit.cloudera.org:8080/#/c/14632/4/be/src/exec/partitioned-hash-join-builder.h@124 PS4, Line 124: // We don't need to pass in a batch because the anti-join only returns tuple data > Is it possible to DCHECK for this? I guess it would be null_aware_partition I don't think there's a direct way to assert on it. It follows from the join node not including this tuple in its output row descriptor. I added that fact to the comment. Seems like an improvement since the reader can verify that fact (e.g. see the DCHECK in BlockingJoinNode::Prepare()). http://gerrit.cloudera.org:8080/#/c/14632/4/be/src/exec/partitioned-hash-join-builder.h@163 PS4, Line 163: /// Needs to be log2(PARTITION_FANOUT). > Might make sense to move this and MAX_PARTITION_DEPTH up too, to keep the r good point http://gerrit.cloudera.org:8080/#/c/14632/4/be/src/exec/partitioned-hash-join-builder.cc File be/src/exec/partitioned-hash-join-builder.cc: http://gerrit.cloudera.org:8080/#/c/14632/4/be/src/exec/partitioned-hash-join-builder.cc@483 PS4, Line 483: stream->Close(nullptr, RowBatch::FlushMode::NO_FLUSH_RESOURCES); > Maybe comment/DCHECK why we don't need to pass row_batch here Done http://gerrit.cloudera.org:8080/#/c/14632/4/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: http://gerrit.cloudera.org:8080/#/c/14632/4/be/src/exec/partitioned-hash-join-node.cc@339 PS4, Line 339: if (state_ == PROBING_SPILLED_PARTITION && NeedToProcessUnmatchedBuildRows(join_op_)) { > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/14632/4/be/src/exec/partitioned-hash-join-node.cc@1154 PS4, Line 1154: hash_tbl_iterator_ = output_build_partitions_.front()->hash_tbl()->FirstUnmatched(ht_ctx_.get()); > line too long (101 > 90) Done -- To view, visit http://gerrit.cloudera.org:8080/14632 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ife8d0fa5dd14c7d3f3d726dd38c07d8cbceabadb Gerrit-Change-Number: 14632 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 19 Nov 2019 21:55:00 +0000 Gerrit-HasComments: Yes