Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10926 )
Change subject: memrowset: support iteration with snap_to_exclude ...................................................................... Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc File src/kudu/tablet/memrowset.cc: http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc@514 PS7, Line 514: bool unset_in_sel_vector > this name is making me a little confused, because it can change again after Yeah, I agree that mutating unset_in_sel_vector inside ApplyMutationsToProjectedRow makes this harder to understand. I think a descriptive enum as an out parameter is a nice touch. I'll make the change here knowing that the net result is more information than necessary for this patch, but useful for the next one. http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc@517 PS7, Line 517: if (has_upper_bound() && out_of_bounds(k)) { : state_ = kFinished; : break; > I wonder if this actually belongs outside the if condition? Even if we excl Makes sense. I'll pull this check out of the outer if. -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 7 Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 25 Jul 2018 02:30:30 +0000 Gerrit-HasComments: Yes
