Adar Dembo 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's tricky. There's a CHECK that'll fire if the projection has zero key columns, but we can't enforce that it actually has _all_ of the columns that comprise the primary key, because the MergeIterator doesn't have access to the tablet's schema and thus doesn't know what they all are. There's code in the scan setup (tablet_service.cc) that will add any missing key columns to an ORDERED scan's projection, so it shouldn't be possible to actually construct a non-conformant MergeIterator in production. But it is possible if you looked at MergeIterator in isolation. I'll move the comment and expand on this a bit. 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? It's a bit moot because later on I'm converting states_ into an intrusive list which doesn't support emplace_back. -- 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: Mon, 14 Jan 2019 21:04:01 +0000 Gerrit-HasComments: Yes
