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 1 commit
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
3 changes: 3 additions & 0 deletions cpp/src/arrow/buffer-builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,9 @@ class TypedBufferBuilder<bool> {
}

Status Finish(std::shared_ptr<Buffer>* out, bool shrink_to_fit = true) {
// set bytes_builder_.size_ == byte size of data
bytes_builder_.UnsafeAdvance(BitUtil::BytesForBits(bit_length_) -
bytes_builder_.length());
Copy link
Member

@wesm wesm Feb 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these bytes need to be zeroed?

Copy link
Member Author

@bkietz bkietz Feb 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypedBufferBuilder<bool> doesn't utilize bytes_builder_'s append methods, which means the size of bytes_builder_ doesn't change as bits are appended.

I'm thinking it's not worthwhile to use BufferBuilder in this class, I'm going to refactor and instead just manage a buffer directly

Copy link
Member

@wesm wesm Feb 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, please do that refactoring in a follow up PR and not this one =)

bit_length_ = false_count_ = 0;
return bytes_builder_.Finish(out, shrink_to_fit);
}
Expand Down
4 changes: 4 additions & 0 deletions cpp/src/arrow/buffer-test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,8 @@ TEST(TestBufferBuilder, BasicBoolBufferBuilderUsage) {
for (int i = 0; i != nvalues; ++i) {
ASSERT_EQ(BitUtil::GetBit(built->data(), i + 1), static_cast<bool>(values[i]));
}

ASSERT_EQ(built->size(), BitUtil::BytesForBits(nvalues + 1));
}

TEST(TestBufferBuilder, BoolBufferBuilderAppendCopies) {
Expand All @@ -367,6 +369,8 @@ TEST(TestBufferBuilder, BoolBufferBuilderAppendCopies) {
for (int i = 0; i != 13 + 17; ++i) {
EXPECT_EQ(BitUtil::GetBit(built->data(), i), i < 13) << "index = " << i;
}

ASSERT_EQ(built->size(), BitUtil::BytesForBits(13 + 17));
}

template <typename T>
Expand Down