Tim Armstrong has posted comments on this change. Change subject: IMPALA-5085: large rows in BufferedTupleStreamV2 ......................................................................
Patch Set 7: (2 comments) I ended up reworking this a lot so that it does lazy page allocation and explicitly saves read and write reservations when they're not being used to hold a buffer in memory. 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: Line 340: return has_write_iterator && num_pages == 0; > Doesn't the large page case already break that? i.e. how do we ensure the r Yeah that's fair - the client definitely needs to be involved here. I'm assuming clients would handle failure to append a large page to an unpinned stream in a similar way to handling failure to append to a pinned stream - by freeing up memory by unpinning a pinned stream. The handling of large pages in this patch makes it easier on the client because the large page in an unpinned stream doesn't hold onto excess reservation. PS5, Line 380: ONSISTENCY(); > Yeah that's true. It would make sense if appending a larger row was "best e I removed this increase logic from the read page, agree that it doesn't make sense on the read path. The write path handles both pinned and unpinned cases so it makes a bit more sense to try to increase there. -- 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: 7 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
