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 <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
