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/04/13 04:12:16 UTC

[kudu-CR] generic iterators: refactor MergeIterator for whole-block-copy optimization 

Hello Mike Percy, Todd Lipcon,

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

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

to review the following change.


Change subject: generic_iterators: refactor MergeIterator for whole-block-copy optimization
......................................................................

generic_iterators: refactor MergeIterator for whole-block-copy optimization

This patch refactors MergeIterator::MaterializeBlock in preparation for the
whole-block-copy optimization. Specifically, the "copy next row" and
"advance iter and rebalance heaps" operations have been decomposed into
standalone functions so that it'll be easier to add "copy whole block" as
an equivalent function in the future.

Also of note: we no longer set the entire client selection vector up front,
as this won't necessarily be the case when we copy a block.

Change-Id: I050116edc51bb3e91852e6286c221175770e6382
---
M src/kudu/common/generic_iterators.cc
1 file changed, 135 insertions(+), 130 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/13010/1
-- 
To view, visit http://gerrit.cloudera.org:8080/13010
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I050116edc51bb3e91852e6286c221175770e6382
Gerrit-Change-Number: 13010
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] generic iterators: refactor MergeIterator for whole-block-copy optimization

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Kudu Jenkins, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#2).

Change subject: generic_iterators: refactor MergeIterator for whole-block-copy optimization
......................................................................

generic_iterators: refactor MergeIterator for whole-block-copy optimization

This patch refactors MergeIterator::MaterializeBlock in preparation for the
whole-block-copy optimization. Specifically, the "copy next row" and
"advance iter and rebalance heaps" operations have been decomposed into
standalone functions so that it'll be easier to add "copy whole block" as
an equivalent function in the future.

Also of note: we no longer set the entire client selection vector up front,
as this won't necessarily be the case when we copy a block.

Change-Id: I050116edc51bb3e91852e6286c221175770e6382
---
M src/kudu/common/generic_iterators.cc
1 file changed, 135 insertions(+), 130 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/13010/2
-- 
To view, visit http://gerrit.cloudera.org:8080/13010
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I050116edc51bb3e91852e6286c221175770e6382
Gerrit-Change-Number: 13010
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] generic iterators: refactor MergeIterator for whole-block-copy optimization

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed a vote on this change.

Change subject: generic_iterators: refactor MergeIterator for whole-block-copy optimization
......................................................................


Removed Verified+1 by Adar Dembo <ad...@cloudera.com>
-- 
To view, visit http://gerrit.cloudera.org:8080/13010
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I050116edc51bb3e91852e6286c221175770e6382
Gerrit-Change-Number: 13010
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] generic iterators: refactor MergeIterator for whole-block-copy optimization

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change.  ( http://gerrit.cloudera.org:8080/13010 )

Change subject: generic_iterators: refactor MergeIterator for whole-block-copy optimization
......................................................................


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/13010
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I050116edc51bb3e91852e6286c221175770e6382
Gerrit-Change-Number: 13010
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] generic iterators: refactor MergeIterator for whole-block-copy optimization

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Mike Percy, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#4).

Change subject: generic_iterators: refactor MergeIterator for whole-block-copy optimization
......................................................................

generic_iterators: refactor MergeIterator for whole-block-copy optimization

This patch refactors MergeIterator::MaterializeBlock in preparation for the
whole-block-copy optimization. Specifically, the "copy next row" and
"advance iter and rebalance heaps" operations have been decomposed into
standalone functions so that it'll be easier to add "copy whole block" as
an equivalent function in the future.

Also of note: we no longer set the entire client selection vector up front,
as this won't necessarily be the case when we copy a block.

Change-Id: I050116edc51bb3e91852e6286c221175770e6382
---
M src/kudu/common/generic_iterators.cc
1 file changed, 135 insertions(+), 130 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/13010/4
-- 
To view, visit http://gerrit.cloudera.org:8080/13010
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I050116edc51bb3e91852e6286c221175770e6382
Gerrit-Change-Number: 13010
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] generic iterators: refactor MergeIterator for whole-block-copy optimization

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13010 )

Change subject: generic_iterators: refactor MergeIterator for whole-block-copy optimization
......................................................................


Patch Set 5:

> As far as I can tell you didn't change any functionality in this patch, rather just reorganized the existing code, so I only did a cursory code review.

That's more or less true. The (small) things that changed:
- The RowBlock's selection vector is now set row-by-row instead of all true in one go. I could have moved that change to the "whole block copy" optimization patch but I got lazy.
- There's a tiny bit of handling for dst_row_idx since it's now passed across functions.
- The 'smallest' vector is initialized via default constructor and reserve() rather than filled.


-- 
To view, visit http://gerrit.cloudera.org:8080/13010
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I050116edc51bb3e91852e6286c221175770e6382
Gerrit-Change-Number: 13010
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Apr 2019 21:05:05 +0000
Gerrit-HasComments: No

[kudu-CR] generic iterators: refactor MergeIterator for whole-block-copy optimization

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change.  ( http://gerrit.cloudera.org:8080/13010 )

Change subject: generic_iterators: refactor MergeIterator for whole-block-copy optimization
......................................................................


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/13010
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I050116edc51bb3e91852e6286c221175770e6382
Gerrit-Change-Number: 13010
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] generic iterators: refactor MergeIterator for whole-block-copy optimization

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13010 )

Change subject: generic_iterators: refactor MergeIterator for whole-block-copy optimization
......................................................................


Patch Set 3: Verified+1

Overriding Jenkins, known Java flakes.


-- 
To view, visit http://gerrit.cloudera.org:8080/13010
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I050116edc51bb3e91852e6286c221175770e6382
Gerrit-Change-Number: 13010
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 15 Apr 2019 04:22:41 +0000
Gerrit-HasComments: No

[kudu-CR] generic iterators: refactor MergeIterator for whole-block-copy optimization

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13010 )

Change subject: generic_iterators: refactor MergeIterator for whole-block-copy optimization
......................................................................


Patch Set 5: Code-Review+2

Thanks for splitting out this refactor to simplify reviewing the main whole-block-copy patch. As far as I can tell you didn't change any functionality in this patch, rather just reorganized the existing code, so I only did a cursory code review.


-- 
To view, visit http://gerrit.cloudera.org:8080/13010
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I050116edc51bb3e91852e6286c221175770e6382
Gerrit-Change-Number: 13010
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Apr 2019 20:50:41 +0000
Gerrit-HasComments: No

[kudu-CR] generic iterators: refactor MergeIterator for whole-block-copy optimization

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13010 )

Change subject: generic_iterators: refactor MergeIterator for whole-block-copy optimization
......................................................................

generic_iterators: refactor MergeIterator for whole-block-copy optimization

This patch refactors MergeIterator::MaterializeBlock in preparation for the
whole-block-copy optimization. Specifically, the "copy next row" and
"advance iter and rebalance heaps" operations have been decomposed into
standalone functions so that it'll be easier to add "copy whole block" as
an equivalent function in the future.

Also of note: we no longer set the entire client selection vector up front,
as this won't necessarily be the case when we copy a block.

Change-Id: I050116edc51bb3e91852e6286c221175770e6382
Reviewed-on: http://gerrit.cloudera.org:8080/13010
Reviewed-by: Mike Percy <mp...@apache.org>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/common/generic_iterators.cc
1 file changed, 135 insertions(+), 130 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Adar Dembo: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/13010
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I050116edc51bb3e91852e6286c221175770e6382
Gerrit-Change-Number: 13010
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] generic iterators: refactor MergeIterator for whole-block-copy optimization

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13010 )

Change subject: generic_iterators: refactor MergeIterator for whole-block-copy optimization
......................................................................


Patch Set 5: Verified+1

Overriding Jenkins, unrelated flaky test failure.


-- 
To view, visit http://gerrit.cloudera.org:8080/13010
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I050116edc51bb3e91852e6286c221175770e6382
Gerrit-Change-Number: 13010
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 17 Apr 2019 21:05:23 +0000
Gerrit-HasComments: No