impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
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:

(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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message