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] SelectionVector: pad extra bits with zeroes in constructor
Hello Mike Percy, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/13009
to review the following change.
Change subject: SelectionVector: pad extra bits with zeroes in constructor
......................................................................
SelectionVector: pad extra bits with zeroes in constructor
I found this after removing the MergeIterator's call to SetAllTrue(). It
turns out that if you don't call any functions that set all bytes en masse,
CountSelected() and AnySelected() will misbehave as they'll read garbage
data from any trailing bits beyond the logical end of the bitmap.
We can fix this in one of two ways:
- Modify CountSelected()/AnySelected() to ignore the trailing bits.
- Zero out the trailing bits in the constructor.
I opted for the second approach as I found it easier to implement, and I
suspect it's more performant than the first.
Change-Id: I74baf87c7f38b7eea0ad46825c2421c99f8f42a1
---
M src/kudu/common/rowblock-test.cc
M src/kudu/common/rowblock.cc
M src/kudu/common/rowblock.h
3 files changed, 41 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/13009/1
--
To view, visit http://gerrit.cloudera.org:8080/13009
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I74baf87c7f38b7eea0ad46825c2421c99f8f42a1
Gerrit-Change-Number: 13009
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] SelectionVector: pad extra bits with zeroes in constructor
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13009 )
Change subject: SelectionVector: pad extra bits with zeroes in constructor
......................................................................
SelectionVector: pad extra bits with zeroes in constructor
I found this after removing the MergeIterator's call to SetAllTrue(). It
turns out that if you don't call any functions that set all bytes en masse,
CountSelected() and AnySelected() will misbehave as they'll read garbage
data from any trailing bits beyond the logical end of the bitmap.
We can fix this in one of two ways:
- Modify CountSelected()/AnySelected() to ignore the trailing bits.
- Zero out the trailing bits in the constructor.
I opted for the second approach as I found it easier to implement, and I
suspect it's more performant than the first.
Change-Id: I74baf87c7f38b7eea0ad46825c2421c99f8f42a1
Reviewed-on: http://gerrit.cloudera.org:8080/13009
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/common/rowblock-test.cc
M src/kudu/common/rowblock.cc
M src/kudu/common/rowblock.h
3 files changed, 41 insertions(+), 14 deletions(-)
Approvals:
Kudu Jenkins: Verified
Mike Percy: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/13009
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I74baf87c7f38b7eea0ad46825c2421c99f8f42a1
Gerrit-Change-Number: 13009
Gerrit-PatchSet: 6
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] SelectionVector: pad extra bits with zeroes in constructor
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13009 )
Change subject: SelectionVector: pad extra bits with zeroes in constructor
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13009/5/src/kudu/common/rowblock-test.cc
File src/kudu/common/rowblock-test.cc:
http://gerrit.cloudera.org:8080/#/c/13009/5/src/kudu/common/rowblock-test.cc@20
PS5, Line 20: #include <cstddef>
Just curious, what is this for?
--
To view, visit http://gerrit.cloudera.org:8080/13009
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I74baf87c7f38b7eea0ad46825c2421c99f8f42a1
Gerrit-Change-Number: 13009
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 02:14:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] SelectionVector: pad extra bits with zeroes in constructor
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13009 )
Change subject: SelectionVector: pad extra bits with zeroes in constructor
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13009/5/src/kudu/common/rowblock-test.cc
File src/kudu/common/rowblock-test.cc:
http://gerrit.cloudera.org:8080/#/c/13009/5/src/kudu/common/rowblock-test.cc@20
PS5, Line 20: #include <cstddef>
> Just curious, what is this for?
It's for size_t.
--
To view, visit http://gerrit.cloudera.org:8080/13009
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I74baf87c7f38b7eea0ad46825c2421c99f8f42a1
Gerrit-Change-Number: 13009
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:00:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] SelectionVector: pad extra bits with zeroes in constructor
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13009 )
Change subject: SelectionVector: pad extra bits with zeroes in constructor
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13009
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I74baf87c7f38b7eea0ad46825c2421c99f8f42a1
Gerrit-Change-Number: 13009
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] SelectionVector: pad extra bits with zeroes in constructor
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/13009
to look at the new patch set (#4).
Change subject: SelectionVector: pad extra bits with zeroes in constructor
......................................................................
SelectionVector: pad extra bits with zeroes in constructor
I found this after removing the MergeIterator's call to SetAllTrue(). It
turns out that if you don't call any functions that set all bytes en masse,
CountSelected() and AnySelected() will misbehave as they'll read garbage
data from any trailing bits beyond the logical end of the bitmap.
We can fix this in one of two ways:
- Modify CountSelected()/AnySelected() to ignore the trailing bits.
- Zero out the trailing bits in the constructor.
I opted for the second approach as I found it easier to implement, and I
suspect it's more performant than the first.
Change-Id: I74baf87c7f38b7eea0ad46825c2421c99f8f42a1
---
M src/kudu/common/rowblock-test.cc
M src/kudu/common/rowblock.cc
M src/kudu/common/rowblock.h
3 files changed, 41 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/13009/4
--
To view, visit http://gerrit.cloudera.org:8080/13009
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I74baf87c7f38b7eea0ad46825c2421c99f8f42a1
Gerrit-Change-Number: 13009
Gerrit-PatchSet: 4
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] SelectionVector: pad extra bits with zeroes in constructor
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13009 )
Change subject: SelectionVector: pad extra bits with zeroes in constructor
......................................................................
Patch Set 4: Verified+1
Overriding Jenkins, known flaky test failures.
--
To view, visit http://gerrit.cloudera.org:8080/13009
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I74baf87c7f38b7eea0ad46825c2421c99f8f42a1
Gerrit-Change-Number: 13009
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>
Gerrit-Comment-Date: Mon, 15 Apr 2019 22:42:11 +0000
Gerrit-HasComments: No