-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
ARROW-1712: [C++] Add method to BinaryBuilder to reserve space for value data #1481
Changes from 1 commit
c2f8dc4
232024e
d021c54
5b73c1c
5ebfb32
e0434e6
de318f4
b002e0b
8dd5eaa
9b5e805
5a5593e
15e045c
bbc6527
18f90fb
0b07895
d3c8202
8e4c892
5a5b70e
bc5db7d
77f8f3c
d4bbd15
360e601
707b67b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1154,40 +1154,44 @@ TEST_F(TestBinaryBuilder, TestScalarAppend) { | |
} | ||
} | ||
} | ||
|
||
TEST_F(TestBinaryBuilder, TestCapacityReserve) { | ||
vector<string> strings = {"aaaaa", "bbbbbbbbbb", "ccccccccccccccc", "dddddddddddddddddddd", "eeeeeeeeee"}; | ||
vector<string> strings = {"aaaaa", "bbbbbbbbbb", "ccccccccccccccc", "dddddddddd"}; | ||
int N = static_cast<int>(strings.size()); | ||
int reps = 10; | ||
int reps = 15; | ||
int64_t length = 0; | ||
int64_t capacity = 1000; | ||
|
||
int64_t expected_capacity = BitUtil::RoundUpToMultipleOf64(capacity); | ||
|
||
ASSERT_OK(builder_->ReserveData(capacity)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These reservations should be viewed as different, because the size of the offsets buffer and the data buffer grow at different rates. The way this unit test should read is:
|
||
|
||
ASSERT_EQ(length, builder_->value_data_length()); | ||
ASSERT_EQ(BitUtil::RoundUpToMultipleOf64(capacity), builder_->value_data_capacity()); | ||
ASSERT_EQ(expected_capacity, builder_->value_data_capacity()); | ||
|
||
for (int j = 0; j < reps; ++j) { | ||
for (int i = 0; i < N; ++i) { | ||
ASSERT_OK(builder_->Append(strings[i])); | ||
length += static_cast<int>(strings[i].size()); | ||
|
||
ASSERT_EQ(length, builder_->value_data_length()); | ||
ASSERT_EQ(BitUtil::RoundUpToMultipleOf64(capacity), builder_->value_data_capacity()); | ||
ASSERT_EQ(expected_capacity, builder_->value_data_capacity()); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add another call to ReserveData here, like |
||
|
||
int extra_capacity = 500; | ||
expected_capacity = BitUtil::RoundUpToMultipleOf64(length + extra_capacity); | ||
|
||
ASSERT_OK(builder_->ReserveData(extra_capacity)); | ||
|
||
ASSERT_EQ(length, builder_->value_data_length()); | ||
ASSERT_EQ(BitUtil::RoundUpToMultipleOf64(length + extra_capacity), builder_->value_data_capacity()); | ||
ASSERT_EQ(expected_capacity, builder_->value_data_capacity()); | ||
|
||
Done(); | ||
|
||
ASSERT_EQ(reps * N, result_->length()); | ||
ASSERT_EQ(0, result_->null_count()); | ||
ASSERT_EQ(reps * 60, result_->value_data()->size()); | ||
ASSERT_EQ(BitUtil::RoundUpToMultipleOf64(length + extra_capacity), result_->value_data()->capacity()); | ||
ASSERT_EQ(reps * 40, result_->value_data()->size()); | ||
ASSERT_EQ(expected_capacity, result_->value_data()->capacity()); | ||
} | ||
|
||
TEST_F(TestBinaryBuilder, TestZeroLength) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, you can run
make format
(which uses clang-format) to fix these long lines without having to make code changes