Thomas Tauber-Marshall 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: (4 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 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_->build_rows()->num_rows() == 0? Maybe unnecessarily defensive 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 related static const stuff all together. 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 -- 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 20:08:42 +0000 Gerrit-HasComments: Yes
