impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4674: Part 1: port BufferedTupleStream to BufferPool
Date Wed, 15 Mar 2017 23:08:40 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4674: Part 1: port BufferedTupleStream to BufferPool

Patch Set 10:

File be/src/runtime/

PS9, Line 322: 
> !has_read_iterator() ?
File be/src/runtime/

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?

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

PS10, Line 158:   // This must be the first call to PrepareForWrite().
> same

PS10, Line 205: if
> when

PS10, Line 211: If
> When

PS10, Line 258: get_extra_reservation
> it's unfortunate we need this. i wonder if moving the reservation logic (an

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.

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 

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 on
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

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?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7bb47a818564ac19a6be4ca88db0578f4ea0b709
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message