Tim Armstrong has posted comments on this change. Change subject: IMPALA-4023: don't attach buffered tuple streams to batches ......................................................................
Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/4448/6/be/src/exec/analytic-eval-node.cc File be/src/exec/analytic-eval-node.cc: Line 758: if (row_batch->num_blocks() > 0) row_batch->MarkFlushResources(); > Can we move this logic into BufferedTupleStream::Close()? I deliberately avoided doing that because the decision to flush should be the responsibility of the ExecNode, rather than something that is hidden in a utility class. The BufferedTupleStream doesn't really know what the ExecNode's memory requirements are for the next phase of its processing, so shouldn't be making the decision to flush resources. http://gerrit.cloudera.org:8080/#/c/4448/6/be/src/runtime/row-batch.h File be/src/runtime/row-batch.h: Line 356: bool flush_resources_; > I don't see flush_resources_ being read/returned. Could we replace FlushRes The bit is transferred in RowBatch::TransferResourceOwnership(), which is the key difference from MarkAtCapacity(). We need the extra bit of information to distinguish between a batch that's just full and a request to flush the resources all the way up the operator pipeline. The old AddTupleStream() API essentially did that, but in an obfuscated way. E.g. if you have a pipeline of joins, and attach a Block at the bottom join node then call FlushResources(), you want resources to be flushed all the way up before pulling the next row from the bottom join node. The resources should be transferred from the probe batch to the output batch at each join node, so it will propagate all the way up. -- 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: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
