impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4023: don't attach buffered tuple streams to batches
Date Sat, 22 Oct 2016 00:04:58 GMT
Dan Hecht has posted comments on this change.

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


Patch Set 9:

(7 comments)

Thanks, the renaming helps a lot.

My feeling is that the separation of attaching resources and flushing resources would be clearest
if the exec node just used MarkFlushResources() directly rather than threading through the
parameter.  But if you and Alex feel otherwise, I'm okay with leaving it this way.

http://gerrit.cloudera.org:8080/#/c/4448/9/be/src/runtime/buffered-tuple-stream.cc
File be/src/runtime/buffered-tuple-stream.cc:

Line 653:     // the caller can pass the rows up the operator tree.
let's make this comment actually be useful.  how about something like:

No more data in this block.  The batch must be immediately returned up the operator tree and
deep copied so that NextReadBlock() can reuse the read block's buffer.


http://gerrit.cloudera.org:8080/#/c/4448/9/be/src/runtime/row-batch.h
File be/src/runtime/row-batch.h:

PS9, Line 77: Different modes for flushing resources
these are really different modes for flushing resources.  isn't it just a flag to indicate
whether resources need to be flushed or not?


PS9, Line 227: as soon as possible
this is pretty vague.  how about saying what the rule is specifically:

Used by an operator to indicate that it cannot produce more rows until the resources that
it has attached to the row batch are freed or acquired by an ancestor operator.


PS9, Line 242:  
(that have not been attached to the batch)


PS9, Line 243: .
and deep copied.

(to differentiate from flush where the resources can be acquired).


PS9, Line 244: This is used to control memory management for streaming rows
this is kind of misleading and confusing now especially since "flush" controls this.


Line 246:   /// are not allowed to accumulate batches with the 'needs_deep_copy' flag.
how about saying this is deprecated?


-- 
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: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message