Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12156 )
Change subject: generic_iterators: assorted cleanup ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.h File src/kudu/common/generic_iterators.h: http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.h@58 PS3, Line 58: // All iterators must be fully able to evaluate all predicates (i.e. calling : // iter->Init(spec) should remove all predicates from the ScanSpec). > is this true? i thought the fact that we use InitAndMaybeWrap means that we Hmm, yes, that appears to be the case. I copied this comment from UnionIterator thinking it that if it's true there it must be true for both since both use the same "maybe wrapping" approach. I'll remove the comment from both. http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.cc File src/kudu/common/generic_iterators.cc: http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.cc@247 PS3, Line 247: if (!s->schema().Equals(*schema_)) { > I'd be on board with making this a DCHECK. It's on in all builds for UnionIterator so I assume we were comfortable with the perf hit. But I don't feel strongly, and since Mike prefers making it DEBUG-only, I'll make that change. http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.cc@249 PS3, Line 249: string("Schemas do not match: ") + schema_->ToString() : + " vs " + s->schema().ToString()); > this is somewhat non-idiomatic (why not use Substitute?) Copied code from UnionIterator. Will fix in both. http://gerrit.cloudera.org:8080/#/c/12156/3/src/kudu/common/generic_iterators.cc@472 PS3, Line 472: s.append("Union("); > I'd be inclined to do this in a follow up because I am modifying this same I don't mind doing this now; Mike's concern is valid but I don't think he's touching UnionIterator. -- 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: 3 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 (120) Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Fri, 11 Jan 2019 23:52:45 +0000 Gerrit-HasComments: Yes
