impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5085: large rows in BufferedTupleStreamV2
Date Sat, 27 May 2017 01:16:09 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-5085: large rows in BufferedTupleStreamV2
......................................................................


Patch Set 5:

(7 comments)

Want to think more about the Allocate/AddRow stuff, but here's comments for the other changes.

http://gerrit.cloudera.org:8080/#/c/6638/5/be/src/runtime/buffered-tuple-stream-v2.cc
File be/src/runtime/buffered-tuple-stream-v2.cc:

PS5, Line 118: (a tricky edge case).
how so?
oh, reading further I see this is explained by ResetWritePage(). let's say "See ResetWritePage()."


Line 340:     write_page_ = nullptr;
this case occurs because we eagerly create write pages. Did you consider making it so we create
write pages lazily instead (and then we always know the right page size so we will never create
empty pages)? Maybe that complicates the read/write case, though, but maybe not.


Line 378:   // enough reservation by deleting or unpinning the previous page.
this comment seems misplaced now, seems better attached to 390.


PS5, Line 380: !buffer_pool_client_->IncreaseReservationToFit(read_page_->len()
I don't understand why we do that but then the comment below says "The client is responsible
for ensuring the reservation is available".  Seems to be contradictory.


PS5, Line 421: pages_.empty()
why is this needed? won't we have at least one page? oh, I guess in the case of non-read-write
we might end up with no pages due to the logic added in ResetWritePage()?


http://gerrit.cloudera.org:8080/#/c/6638/3/be/src/runtime/buffered-tuple-stream-v2.h
File be/src/runtime/buffered-tuple-stream-v2.h:

PS3, Line 212: ol::ClientHa
> This is just adding the mechanism, so policy is TBD. We'd discussed adding 
Yeah, I'm more just asking for the motivation. I guess it's a guard rail to prevent a this
from hoarding reservation?


PS3, Line 254: sed reservation was not sufficient to add
             :   ///   a new page t
> I'm not sure where the "above" is. 
By "above" i meant lines 87-88: 
/// To read or write a row larger than the default page size to/from an unpinned stream,
/// the caller must have max_page_len - default_page_len unused reservation.

That seems to contradict what this says which is that the callee can increase the reservation.


-- 
To view, visit http://gerrit.cloudera.org:8080/6638
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2861c58efa7bc1aeaa5b7e2f043c97cb3985c8f5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message