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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
