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

[Todd Lipcon (Code Review)]

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 
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 
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

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

[Mike Percy (Code Review)]

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 
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

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

[Todd Lipcon (Code Review)]

Todd Lipcon 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.
oops, thanks for checking. when I did some test cleanup, I lost the 
'set_block_size(10)' call on the VectorIterator. Fixed.


-- 
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 
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

[Kudu Jenkins (Code Review)]

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 
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

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

[Todd Lipcon (Code Review)]

Hello 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 (#4).

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, 61 insertions(+), 44 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/3976/4
-- 
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

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

[Kudu Jenkins (Code Review)]

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 
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

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

[Mike Percy (Code Review)]

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 
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

[Kudu Jenkins (Code Review)]

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 
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

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

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Anonymous Coward #80
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon