Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12946 )
Change subject: generic_iterators: pass rowset bounds into grouping iterators ...................................................................... Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/12946/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12946/6//COMMIT_MSG@14 PS6, Line 14: Normally the rowset bounds are encoded, but they need to be decoded upon > I'm confused about this part. Is this comment still relevant to this patch, I was trying to draw attention to the difference between the bounds retrieved from RowSet::GetBounds, which are encoded, and the rows retrieved from sub-iterators, which are decoded. I'm trying to say that it's more efficient to decode the encoded bounds once and store them that way than it is to encode the retrieved rows during the merge. You're right that some of this explanation is not relevant to this patch because, since writing this, I moved the decoding into a different patch. I'll update it. http://gerrit.cloudera.org:8080/#/c/12946/6/src/kudu/tablet/rowset.h File src/kudu/tablet/rowset.h: http://gerrit.cloudera.org:8080/#/c/12946/6/src/kudu/tablet/rowset.h@153 PS6, Line 153: virtual Status NewRowIteratorWithBounds(const RowIteratorOptions& opts, > I'm curious: why did you put the implementation in the header file? NewRowI If there was a good reason, I can't remember it. I'll move it out. Not even sure why it's a virtual function. http://gerrit.cloudera.org:8080/#/c/12946/6/src/kudu/tablet/rowset.h@167 PS6, Line 167: std::move(encoded_bounds); > nit: would be more terse to say: Done -- To view, visit http://gerrit.cloudera.org:8080/12946 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id1aebd84507f87c869d781e117225c37c8f15969 Gerrit-Change-Number: 12946 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Sat, 13 Apr 2019 04:12:24 +0000 Gerrit-HasComments: Yes
