review comments

apache · Nov 29, 2023 · df068c2 · df068c2
1 parent 14db1ce
commit df068c2
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 20 deletions.
diff --git a/cpp/src/arrow/c/bridge.cc b/cpp/src/arrow/c/bridge.cc
@@ -53,7 +53,6 @@ namespace arrow {
 using internal::checked_cast;
 using internal::checked_pointer_cast;
 
-using internal::Zip;
 
 using internal::SmallVector;
 using internal::StaticVector;
@@ -595,8 +594,9 @@ struct ArrayExporter {
     if (need_variadic_buffer_sizes) {
       auto variadic_buffers = util::span(data->buffers).subspan(2);
       export_.variadic_buffer_sizes_.resize(variadic_buffers.size());
-      for (auto [size, buf] : Zip(export_.variadic_buffer_sizes_, variadic_buffers)) {
-        size = buf->size();
+      size_t i = 0;
+      for (const auto& buf : variadic_buffers) {
+        export_.variadic_buffer_sizes_[i++] = buf->size();
       }
       export_.buffers_.back() = export_.variadic_buffer_sizes_.data();
     }
@@ -1896,7 +1896,8 @@ struct ArrayImporter {
   std::shared_ptr<Buffer> zero_size_buffer_;
 
   std::shared_ptr<MemoryManager> memory_mgr_;
-  DeviceAllocationType device_type_{};
+  DeviceAllocationType device_type_{DeviceAllocationType::kCPU};
+;
 };
 
 }  // namespace

diff --git a/cpp/src/arrow/c/bridge_test.cc b/cpp/src/arrow/c/bridge_test.cc
@@ -126,7 +126,7 @@ class ReleaseCallback {
  private:
   ARROW_DISALLOW_COPY_AND_ASSIGN(ReleaseCallback);
 
-  bool called_{};
+  bool called_{false};
   void (*orig_release_)(CType*);
   void* orig_private_data_;
 };
@@ -290,7 +290,7 @@ struct SchemaExportChecker {
   const std::vector<std::string> flattened_names_;
   std::vector<int64_t> flattened_flags_;
   const std::vector<std::string> flattened_metadata_;
-  size_t flattened_index_{};
+  size_t flattened_index_{0};
 };
 
 class TestSchemaExport : public ::testing::Test {
@@ -928,16 +928,23 @@ static const BinaryViewType::c_type binary_view_buffer1[] = {
     util::ToInlineBinaryView("quux"),
 };
 
-TEST_F(TestArrayExport, BinaryViewMultipleBuffers) {
-  auto length = static_cast<int64_t>(std::size(binary_view_buffer1));
-  auto arr = std::make_shared<BinaryViewArray>(
-      binary_view(), length, Buffer::Wrap(binary_view_buffer1, length),
+static auto MakeBinaryViewArrayWithMultipleDataBuffers() {
+  static const auto kLength = static_cast<int64_t>(std::size(binary_view_buffer1));
+  return std::make_shared<BinaryViewArray>(
+      binary_view(), kLength,
+      Buffer::FromVector(std::vector(binary_view_buffer1, binary_view_buffer1 + kLength)),
       BufferVector{
-          std::make_shared<Buffer>(binary_view_buffer_content0),
-          std::make_shared<Buffer>(binary_view_buffer_content1),
+          Buffer::FromString(std::string{binary_view_buffer_content0}),
+          Buffer::FromString(std::string{binary_view_buffer_content1}),
       });
-  TestNested([&] { return arr; });
-  TestNested([&] { return arr->Slice(1, length - 2); });
+}
+
+TEST_F(TestArrayExport, BinaryViewMultipleBuffers) {
+  TestPrimitive(MakeBinaryViewArrayWithMultipleDataBuffers);
+  TestPrimitive([&] {
+    auto arr = MakeBinaryViewArrayWithMultipleDataBuffers();
+    return arr->Slice(1, arr->length() - 2);
+  });
 }
 
 TEST_F(TestArrayExport, Null) {
@@ -2913,12 +2920,7 @@ TEST_F(TestArrayImport, String) {
 
   auto length = static_cast<int64_t>(std::size(binary_view_buffer1));
   FillStringViewLike(length, 0, 0, binary_view_buffers_no_nulls1, 2);
-  CheckImport(std::make_shared<BinaryViewArray>(
-      binary_view(), length, Buffer::Wrap(binary_view_buffer1, length),
-      BufferVector{
-          std::make_shared<Buffer>(binary_view_buffer_content0),
-          std::make_shared<Buffer>(binary_view_buffer_content1),
-      }));
+  CheckImport(MakeBinaryViewArrayWithMultipleDataBuffers());
 
   // Empty array with null data pointers
   FillStringLike(0, 0, 0, string_buffers_omitted);
@@ -3625,6 +3627,7 @@ TEST_F(TestSchemaRoundtrip, Primitive) {
   TestWithTypeFactory([] { return fixed_size_binary(3); });
   TestWithTypeFactory(binary);
   TestWithTypeFactory(large_utf8);
+  TestWithTypeFactory(binary_view);
 }
 
 TEST_F(TestSchemaRoundtrip, Temporal) {
@@ -3889,6 +3892,14 @@ TEST_F(TestArrayRoundtrip, Primitive) {
                      R"([[4, 5, 6], [1, -600, 5000], null, null])");
 }
 
+TEST_F(TestArrayRoundtrip, BinaryViewMultipleBuffers) {
+  TestWithArrayFactory(MakeBinaryViewArrayWithMultipleDataBuffers);
+  TestWithArrayFactory([&] {
+    auto arr = MakeBinaryViewArrayWithMultipleDataBuffers();
+    return arr->Slice(1, arr->length() - 2);
+  });
+}
+
 TEST_F(TestArrayRoundtrip, UnknownNullCount) {
   TestWithArrayFactory([]() -> Result<std::shared_ptr<Array>> {
     auto arr = ArrayFromJSON(int32(), "[0, 1, 2]");