[kudu-CR] SelectionVector: pad extra bits with zeroes in constructor
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 --- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] SelectionVector: pad extra bits with zeroes in constructor
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 > 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Apr 2019 21:00:31 + Gerrit-HasComments: Yes
[kudu-CR] SelectionVector: pad extra bits with zeroes in constructor
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 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 17 Apr 2019 02:14:45 + Gerrit-HasComments: Yes
[kudu-CR] SelectionVector: pad extra bits with zeroes in constructor
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] SelectionVector: pad extra bits with zeroes in constructor
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 15 Apr 2019 22:42:11 + Gerrit-HasComments: No
[kudu-CR] SelectionVector: pad extra bits with zeroes in constructor
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 Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[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 Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon