Skip to content

ARROW-4341: [C++] Refactor Primitive builders and BooleanBuilder to use TypedBufferBuilder<T> #3575

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bkietz wants to merge 17 commits into from
Closed

ARROW-4341: [C++] Refactor Primitive builders and BooleanBuilder to use TypedBufferBuilder<T> #3575

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
88eb71c
Use TypedBufferBuilder<bool> in BooleanBuilder
bkietz Feb 5, 2019
078497a
fix BooleanBuilder::AppendNull
bkietz Feb 6, 2019
9c050b4
Use TypedBufferBuilder for PrimitiveBuilder
bkietz Feb 6, 2019
88e57fe
remove PrimitiveBuilder
bkietz Feb 6, 2019
8934573
add explicit cast
bkietz Feb 6, 2019
188b7b9
fix format
bkietz Feb 6, 2019
7ba692c
moving to iterator append in NumericBuilder
bkietz Feb 8, 2019
bd736c3
add ArrowLogIgnore and use for release mode DCHECK*
bkietz Feb 9, 2019
09d2bfe
reduce unnecessary zeroing in BufferBuilder
bkietz Feb 11, 2019
6c1e99d
Add TypedBufferBuilder<bool> UnsafeAppend compile-time option to not …
wesm Feb 14, 2019
d4ab3b5
Move NumericBuilder implementation to headers to avoid symbol visibil…
wesm Feb 14, 2019
21ce285
Fix RETURN_NOT_OK usages
wesm Feb 14, 2019
3cc5a0c
Restore memory zeroing. Add missing override
wesm Feb 15, 2019
daf5244
Revert change to UnsafeAppend that broke Python unit test
wesm Feb 15, 2019
b389c13
Revert changes to arrow/util/logging.h
wesm Feb 15, 2019
40c4d8d
TypedBufferBuilder<bool>'s output was not correctly sized
bkietz Feb 15, 2019
3ef2972
Fix BooleanBuilder::AppendNulls, remove valid_bytes argument from App…
wesm Feb 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion cpp/src/arrow/array-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,11 @@ void TestPrimitiveBuilder<PBoolean>::Check(const std::unique_ptr<BooleanBuilder>
ASSERT_EQ(draws_[i] != 0, actual) << i;
}
}
ASSERT_TRUE(result->Equals(*expected));
AssertArraysEqual(*result, *expected);

// buffers are correctly sized
ASSERT_EQ(result->data()->buffers[0]->size(), BitUtil::BytesForBits(size));
ASSERT_EQ(result->data()->buffers[1]->size(), BitUtil::BytesForBits(size));

// Builder is now reset
ASSERT_EQ(0, builder->length());
Expand Down
169 changes: 14 additions & 155 deletions cpp/src/arrow/array/builder_primitive.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,104 +45,8 @@ Status NullBuilder::FinishInternal(std::shared_ptr<ArrayData>* out) {
return Status::OK();
}

// ----------------------------------------------------------------------

template <typename T>
void PrimitiveBuilder<T>::Reset() {
data_.reset();
raw_data_ = nullptr;
}

template <typename T>
Status PrimitiveBuilder<T>::Resize(int64_t capacity) {
RETURN_NOT_OK(CheckCapacity(capacity, capacity_));
capacity = std::max(capacity, kMinBuilderCapacity);

int64_t nbytes = TypeTraits<T>::bytes_required(capacity);
if (capacity_ == 0) {
RETURN_NOT_OK(AllocateResizableBuffer(pool_, nbytes, &data_));
} else {
RETURN_NOT_OK(data_->Resize(nbytes));
}

raw_data_ = reinterpret_cast<value_type*>(data_->mutable_data());
return ArrayBuilder::Resize(capacity);
}

template <typename T>
Status PrimitiveBuilder<T>::AppendValues(const value_type* values, int64_t length,
const uint8_t* valid_bytes) {
RETURN_NOT_OK(Reserve(length));

if (length > 0) {
std::memcpy(raw_data_ + length_, values,
static_cast<std::size_t>(TypeTraits<T>::bytes_required(length)));
}

// length_ is update by these
ArrayBuilder::UnsafeAppendToBitmap(valid_bytes, length);
return Status::OK();
}

template <typename T>
Status PrimitiveBuilder<T>::AppendValues(const value_type* values, int64_t length,
const std::vector<bool>& is_valid) {
RETURN_NOT_OK(Reserve(length));
DCHECK_EQ(length, static_cast<int64_t>(is_valid.size()));

if (length > 0) {
std::memcpy(raw_data_ + length_, values,
static_cast<std::size_t>(TypeTraits<T>::bytes_required(length)));
}

// length_ is update by these
ArrayBuilder::UnsafeAppendToBitmap(is_valid);
return Status::OK();
}

template <typename T>
Status PrimitiveBuilder<T>::AppendValues(const std::vector<value_type>& values,
const std::vector<bool>& is_valid) {
return AppendValues(values.data(), static_cast<int64_t>(values.size()), is_valid);
}

template <typename T>
Status PrimitiveBuilder<T>::AppendValues(const std::vector<value_type>& values) {
return AppendValues(values.data(), static_cast<int64_t>(values.size()));
}

template <typename T>
Status PrimitiveBuilder<T>::FinishInternal(std::shared_ptr<ArrayData>* out) {
RETURN_NOT_OK(TrimBuffer(TypeTraits<T>::bytes_required(length_), data_.get()));
std::shared_ptr<Buffer> null_bitmap;
RETURN_NOT_OK(null_bitmap_builder_.Finish(&null_bitmap));
*out = ArrayData::Make(type_, length_, {null_bitmap, data_}, null_count_);

data_ = nullptr;
capacity_ = length_ = null_count_ = 0;

return Status::OK();
}

template class PrimitiveBuilder<UInt8Type>;
template class PrimitiveBuilder<UInt16Type>;
template class PrimitiveBuilder<UInt32Type>;
template class PrimitiveBuilder<UInt64Type>;
template class PrimitiveBuilder<Int8Type>;
template class PrimitiveBuilder<Int16Type>;
template class PrimitiveBuilder<Int32Type>;
template class PrimitiveBuilder<Int64Type>;
template class PrimitiveBuilder<Date32Type>;
template class PrimitiveBuilder<Date64Type>;
template class PrimitiveBuilder<Time32Type>;
template class PrimitiveBuilder<Time64Type>;
template class PrimitiveBuilder<TimestampType>;
template class PrimitiveBuilder<HalfFloatType>;
template class PrimitiveBuilder<FloatType>;
template class PrimitiveBuilder<DoubleType>;

BooleanBuilder::BooleanBuilder(MemoryPool* pool)
: ArrayBuilder(boolean(), pool), data_(nullptr), raw_data_(nullptr) {}
: ArrayBuilder(boolean(), pool), data_builder_(pool) {}

BooleanBuilder::BooleanBuilder(const std::shared_ptr<DataType>& type, MemoryPool* pool)
: BooleanBuilder(pool) {
Expand All @@ -151,57 +55,23 @@ BooleanBuilder::BooleanBuilder(const std::shared_ptr<DataType>& type, MemoryPool

void BooleanBuilder::Reset() {
ArrayBuilder::Reset();
data_.reset();
raw_data_ = nullptr;
data_builder_.Reset();
}

Status BooleanBuilder::Resize(int64_t capacity) {
RETURN_NOT_OK(CheckCapacity(capacity, capacity_));
capacity = std::max(capacity, kMinBuilderCapacity);

const int64_t new_bitmap_size = BitUtil::BytesForBits(capacity);
if (capacity_ == 0) {
RETURN_NOT_OK(AllocateResizableBuffer(pool_, new_bitmap_size, &data_));
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.
//
// We only zero up to new_bitmap_size because the padding was zeroed by
// AllocateResizableBuffer
memset(raw_data_, 0, static_cast<size_t>(new_bitmap_size));
} else {
const int64_t old_bitmap_capacity = data_->capacity();
RETURN_NOT_OK(data_->Resize(new_bitmap_size));
const int64_t new_bitmap_capacity = data_->capacity();
raw_data_ = reinterpret_cast<uint8_t*>(data_->mutable_data());

// See comment above about why we zero memory for booleans
memset(raw_data_ + old_bitmap_capacity, 0,
static_cast<size_t>(new_bitmap_capacity - old_bitmap_capacity));
}

RETURN_NOT_OK(data_builder_.Resize(capacity));
return ArrayBuilder::Resize(capacity);
}

Status BooleanBuilder::FinishInternal(std::shared_ptr<ArrayData>* out) {
int64_t bit_offset = length_ % 8;
if (bit_offset > 0) {
// Adjust last byte
data_->mutable_data()[length_ / 8] &= BitUtil::kPrecedingBitmask[bit_offset];
}

std::shared_ptr<Buffer> null_bitmap;
std::shared_ptr<Buffer> data, null_bitmap;
RETURN_NOT_OK(data_builder_.Finish(&data));
RETURN_NOT_OK(null_bitmap_builder_.Finish(&null_bitmap));
RETURN_NOT_OK(TrimBuffer(BitUtil::BytesForBits(length_), data_.get()));

*out = ArrayData::Make(boolean(), length_, {null_bitmap, data_}, null_count_);
*out = ArrayData::Make(boolean(), length_, {null_bitmap, data}, null_count_);

data_ = nullptr;
capacity_ = length_ = null_count_ = 0;
return Status::OK();
}
Expand All @@ -211,10 +81,8 @@ Status BooleanBuilder::AppendValues(const uint8_t* values, int64_t length,
RETURN_NOT_OK(Reserve(length));

int64_t i = 0;
internal::GenerateBitsUnrolled(raw_data_, length_, length,
[values, &i]() -> bool { return values[i++] != 0; });

// this updates length_
data_builder_.UnsafeAppend<false>(length,
[values, &i]() -> bool { return values[i++] != 0; });
ArrayBuilder::UnsafeAppendToBitmap(valid_bytes, length);
return Status::OK();
}
Expand All @@ -223,12 +91,9 @@ Status BooleanBuilder::AppendValues(const uint8_t* values, int64_t length,
const std::vector<bool>& is_valid) {
RETURN_NOT_OK(Reserve(length));
DCHECK_EQ(length, static_cast<int64_t>(is_valid.size()));

int64_t i = 0;
internal::GenerateBitsUnrolled(raw_data_, length_, length,
[values, &i]() -> bool { return values[i++]; });

// this updates length_
data_builder_.UnsafeAppend<false>(length,
[values, &i]() -> bool { return values[i++]; });
ArrayBuilder::UnsafeAppendToBitmap(is_valid);
return Status::OK();
}
Expand All @@ -247,25 +112,19 @@ Status BooleanBuilder::AppendValues(const std::vector<bool>& values,
const int64_t length = static_cast<int64_t>(values.size());
RETURN_NOT_OK(Reserve(length));
DCHECK_EQ(length, static_cast<int64_t>(is_valid.size()));

int64_t i = 0;
internal::GenerateBitsUnrolled(raw_data_, length_, length,
[&values, &i]() -> bool { return values[i++]; });

// this updates length_
data_builder_.UnsafeAppend<false>(length,
[&values, &i]() -> bool { return values[i++]; });
ArrayBuilder::UnsafeAppendToBitmap(is_valid);
return Status::OK();
}

Status BooleanBuilder::AppendValues(const std::vector<bool>& values) {
const int64_t length = static_cast<int64_t>(values.size());
RETURN_NOT_OK(Reserve(length));

int64_t i = 0;
internal::GenerateBitsUnrolled(raw_data_, length_, length,
[&values, &i]() -> bool { return values[i++]; });

// this updates length_
data_builder_.UnsafeAppend<false>(length,
[&values, &i]() -> bool { return values[i++]; });
ArrayBuilder::UnsafeSetNotNull(length);
return Status::OK();
}
Expand Down
Loading