Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15612 )
Change subject: IMPALA-9176: shared null-aware anti-join build ...................................................................... Patch Set 10: Code-Review+1 (9 comments) Looks good, only a few nits. A +2 from me but I'll let Csaba +2 in case he wants to take a look. 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: Either 'hash_table' or 'build_row_ptrs' must be provided nit: is this an outdated comment? 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@880 PS10, Line 880: builder_->null_aware_partition()->build_rows()->num_rows() == 0 although there is a dcheck in builder_->BeginNullAwareProbe() for checking builder_->null_aware_partition()!=nullptr, but i feel it might be worth one before this line too, otherwise this might result in a seg-fault before we hit that other dcheck. http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/exec/partitioned-hash-join-node.cc@941 PS10, Line 941: build_it super nit: how about build_itr ? here and L1097 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: /// To use read/write mode, PrepareForReadWrite() is called once to initialize the read : /// and write iterators. AddRow()/AddRowCustom*() then advance a write iterator through : /// the stream, and GetNext() advances a trailing read iterator through the stream. nit: update comment http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/runtime/buffered-tuple-stream.h@267 PS10, Line 267: Status PrepareForPinnedRead(ReadIterator* iter); nit: might be useful to collate all methods that can only be called on external iterator and maybe DCHECK that internal read_itr_ exists or does not exist appropriately. http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/runtime/buffered-tuple-stream.h@451 PS10, Line 451: be nit: extra word http://gerrit.cloudera.org:8080/#/c/15612/10/be/src/runtime/buffered-tuple-stream.h@511 PS10, Line 511: pages_.end() maybe add a dcheck? 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: BufferHandle nit: seems like this will also be used by multiple threads along with the shared pageHandle. Maybe mention that this should be used in read-only mode if accessed by multiple threads. 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. Each PageHandle should : /// only be used by a single thread at a time: concurrently calling PageHandle methods : /// or BufferPool methods with the PageHandle as an argument is not supported. nit: update. -- 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: 10 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-Comment-Date: Wed, 08 Apr 2020 06:19:45 +0000 Gerrit-HasComments: Yes
