Dan Hecht has posted comments on this change. Change subject: IMPALA-4674: Part 1: port BufferedTupleStream to BufferPool ......................................................................
Patch Set 10: (19 comments) http://gerrit.cloudera.org:8080/#/c/5811/9/be/src/runtime/buffered-tuple-stream-v2.cc File be/src/runtime/buffered-tuple-stream-v2.cc: PS9, Line 322: !has_read_iterator() ? http://gerrit.cloudera.org:8080/#/c/5811/10/be/src/runtime/buffered-tuple-stream-v2.cc File be/src/runtime/buffered-tuple-stream-v2.cc: Line 101: for (const Page& page : pages_) { will this be too expensive, even in debug builds (and we should make sure this loop is compiled out of release builds)? PS10, Line 125: read_page_ == pages_.end() has_read_iterator() and switch clauses? Line 129: } does it make sense to print the write page as well? PS10, Line 149: // This must be the first call to PrepareForWrite(). Maybe say, this must be the first iterator? i.e. you can't call this function after starting a read iteration or read/write iteration, either, right? PS10, Line 158: // This must be the first call to PrepareForWrite(). same PS10, Line 205: if when (sorry, i wrote the comment but 'if' sounds confusing since it's not currently pinned) PS10, Line 211: If When PS10, Line 258: get_extra_reservation it's unfortunate we need this. i wonder if moving the reservation logic (and the ResetWritePage() call) to the callers would be simpler? In any case, if we leave it here, how about calling this parameter 'get_read_reservation'? also see next comment. PS10, Line 272: bool pin_for_read = has_read_iterator() && pinned_; : get_extra_reservation |= pin_for_read; this is still pretty confusing. if you don't move the reservation logic completely into the callers, it may still make sense to move the calculation 'pinned_ && has_read_iterator()' into AddRowSlow(). The other callsites are known to be either static true or false. THen you would also move the PinPage() call into the caller (using PinPageIfNeeded()), which seems to make sense since in the case of PrepareForReadWrite(), you really want PrepareForReadInternal() to do that anyway, no? Or at least you could move this after the write iterator is set up and still use PinPageIfNeeded(). Line 283: if (pin_for_read) RETURN_IF_ERROR(PinPage(&new_page)); see comment above. Line 298: int64_t row_size, bool get_extra_reservation, bool* got_page) noexcept { this wrapper now seems unnecessary and not helpful for readability because NewWritePage() has no other callers. PS10, Line 339: read_page_ == pages_.end() !has_read_iterator()? Line 379: ResetReadPage(); this seems to make more sense to do in PrepareForRead() since that's the only case you can possible have a read iterator, right? and that assumption is built into the reservation management code. http://gerrit.cloudera.org:8080/#/c/5811/10/be/src/runtime/buffered-tuple-stream-v2.h File be/src/runtime/buffered-tuple-stream-v2.h: PS10, Line 217: got_buffer maybe this should be 'got_reservation' now, and explained in terms of reservation. or at least got_page? PS10, Line 228: got_buffer same PS10, Line 496: get_extra_reservation this needs explanation, but first see comments in the cc file about it. Line 503: bool* got_page) noexcept WARN_UNUSED_RESULT; seems like we can put this code in NewWritePage() now. PS10, Line 509: got_buffer got_page? -- To view, visit http://gerrit.cloudera.org:8080/5811 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7bb47a818564ac19a6be4ca88db0578f4ea0b709 Gerrit-PatchSet: 10 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
