This is an automated email from the ASF dual-hosted git repository.
apitrou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new 31d2afc28a GH-39126: [C++][CI] Fix Valgrind failures (#39127)
31d2afc28a is described below
commit 31d2afc28a201bda78da8b0229e823413ff82e0d
Author: Antoine Pitrou <[email protected]>
AuthorDate: Sat Dec 9 16:39:51 2023 +0100
GH-39126: [C++][CI] Fix Valgrind failures (#39127)
### Rationale for this change
### What changes are included in this PR?
### Are these changes tested?
### Are there any user-facing changes?
* Closes: #39126
Lead-authored-by: Antoine Pitrou <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Co-authored-by: Benjamin Kietzman <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
---
cpp/src/arrow/array/array_dict_test.cc | 2 +-
cpp/src/arrow/array/array_test.cc | 1 +
cpp/src/arrow/array/builder_binary.cc | 9 +++++----
cpp/src/arrow/array/builder_binary.h | 31 ++++++++++++++++++++-----------
4 files changed, 27 insertions(+), 16 deletions(-)
diff --git a/cpp/src/arrow/array/array_dict_test.cc
b/cpp/src/arrow/array/array_dict_test.cc
index 2f3ee6e2d4..a4c03b5db6 100644
--- a/cpp/src/arrow/array/array_dict_test.cc
+++ b/cpp/src/arrow/array/array_dict_test.cc
@@ -1129,7 +1129,7 @@ TEST(TestDictionary, Validate) {
arr = std::make_shared<DictionaryArray>(dict_type, indices,
MakeArray(invalid_data));
ASSERT_RAISES(Invalid, arr->ValidateFull());
-#if !defined(__APPLE__)
+#if !defined(__APPLE__) && !defined(ARROW_VALGRIND)
// GH-35712: ASSERT_DEATH would make testing slow on MacOS.
ASSERT_DEATH(
{
diff --git a/cpp/src/arrow/array/array_test.cc
b/cpp/src/arrow/array/array_test.cc
index 974eb54d2c..e9d478f108 100644
--- a/cpp/src/arrow/array/array_test.cc
+++ b/cpp/src/arrow/array/array_test.cc
@@ -728,6 +728,7 @@ TEST_F(TestArray, TestMakeArrayFromScalar) {
}
for (auto scalar : scalars) {
+ ARROW_SCOPED_TRACE("scalar type: ", scalar->type->ToString());
AssertAppendScalar(pool_, scalar);
}
}
diff --git a/cpp/src/arrow/array/builder_binary.cc
b/cpp/src/arrow/array/builder_binary.cc
index 3ff22d4a3f..f85852fa0e 100644
--- a/cpp/src/arrow/array/builder_binary.cc
+++ b/cpp/src/arrow/array/builder_binary.cc
@@ -80,10 +80,11 @@ Status BinaryViewBuilder::AppendArraySlice(const ArraySpan&
array, int64_t offse
Status BinaryViewBuilder::FinishInternal(std::shared_ptr<ArrayData>* out) {
ARROW_ASSIGN_OR_RAISE(auto null_bitmap,
null_bitmap_builder_.FinishWithLength(length_));
ARROW_ASSIGN_OR_RAISE(auto data, data_builder_.FinishWithLength(length_));
- BufferVector buffers = {null_bitmap, data};
- for (auto&& buffer : data_heap_builder_.Finish()) {
- buffers.push_back(std::move(buffer));
- }
+ ARROW_ASSIGN_OR_RAISE(auto byte_buffers, data_heap_builder_.Finish());
+ BufferVector buffers(byte_buffers.size() + 2);
+ buffers[0] = std::move(null_bitmap);
+ buffers[1] = std::move(data);
+ std::move(byte_buffers.begin(), byte_buffers.end(), buffers.begin() + 2);
*out = ArrayData::Make(type(), length_, std::move(buffers), null_count_);
Reset();
return Status::OK();
diff --git a/cpp/src/arrow/array/builder_binary.h
b/cpp/src/arrow/array/builder_binary.h
index 3e87cf2403..d825f7d325 100644
--- a/cpp/src/arrow/array/builder_binary.h
+++ b/cpp/src/arrow/array/builder_binary.h
@@ -524,16 +524,11 @@ class ARROW_EXPORT StringHeapBuilder {
"strings larger than 2GB");
}
if (num_bytes > current_remaining_bytes_) {
- // Ensure the buffer is fully overwritten to avoid leaking uninitialized
- // bytes from the allocator
- if (current_remaining_bytes_ > 0) {
- std::memset(current_out_buffer_, 0, current_remaining_bytes_);
- blocks_.back() = SliceBuffer(blocks_.back(), 0,
- blocks_.back()->size() -
current_remaining_bytes_);
- }
+ ARROW_RETURN_NOT_OK(FinishLastBlock());
current_remaining_bytes_ = num_bytes > blocksize_ ? num_bytes :
blocksize_;
- ARROW_ASSIGN_OR_RAISE(std::shared_ptr<Buffer> new_block,
- AllocateBuffer(current_remaining_bytes_,
alignment_, pool_));
+ ARROW_ASSIGN_OR_RAISE(
+ std::shared_ptr<ResizableBuffer> new_block,
+ AllocateResizableBuffer(current_remaining_bytes_, alignment_,
pool_));
current_offset_ = 0;
current_out_buffer_ = new_block->mutable_data();
blocks_.emplace_back(std::move(new_block));
@@ -550,7 +545,10 @@ class ARROW_EXPORT StringHeapBuilder {
int64_t current_remaining_bytes() const { return current_remaining_bytes_; }
- std::vector<std::shared_ptr<Buffer>> Finish() {
+ Result<std::vector<std::shared_ptr<ResizableBuffer>>> Finish() {
+ if (!blocks_.empty()) {
+ ARROW_RETURN_NOT_OK(FinishLastBlock());
+ }
current_offset_ = 0;
current_out_buffer_ = NULLPTR;
current_remaining_bytes_ = 0;
@@ -558,10 +556,21 @@ class ARROW_EXPORT StringHeapBuilder {
}
private:
+ Status FinishLastBlock() {
+ if (current_remaining_bytes_ > 0) {
+ // Avoid leaking uninitialized bytes from the allocator
+ ARROW_RETURN_NOT_OK(
+ blocks_.back()->Resize(blocks_.back()->size() -
current_remaining_bytes_,
+ /*shrink_to_fit=*/true));
+ blocks_.back()->ZeroPadding();
+ }
+ return Status::OK();
+ }
+
MemoryPool* pool_;
int64_t alignment_;
int64_t blocksize_ = kDefaultBlocksize;
- std::vector<std::shared_ptr<Buffer>> blocks_;
+ std::vector<std::shared_ptr<ResizableBuffer>> blocks_;
int32_t current_offset_ = 0;
uint8_t* current_out_buffer_ = NULLPTR;
