Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12156 )
Change subject: generic_iterators: assorted cleanup ...................................................................... Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/12156/4/src/kudu/common/generic_iterators.h File src/kudu/common/generic_iterators.h: http://gerrit.cloudera.org:8080/#/c/12156/4/src/kudu/common/generic_iterators.h@51 PS4, Line 51: otherwise the merge algorithm's key comparisons won't work nit: this is a bit odd wording. Will we get some incorrect results? Or will it fail a CHECK? or what? It seems like it should fail a CHECK and this doc should explicitly say that it REQUIRES that the projection include key columns (and maybe move that comment to the constructor which takes the list of iters) http://gerrit.cloudera.org:8080/#/c/12156/4/src/kudu/common/generic_iterators.cc File src/kudu/common/generic_iterators.cc: http://gerrit.cloudera.org:8080/#/c/12156/4/src/kudu/common/generic_iterators.cc@281 PS4, Line 281: states_.push_back(unique_ptr<MergeIterState nit: emplace_back and you can avoid the unique_ptr<> bit? -- To view, visit http://gerrit.cloudera.org:8080/12156 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f1fd5f4cd1a8ef621d3ac994e764dfd19e99544 Gerrit-Change-Number: 12156 Gerrit-PatchSet: 4 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, 12 Jan 2019 01:05:48 +0000 Gerrit-HasComments: Yes
