Todd Lipcon has posted comments on this change. Change subject: generic_iterators: avoid holding completed iters in merge/union ......................................................................
Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/6460/1/src/kudu/common/generic_iterators.cc File src/kudu/common/generic_iterators.cc: Line 54: > extra line Done Line 65: > nit: mind making the spacing above/below the anonymous namespace consistent Done Line 176: : schema_(std::move(schema)), > warning: passing result of std::move() as a const reference argument; no mo tried to get too fancy here. went back to old code Line 325: iters_.assign(iters.begin(), iters.end()); > I see, so we're avoiding extra shared_ptr copies by not tracking all_iters, yea, the idea is that as we pop off elements from iters_, that is hopefully the last remaining reference to the iter, so it gets to destruct as it goes. http://gerrit.cloudera.org:8080/#/c/6460/1/src/kudu/common/generic_iterators.h File src/kudu/common/generic_iterators.h: Line 43: // a subset of the columns in 'iters'. > So we're copying the input iterators because each subiterator is a shared_p Good catch, I'd missed the critical part of the patch for MergeIterator which actually clears orig_iters_! (I did testing with UnionIterator). Fixed in next rev. PS1, Line 78: vector<IteratorStats> finished_iter_stats_by_col_; > docs Done PS1, Line 124: void PopFront(); > docs Done PS1, Line 131: vector<IteratorStats> finished_iter_stats_by_col_; > docs Done -- To view, visit http://gerrit.cloudera.org:8080/6460 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e068c256c5855efeff5c6325dbb5fbf770eb900 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
