This is an automated email from the ASF dual-hosted git repository.
uwe pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/master by this push:
new a096eb1 ARROW-2821: [C++] Remove redundant memsets in BooleanBuilder
a096eb1 is described below
commit a096eb12ee0af9bdb2ac02ea3ef913a139f2d948
Author: Wes McKinney <[email protected]>
AuthorDate: Tue Jul 10 10:30:42 2018 +0200
ARROW-2821: [C++] Remove redundant memsets in BooleanBuilder
After the work in ARROW-2790, we were zeroing the memory in two places.
Even though there was some memory zeroing logic in AppendNull and
AppendNulls, there are still some cases where the data buffer might have set
bits when there is null data. While the Arrow columnar format does not
guarantee that boolean values in null slots will be false, zeroing things make
the array contents coming from this path deterministic.
Author: Wes McKinney <[email protected]>
Closes #2236 from wesm/ARROW-2821 and squashes the following commits:
e7dcbf9e <Wes McKinney> Expand comment
c102ceb9 <Wes McKinney> Remove additional memsets
---
cpp/src/arrow/builder.cc | 9 +++++++++
cpp/src/arrow/builder.h | 10 ----------
2 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/cpp/src/arrow/builder.cc b/cpp/src/arrow/builder.cc
index b40453c..d8498ce 100644
--- a/cpp/src/arrow/builder.cc
+++ b/cpp/src/arrow/builder.cc
@@ -738,6 +738,13 @@ Status BooleanBuilder::Init(int64_t capacity) {
RETURN_NOT_OK(data_->Resize(nbytes));
raw_data_ = reinterpret_cast<uint8_t*>(data_->mutable_data());
+
+ // We zero the memory for booleans to keep things simple; for some reason if
+ // we do not, even though we may write every bit (through in-place | or &),
+ // valgrind will still show a warning. If we do not zero the bytes here, we
+ // will have to be careful to zero them in AppendNull and AppendNulls. Also,
+ // zeroing the bits results in deterministic bits when each byte may have a
+ // mix of nulls and not nulls.
memset(raw_data_, 0, static_cast<size_t>(nbytes));
return Status::OK();
@@ -760,6 +767,8 @@ Status BooleanBuilder::Resize(int64_t capacity) {
if (new_bytes > old_bytes) {
RETURN_NOT_OK(data_->Resize(new_bytes));
raw_data_ = reinterpret_cast<uint8_t*>(data_->mutable_data());
+
+ // See comment above about why we zero memory for booleans
memset(raw_data_ + old_bytes, 0, static_cast<size_t>(new_bytes -
old_bytes));
}
}
diff --git a/cpp/src/arrow/builder.h b/cpp/src/arrow/builder.h
index 3753923..7a4704d 100644
--- a/cpp/src/arrow/builder.h
+++ b/cpp/src/arrow/builder.h
@@ -558,12 +558,6 @@ class ARROW_EXPORT BooleanBuilder : public ArrayBuilder {
/// Write nulls as uint8_t* (0 value indicates null) into pre-allocated
memory
Status AppendNulls(const uint8_t* valid_bytes, int64_t length) {
RETURN_NOT_OK(Reserve(length));
- // zero bits starting with the next whole bytes, since the bytes before
- // should be already initialized
-
- const int64_t old_bytes = BitUtil::BytesForBits(length_);
- const int64_t new_bytes = BitUtil::BytesForBits(length_ + length);
- memset(raw_data_ + old_bytes, 0, new_bytes - old_bytes);
UnsafeAppendToBitmap(valid_bytes, length);
return Status::OK();
@@ -571,10 +565,6 @@ class ARROW_EXPORT BooleanBuilder : public ArrayBuilder {
Status AppendNull() {
RETURN_NOT_OK(Reserve(1));
- if (BitUtil::IsMultipleOf8(length_)) {
- // zero next byte
- memset(raw_data_ + (length_ / 8), 0, 1);
- }
UnsafeAppendToBitmap(false);
return Status::OK();
}
