Tim Armstrong 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: (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 logi Yeah this was a remnant of a different change I was working on. Fixed 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 Done 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 ag Yep, not sure what I was thinking here exactly. We already had a check in PrepareForReadInternal() that should ensure this, but it makes sense to have here too. 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. Done -- 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 23:47:29 +0000 Gerrit-HasComments: Yes
