Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15612 )
Change subject: IMPALA-9176: shared null-aware anti-join build ...................................................................... Patch Set 12: Code-Review+1 (4 comments) http://gerrit.cloudera.org:8080/#/c/15612/12/be/src/exec/partitioned-hash-join-builder.cc File be/src/exec/partitioned-hash-join-builder.cc: http://gerrit.cloudera.org:8080/#/c/15612/12/be/src/exec/partitioned-hash-join-builder.cc@838 PS12, Line 838: build_rows->byte_size() + sizeof(void*) * build_rows->num_rows() Is this different than BufferedTupleStream::byte_size()? It seems more logical to me to calculate the memory requirement of PinStreamin BufferedTupleStream http://gerrit.cloudera.org:8080/#/c/15612/12/be/src/runtime/buffered-tuple-stream-test.cc File be/src/runtime/buffered-tuple-stream-test.cc: http://gerrit.cloudera.org:8080/#/c/15612/12/be/src/runtime/buffered-tuple-stream-test.cc@1414 PS12, Line 1414: // Invalidate the write iterator explicitly so that we can read concurrently. nit: +1 indent http://gerrit.cloudera.org:8080/#/c/15612/12/be/src/runtime/buffered-tuple-stream.cc File be/src/runtime/buffered-tuple-stream.cc: http://gerrit.cloudera.org:8080/#/c/15612/12/be/src/runtime/buffered-tuple-stream.cc@1081 PS12, Line 1081: // 'attach_on_read_' cannot be set back to false. Shouldn't we add a DCHECK for this? Or there are places where we call it again with false? http://gerrit.cloudera.org:8080/#/c/15612/12/be/src/runtime/buffered-tuple-stream.cc@1117 PS12, Line 1117: { nit: do we close this brace somewhere? I couldn't find where. -- To view, visit http://gerrit.cloudera.org:8080/15612 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I95ead761430b0aa59a4fb2e7848e47d1bf73c1c9 Gerrit-Change-Number: 15612 Gerrit-PatchSet: 12 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-Comment-Date: Thu, 23 Apr 2020 16:16:19 +0000 Gerrit-HasComments: Yes
