Michael Ho has posted comments on this change. Change subject: IMPALA-5481: Clarify RowDescriptor ownership ......................................................................
Patch Set 3: (3 comments) May want to wait for IMPALA-4192 to merge first before rebasing. http://gerrit.cloudera.org:8080/#/c/7206/2//COMMIT_MSG Commit Message: PS2, Line 18: Method arguments are either const* to convey that : ownership is to be shared, or const& to convey that the descriptor is to : be used but not mutated by the callee. > We have typically used T* to show that a T is not owned by the enclosing ob Thanks for the explanation. I am aware of the convention to use "T*" to show ownership doesn't belong to the enclosing object. This patch is more of a clean up to convert the use of const T& to const T* for non-owning reference to an object. Part of the confusion is that in past reviews, the opposite has been asked: why a member is const T* instead of const T&. const T* suggests that the member could be null. I am fine with either way as long as we stick to one convention. http://gerrit.cloudera.org:8080/#/c/7206/2/be/src/exec/data-sink.h File be/src/exec/data-sink.h: PS2, Line 82: static Status Create(ObjectPool* pool, const TPlanFragmentCtx& fragment_ctx, : const TPlanFragmentInstanceCtx& fragment_instance_ctx, : const RowDescriptor* row_desc, DataSink** sink); > In that case DataSinkCreate() is called with an ExecNode::row_desc() parame I see. http://gerrit.cloudera.org:8080/#/c/7206/3/be/src/exec/data-sink.h File be/src/exec/data-sink.h: Line 103: /// The row descriptor for the rows consumed by the sink. Not owned. May help to also add the owner's identity here. -- To view, visit http://gerrit.cloudera.org:8080/7206 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2fc39170f775581d406b6a97445046f508d8d75f Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
