You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/01/09 09:42:56 UTC

[kudu-CR] generic iterators: move MergeIterState into the header

Hello Mike Percy, Grant Henke, Todd Lipcon,

I'd like you to do a code review. Please visit

    http://gerrit.cloudera.org:8080/12195

to review the following change.


Change subject: generic_iterators: move MergeIterState into the header
......................................................................

generic_iterators: move MergeIterState into the header

I plan on converting states_ into an intrusive list, which means we can't
forward declare MergeIterState anymore.

Change-Id: I00f3ec1a0007ab1f554f4b799884e94c90c7cc9b
---
M src/kudu/common/generic_iterators.cc
M src/kudu/common/generic_iterators.h
2 files changed, 84 insertions(+), 80 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/12195/1
-- 
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: newchange
Gerrit-Change-Id: I00f3ec1a0007ab1f554f4b799884e94c90c7cc9b
Gerrit-Change-Number: 12195
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] generic iterators: move MergeIterState into the header

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

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 <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 11 Jan 2019 23:53:38 +0000
Gerrit-HasComments: Yes