Tim Armstrong 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() ? Done 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 t We were already doing a pass for the list in one of the Calc* methods, so it's not likely to be significantly worse. I removed the consistency check from GetNextInternal(), which is likely to be the most commonly-called. I added a CHECK_CONSISTENCY macro to guarantee the calls get removed in release builds. PS10, Line 125: read_page_ == pages_.end() > has_read_iterator() and switch clauses? Done Line 129: } > does it make sense to print the write page as well? It's printed on line 123 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 functi Done PS10, Line 158: // This must be the first call to PrepareForWrite(). > same Done PS10, Line 205: if > when Done PS10, Line 211: If > When Done PS10, Line 258: get_extra_reservation > it's unfortunate we need this. i wonder if moving the reservation logic (an Done 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 com I restructured this code and move the reservation logic into the callers. It adds a bit more boilerplate but seems less magical. Line 283: if (pin_for_read) RETURN_IF_ERROR(PinPage(&new_page)); > see comment above. Done 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 Done PS10, Line 339: read_page_ == pages_.end() > !has_read_iterator()? Done Line 379: ResetReadPage(); > this seems to make more sense to do in PrepareForRead() since that's the on Done 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 reser Works for me - done. PS10, Line 228: got_buffer > same Done PS10, Line 496: get_extra_reservation > this needs explanation, but first see comments in the cc file about it. Reworked this. Line 503: bool* got_page) noexcept WARN_UNUSED_RESULT; > seems like we can put this code in NewWritePage() now. This ended up being restructured a bit - some of the code is in a new function AdvanceWritePage(). PS10, Line 509: got_buffer > got_page? Done -- 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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
