impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4023: don't attach buffered tuple streams to batches
Date Wed, 05 Oct 2016 21:33:13 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4023: don't attach buffered tuple streams to batches

Patch Set 6:

File be/src/exec/

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.
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I6471422f86ce71e4c6ab277a276000051bc2e8ff
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message