Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12195 )
Change subject: generic_iterators: move MergeIterState into the header ...................................................................... Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/12195/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12195/2//COMMIT_MSG@10 PS2, Line 10: forward declare MergeIterState anymore. > another option here to consider is moving the definition of UnionIterator a It can be done, but: 1. It'd require many small changes to generic_iterators-test. 2. More importantly, it won't be possible to test the innards of the moved iterators. For MergeIterator the only such test is the one that uses the new histogram, but I think I'll be removing that anyway. PredicateEvaluatingIterator does have a similar test right now (though you didn't mention moving that one). So it's not that big of a deal right now, but could be annoying in the future. Originally I was inclined to punt, but I gave it a shot and, after some more yak shaving, produced http://gerrit.cloudera.org:8080/12223. So this patch has now been superceded. -- To view, visit http://gerrit.cloudera.org:8080/12195 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00f3ec1a0007ab1f554f4b799884e94c90c7cc9b Gerrit-Change-Number: 12195 Gerrit-PatchSet: 4 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:53:38 +0000 Gerrit-HasComments: Yes
