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

Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2016/08/15 03:33:11 UTC

[kudu-CR] WIP: Fix ordered iterators

Hello Todd Lipcon,

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

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

to review the following change.

Change subject: WIP: Fix ordered iterators
......................................................................

WIP: Fix ordered iterators

Needs test.

Written by Todd.

Change-Id: I07651ef4451609d8228b5e0f0de022970db4ebcf
---
M src/kudu/common/generic_iterators.cc
1 file changed, 30 insertions(+), 24 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I07651ef4451609d8228b5e0f0de022970db4ebcf
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] MergeIterator: don't stop iterating on an empty block

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: MergeIterator: don't stop iterating on an empty block
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3976/2/src/kudu/common/generic_iterators-test.cc
File src/kudu/common/generic_iterators-test.cc:

Line 226: TEST(TestMergeIterator, TestMergePredicate2) {
This test passes for me without the fix.

However I believe the fix works because I rebased failing stuff on top of it and the failing stuff no longer fails.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07651ef4451609d8228b5e0f0de022970db4ebcf
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] MergeIterator: don't stop iterating on an empty block

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.

Kudu Jenkins has posted comments on this change.

Change subject: MergeIterator: don't stop iterating on an empty block
......................................................................


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/2888/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07651ef4451609d8228b5e0f0de022970db4ebcf
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] MergeIterator: don't stop iterating on an empty block

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.

Kudu Jenkins has posted comments on this change.

Change subject: MergeIterator: don't stop iterating on an empty block
......................................................................


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/2909/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07651ef4451609d8228b5e0f0de022970db4ebcf
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] MergeIterator: don't stop iterating on an empty block

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

Todd Lipcon has submitted this change and it was merged.

Change subject: MergeIterator: don't stop iterating on an empty block
......................................................................


MergeIterator: don't stop iterating on an empty block

The MergeIterator implementation was incorrectly considering an input
iterator to be fully exhausted after it received any block which had no
selected rows. This could be triggered in a couple cases:

- if a predicate dropped more consecutive rows than the block size, then
  the entire iterator would be empty and any further rows from that
  iterator would be skipped.
- if enough consecutive rows were deleted such that an entire block
  needed to be skipped, the same behavior was triggered.

The new test changes the test iterator so that it doesn't yield its
entire input in a single block. Then, I added a new predicate which
triggered the first case above. I also had to add another assertion
which verifies that the result list was not incorrectly truncated.
Lastly, the test had incorrectly treated the predicate upper bound as an
inclusive bound, whereas in fact it has been exclusive for quite some
time.

Change-Id: I07651ef4451609d8228b5e0f0de022970db4ebcf
Reviewed-on: http://gerrit.cloudera.org:8080/3976
Reviewed-by: Mike Percy <mp...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/common/generic_iterators-test.cc
M src/kudu/common/generic_iterators.cc
2 files changed, 61 insertions(+), 44 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I07651ef4451609d8228b5e0f0de022970db4ebcf
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] MergeIterator: don't stop iterating on an empty block

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.

Hello Anonymous Coward #80, Mike Percy, Kudu Jenkins,

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

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

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

Change subject: MergeIterator: don't stop iterating on an empty block
......................................................................

MergeIterator: don't stop iterating on an empty block

The MergeIterator implementation was incorrectly considering an input
iterator to be fully exhausted after it received any block which had no
selected rows. This could be triggered in a couple cases:

- if a predicate dropped more consecutive rows than the block size, then
  the entire iterator would be empty and any further rows from that
  iterator would be skipped.
- if enough consecutive rows were deleted such that an entire block
  needed to be skipped, the same behavior was triggered.

The new test changes the test iterator so that it doesn't yield its
entire input in a single block. Then, I added a new predicate which
triggered the first case above. I also had to add another assertion
which verifies that the result list was not incorrectly truncated.
Lastly, the test had incorrectly treated the predicate upper bound as an
inclusive bound, whereas in fact it has been exclusive for quite some
time.

Change-Id: I07651ef4451609d8228b5e0f0de022970db4ebcf
---
M src/kudu/common/generic_iterators-test.cc
M src/kudu/common/generic_iterators.cc
2 files changed, 57 insertions(+), 38 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I07651ef4451609d8228b5e0f0de022970db4ebcf
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] MergeIterator: don't stop iterating on an empty block

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.

Mike Percy has posted comments on this change.

Change subject: MergeIterator: don't stop iterating on an empty block
......................................................................


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07651ef4451609d8228b5e0f0de022970db4ebcf
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] MergeIterator: don't stop iterating on an empty block

Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.

Kudu Jenkins has posted comments on this change.

Change subject: MergeIterator: don't stop iterating on an empty block
......................................................................


Patch Set 4:

Build Started http://104.196.14.100/job/kudu-gerrit/2913/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07651ef4451609d8228b5e0f0de022970db4ebcf
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No