apacheGH-39126: [C++][CI] Fix Valgrind failures (apache#39127)

### Rationale for this change ### What changes are included in this PR? ### Are these changes tested? ### Are there any user-facing changes? * Closes: apache#39126 Lead-authored-by: Antoine Pitrou <pitrou@free.fr> Co-authored-by: Antoine Pitrou <antoine@python.org> Co-authored-by: Benjamin Kietzman <bengilgit@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
mapleFU · Dec 13, 2023 · d317a14 · d317a14
1 parent 991192c
commit d317a14
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 16 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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,18 +545,32 @@ 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;
     return std::move(blocks_);
   }
 
  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;