Dan Hecht has posted comments on this change. Change subject: IMPALA-4023: don't attach buffered tuple streams to batches ......................................................................
Patch Set 8: (2 comments) Yeah, I was thinking we could always attach resources to make it unambiguous. But I guess we can't necessarily do that on the non-close paths yet, until we have reservations? I think one thing that makes the row-batch code tough to follow is that "needs to return" is such vague terminology (it already was), and that just gets worse since "flush" also means (streaming) operators need to return immediately. So, what do you think about renaming "needs to return" with "needs deep copy" or something like that? In my mind, "needs deep copy" more clearly implies that the batch needs to be flushed up through the streaming operators and then copied at the blocking operator, which also makes the relationship with "flushing" more clear. 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 always flush resources on closing if we give it a batch to attach to? Likewise, if there is no batch, then we must have no flush (or rather, the flush mode doesn't make sense). So, why both having the caller pass in 'flush'? Why not just always flush in this branch? 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 the flag to AddBlock() but instead just calling this routine. That more clearly separates attaching and flushing concept (which you commit message says was a goal and I agree it seems better). -- 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
