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: (10 comments) http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/exec/partitioned-hash-join-node.h File be/src/exec/partitioned-hash-join-node.h: http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/exec/partitioned-hash-join-node.h@465 PS10, Line 465: 'build' must be pinned. > nit: is this an outdated comment? Oops, yep, that was from an earlier failed attempt to implement this. http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/exec/partitioned-hash-join-node.cc@875 PS10, Line 875: DCHECK(builder_->null_aware_partition() != nullptr); I went through and replaced all the instances of NULL in this file while I'm here, isntead of continuing to do it piecemeal http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/exec/partitioned-hash-join-node.cc@880 PS10, Line 880: eredTupleStream* probe_stream = null_aware_probe_partition_->pr > although there is a dcheck in builder_->BeginNullAwareProbe() for checking Done http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/exec/partitioned-hash-join-node.cc@941 PS10, Line 941: d()); > super nit: how about build_itr ? here and L1097 Done http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/runtime/buffered-tuple-stream.h File be/src/runtime/buffered-tuple-stream.h: http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/runtime/buffered-tuple-stream.h@62 PS10, Line 62: /// PrepareForPinnedRead(). Multiple external read iterators can be active at the same : /// time, reading from different positions in the stream. External read iterators are : /// thread-safe, which allows safe concurrent access to the stream from different t > nit: update comment Done http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/runtime/buffered-tuple-stream.h@267 PS10, Line 267: /// iterator and stream must be pinned. This does not attach pages on read. > nit: might be useful to collate all methods that can only be called on exte It is valid to use the built-in read iterator at the same time as an external iterator (the unit test exercises this). I think I am actually already validating all the preconditions, i.e. that it's pinned, that there's no write iterator and that attach_on_read_ is false, between PrepareForPinnedRead(), PrepareForReadInternal(), GetNext() and CheckConsistencyFast(). It is a little scattered, but I couldn't see any additional checks to add. http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/runtime/buffered-tuple-stream.h@451 PS10, Line 451: > nit: extra word Done http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/runtime/buffered-tuple-stream.h@511 PS10, Line 511: > maybe add a dcheck? Did this. It's a little annoying that I have to pass in pages_, but not a big deal. http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/runtime/bufferpool/buffer-pool.h File be/src/runtime/bufferpool/buffer-pool.h: http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/runtime/bufferpool/buffer-pool.h@448 PS10, Line 448: operations > nit: seems like this will also be used by multiple threads along with the s Good catch.. http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/runtime/bufferpool/buffer-pool.h@511 PS10, Line 511: : /// The handle for a page used by clients of the BufferPool. Only const methods on : /// PageHandle are thread-safe. It is not safe to call non-constant PageHandle > nit: update. Good catch. -- 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, 09 Apr 2020 19:36:58 +0000 Gerrit-HasComments: Yes
