Tim Armstrong has posted comments on this change. Change subject: IMPALA-4023: don't attach buffered tuple streams to batches ......................................................................
Patch Set 8: (3 comments) http://gerrit.cloudera.org:8080/#/c/4448/8//COMMIT_MSG Commit Message: Did the need_to_return/needs_deep_copy rename http://gerrit.cloudera.org:8080/#/c/4448/8/be/src/runtime/buffered-tuple-stream.cc File be/src/runtime/buffered-tuple-stream.cc: Line 196: batch->AddBlock(block, flush); > when would we ever want to have batch != NULL and no flush? Shouldn't we a I don't think the BufferedTupleStream abstraction show have to know anything about the buffer management algorithm in the ExecNode implementations. Always having Close() flush assumes that that's what the ExecNode wants, but I don't think that would always be the case. E.g. it could make sense to have an ExecNode that flushes only when it needs more memory rather than every time it calls a particular BufferedTupleStream method. This is also to make it explicit in the calling code that a flush is occurring (since it's normally an important part of the buffer management algorithm and it's pretty obscure if it's hidden in a Close() function). We also have callsites where we call Close(batch, FlushMode::FLUSH_RESOURCES) where batch is sometimes NULL. E.g. Partition::Close(RowBatch* batch). In the case it makes sense to ignore just the flag if the batch is NULL. http://gerrit.cloudera.org:8080/#/c/4448/8/be/src/runtime/sorter.cc File be/src/runtime/sorter.cc: Line 828: output_batch->MarkFlushResources(); > if we need to expose MarkFlushResources() too, how about not also having th I added the flag mainly as a way to force the caller to think about whether they should be flushing resources or not. Happy to remove it if you think it's unnecessary. -- To view, visit http://gerrit.cloudera.org:8080/4448 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6471422f86ce71e4c6ab277a276000051bc2e8ff Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
