Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12157 )
Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock ...................................................................... Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/12157/4/src/kudu/common/generic_iterators-test.cc File src/kudu/common/generic_iterators-test.cc: http://gerrit.cloudera.org:8080/#/c/12157/4/src/kudu/common/generic_iterators-test.cc@235 PS4, Line 235: } > would this work just using emplace_back(std::move(ints), std::move(sv))? or Brace initialization apparently only works if I qualify the initializer-list with List: all_ints.emplace_back(List{ std::move(ints), std::move(sv) }); http://gerrit.cloudera.org:8080/#/c/12157/6/src/kudu/common/generic_iterators-test.cc File src/kudu/common/generic_iterators-test.cc: http://gerrit.cloudera.org:8080/#/c/12157/6/src/kudu/common/generic_iterators-test.cc@217 PS6, Line 217: VLOG(2) << Substitute("Row $0 with value $1 selected? $2", > Would you mind adding a comment about this? I'm confused about what is bein Think of the deselection as yet another predicate: besides omitting certain rows from output of the iteration it also affects the list of ints we expect (see L227). Will add a comment. http://gerrit.cloudera.org:8080/#/c/12157/4/src/kudu/common/generic_iterators.cc File src/kudu/common/generic_iterators.cc: http://gerrit.cloudera.org:8080/#/c/12157/4/src/kudu/common/generic_iterators.cc@199 PS4, Line 199: // Seek next_row_ to the first selected row. > maybe this isn't a bottleneck, but mind adding a TODO to use BitmapFindFirs The only reason I didn't use BitmapFindFirst already is because a follow-up patch needs to find the last selected row, and I didn't want to spend a bunch of time writing a correct BitmapFindLast. And without BitmapFindLast, the asymmetry bothered me. :/ But that's pretty silly, so I modified this to use BitmapFindFirst. http://gerrit.cloudera.org:8080/#/c/12157/4/src/kudu/common/generic_iterators.cc@207 PS4, Line 207: LOG(DFATAL) << "Unreachable code"; // guaranteed by the short-circuit above > wonder whether this should be a real FATAL - note that DFATAL still emits e Moot point now; BitmapFindFirst alters the control flow so that this isn't needed anymore. -- To view, visit http://gerrit.cloudera.org:8080/12157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Gerrit-Change-Number: 12157 Gerrit-PatchSet: 7 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: Fri, 11 Jan 2019 23:52:49 +0000 Gerrit-HasComments: Yes
