fix: Ensure ArrowArrayBuffer() and ArrowArraySetBuffer() work for variadic buffers

python/src/nanoarrow/_array.pyx

@@ -32,6 +32,7 @@ from cpython cimport (
 
 from nanoarrow_c cimport (
     ArrowArray,
+    ArrowArrayAddVariadicBuffers,
     ArrowArrayAppendBytes,
     ArrowArrayAppendNull,
     ArrowArrayAppendString,
@@ -721,6 +722,13 @@ cdef class CArrayBuilder:
         self._ptr.null_count = self._ptr.length - count
         return self
 
+    def ensure_buffers(self, n_buffers) -> CArrayBuilder:
+        if self._ptr.n_buffers < n_buffers:
+            code = ArrowArrayAddVariadicBuffers(self._ptr, n_buffers - self._ptr.n_buffers)
+            Error.raise_error_not_ok("ArrowArrayAddVariadicBuffers() failed", code)
+
+        return self
+
     def set_buffer(self, int64_t i, CBuffer buffer, move=False) -> CArrayBuilder:
         """Set an ArrowArray buffer
 
@@ -732,8 +740,8 @@ cdef class CArrayBuilder:
         is False (the default), this function will a make a shallow copy via another
         layer of Python object wrapping.
         """
-        if i < 0 or i > 3:
-            raise IndexError("i must be >= 0 and <= 3")
+        if i < 0 or i >= self._ptr.n_buffers:
+            raise IndexError(f"i must be >= 0 and < {self._ptr.n_buffers}")
 
         if buffer._device != self._device:
             raise ValueError(
@@ -749,11 +757,6 @@ cdef class CArrayBuilder:
 
         ArrowBufferMove(buffer._ptr, ArrowArrayBuffer(self._ptr, i))
 
-        # The buffer's lifecycle is now owned by the array; however, we need
-        # array->buffers[i] to be updated such that it equals
-        # ArrowArrayBuffer(array, i)->data.
-        self._ptr.buffers[i] = ArrowArrayBuffer(self._ptr, i).data
-
         return self
 
     def set_child(self, int64_t i, CArray c_array, move=False) -> CArrayBuilder:
@@ -819,20 +822,22 @@ cdef class CArrayBuilder:
         cdef Error error = Error()
         cdef int code
 
-        if self._can_validate:
-            c_validation_level = NANOARROW_VALIDATION_LEVEL_DEFAULT
-            if validation_level == "full":
-                c_validation_level = NANOARROW_VALIDATION_LEVEL_FULL
-            elif validation_level == "minimal":
-                c_validation_level = NANOARROW_VALIDATION_LEVEL_MINIMAL
-            elif validation_level == "none":
-                c_validation_level = NANOARROW_VALIDATION_LEVEL_NONE
+        if not self._can_validate and validation_level not in (None, "none"):
+            raise NotImplementedError("Validation for array with children is not implemented")
 
-            code = ArrowArrayFinishBuilding(self._ptr, c_validation_level, &error.c_error)
-            error.raise_message_not_ok("ArrowArrayFinishBuildingDefault()", code)
+        if not self._can_validate:
+            validation_level = "none"
 
-        elif validation_level not in (None, "none"):
-            raise NotImplementedError("Validation for array with children is not implemented")
+        c_validation_level = NANOARROW_VALIDATION_LEVEL_DEFAULT
+        if validation_level == "full":
+            c_validation_level = NANOARROW_VALIDATION_LEVEL_FULL
+        elif validation_level == "minimal":
+            c_validation_level = NANOARROW_VALIDATION_LEVEL_MINIMAL
+        elif validation_level == "none":
+            c_validation_level = NANOARROW_VALIDATION_LEVEL_NONE
+
+        code = ArrowArrayFinishBuilding(self._ptr, c_validation_level, &error.c_error)
+        error.raise_message_not_ok("ArrowArrayFinishBuildingDefault()", code)
 
         out = self.c_array
         self.c_array = CArray.allocate(CSchema.allocate())

python/src/nanoarrow/_utils.pyx

@@ -32,6 +32,7 @@ from cpython.ref cimport Py_INCREF, Py_DECREF
 
 from nanoarrow_c cimport (
     ArrowArray,
+    ArrowArrayAddVariadicBuffers,
     ArrowArrayAllocateChildren,
     ArrowArrayAllocateDictionary,
     ArrowArrayBuffer,
@@ -363,6 +364,11 @@ cdef void c_array_shallow_copy(object base, const ArrowArray* src, ArrowArray* d
     tmp.offset = src.offset
     tmp.null_count = src.null_count
 
+    # We might need some more buffers if we are shallowly copying a string/binary view
+    if src.n_buffers > 3:
+        code = ArrowArrayAddVariadicBuffers(dst, src.n_buffers - 3)
+        Error.raise_error_not_ok("ArrowArrayAddVariadicBuffers()", code)
+
     for i in range(src.n_buffers):
         if src.buffers[i] != NULL:
             # The purpose of this buffer is soley so that we can use the

python/src/nanoarrow/c_array.py

@@ -278,6 +278,10 @@ def c_array_from_buffers(
     # of children have been initialized.
     builder.init_from_schema(schema)
 
+    # We might need more buffers if we're constructing a string or binary view
+    buffers = list(buffers)
+    builder.ensure_buffers(len(buffers))
+
     # Set buffers, optionally moving ownership of the buffers as well (i.e.,
     # the objects in the input buffers would be replaced with an empty ArrowBuffer)
     for i, buffer in enumerate(buffers):

python/tests/test_c_array.py

@@ -501,6 +501,36 @@ def test_c_array_from_buffers_recursive():
         )
 
 
+def test_c_array_from_buffers_string_view():
+    # Creating the actual view buffer is hard, but we can at least make sure the
+    # buffers are able to be roundtripped.
+    c_array = na.c_array_from_buffers(
+        na.string_view(),
+        0,
+        [
+            None,
+            None,
+            b"banana one",
+            b"banana two",
+            b"banana three",
+            na.c_buffer([10, 10, 12], na.int64()),
+        ],
+        validation_level="none",
+    )
+    assert c_array.length == 0
+    assert c_array.null_count == 0
+    assert c_array.offset == 0
+
+    array_view = c_array.view()
+    assert array_view.storage_type == "string_view"
+    assert bytes(array_view.buffer(0)) == b""
+    assert bytes(array_view.buffer(1)) == b""
+    assert bytes(array_view.buffer(2)) == b"banana one"
+    assert bytes(array_view.buffer(3)) == b"banana two"
+    assert bytes(array_view.buffer(4)) == b"banana three"
+    assert list(array_view.buffer(5)) == [10, 10, 12]
+
+
 def test_c_array_from_buffers_validation():
     # Should fail with all validation levels except none
     for validation_level in ("full", "default", "minimal"):

r/R/array.R

@@ -332,6 +332,10 @@ nanoarrow_array_modify <- function(array, new_values, validate = TRUE) {
     )
   }
 
+  # Finish building (e.g., ensure pointers are flushed)
+  .Call(nanoarrow_c_array_finish_building, array_copy)
+
+  # Validate if requested
   if (!is.null(schema) && validate) {
     array_copy <- .Call(nanoarrow_c_array_validate_after_modify, array_copy, schema)
   }

r/src/array.c

@@ -93,8 +93,8 @@ SEXP nanoarrow_c_array_set_buffers(SEXP array_xptr, SEXP buffers_sexp) {
   struct ArrowArray* array = nanoarrow_array_from_xptr(array_xptr);
 
   int64_t n_buffers = Rf_xlength(buffers_sexp);
-  if (n_buffers > 3) {
-    Rf_error("length(array$buffers) must be <= 3");
+  if (n_buffers != array->n_buffers) {
+    Rf_error("Changing the number of buffers in array_modify is not supported");
   }
 
   // Release any buffers that aren't about to be replaced
@@ -248,6 +248,17 @@ static int move_array_buffers(struct ArrowArray* src, struct ArrowArray* dst,
   return NANOARROW_OK;
 }
 
+SEXP nanoarrow_c_array_finish_building(SEXP array_xptr) {
+  struct ArrowArray* array = nanoarrow_array_from_xptr(array_xptr);
+  struct ArrowError error;
+  int result = ArrowArrayFinishBuilding(array, NANOARROW_VALIDATION_LEVEL_NONE, &error);
+  if (result != NANOARROW_OK) {
+    Rf_error("ArrowArrayFinishBuilding(): %s", error.message);
+  }
+
+  return R_NilValue;
+}
+
 SEXP nanoarrow_c_array_validate_after_modify(SEXP array_xptr, SEXP schema_xptr) {
   // A very particular type of validation we can do with the ArrowArray we use
   // in nanoarrow_array_modify() (which was created using ArrowArrayInit).
@@ -271,6 +282,16 @@ SEXP nanoarrow_c_array_validate_after_modify(SEXP array_xptr, SEXP schema_xptr)
     Rf_error("ArrowArrayInitFromSchema(): %s", error.message);
   }
 
+  // Add any variadic buffers that might be required
+  if ((array->n_buffers > array_dst->n_buffers) &&
+      array->n_buffers > NANOARROW_MAX_FIXED_BUFFERS) {
+    result =
+        ArrowArrayAddVariadicBuffers(array_dst, array->n_buffers - array_dst->n_buffers);
+    if (result != NANOARROW_OK) {
+      Rf_error("ArrowArrayAddVariadicBuffers() failed");
+    }
+  }
+
   result = move_array_buffers(array, array_dst, schema, &error);
   if (result != NANOARROW_OK) {
     Rf_error("move_array_buffers: %s", error.message);

r/src/array.h

@@ -94,6 +94,15 @@ static inline void array_export(SEXP array_xptr, struct ArrowArray* array_copy)
   array_copy->offset = array->offset;
 
   // Get buffer references, each of which preserve a reference to independent_array_xptr
+
+  // We might need some more buffers to be allocated for string views
+  if (array->n_buffers > 3) {
+    result = ArrowArrayAddVariadicBuffers(array_copy, array->n_buffers - 3);
+    if (result != NANOARROW_OK) {
+      Rf_error("ArrowArrayAddVariadicBuffers() failed");
+    }
+  }
+
   array_copy->n_buffers = array->n_buffers;
   for (int64_t i = 0; i < array->n_buffers; i++) {
     SEXP borrowed_buffer =

r/src/init.c

@@ -40,6 +40,7 @@ extern SEXP nanoarrow_c_array_set_offset(SEXP array_xptr, SEXP offset_sexp);
 extern SEXP nanoarrow_c_array_set_buffers(SEXP array_xptr, SEXP buffers_sexp);
 extern SEXP nanoarrow_c_array_set_children(SEXP array_xptr, SEXP children_sexp);
 extern SEXP nanoarrow_c_array_set_dictionary(SEXP array_xptr, SEXP dictionary_xptr);
+extern SEXP nanoarrow_c_array_finish_building(SEXP array_xptr);
 extern SEXP nanoarrow_c_array_validate_after_modify(SEXP array_xptr, SEXP schema_xptr);
 extern SEXP nanoarrow_c_array_set_schema(SEXP array_xptr, SEXP schema_xptr,
                                          SEXP validate_sexp);
@@ -121,6 +122,7 @@ static const R_CallMethodDef CallEntries[] = {
     {"nanoarrow_c_array_set_buffers", (DL_FUNC)&nanoarrow_c_array_set_buffers, 2},
     {"nanoarrow_c_array_set_children", (DL_FUNC)&nanoarrow_c_array_set_children, 2},
     {"nanoarrow_c_array_set_dictionary", (DL_FUNC)&nanoarrow_c_array_set_dictionary, 2},
+    {"nanoarrow_c_array_finish_building", (DL_FUNC)&nanoarrow_c_array_finish_building, 1},
     {"nanoarrow_c_array_validate_after_modify",
      (DL_FUNC)&nanoarrow_c_array_validate_after_modify, 2},
     {"nanoarrow_c_array_set_schema", (DL_FUNC)&nanoarrow_c_array_set_schema, 3},

r/tests/testthat/test-array.R

@@ -342,16 +342,52 @@ test_that("array modify can modify buffers", {
   array <- as_nanoarrow_array(1:5)
   expect_error(
     nanoarrow_array_modify(array, list(buffers = rep(list(NULL), 4))),
-    "must be <= 3"
+    "Changing the number of buffers in array_modify is not supported"
   )
 
   # Check that specifying too few buffers will result in a validation error
   expect_error(
     nanoarrow_array_modify(array, list(buffers = list()), validate = TRUE),
-    "Expected 2 buffer"
+    "Changing the number of buffers in array_modify is not supported"
   )
 })
 
+test_that("array modify can modify variadic buffers", {
+  # Create a string view array with >1 variadic buffers. The default
+  # internal threshold for splitting up internal buffers is ~30kb.
+  boring_strings <- vapply(
+    1:10000,
+    function(i) paste0("boring string", i),
+    character(1)
+  )
+
+  array <- as_nanoarrow_array(boring_strings, schema = na_string_view())
+  expect_identical(length(array$buffers), 9L)
+  variadic_sizes <- convert_buffer(array$buffers[[9]])
+  expect_identical(variadic_sizes, c(32757, 32759, 32759, 32759, 32759, 5101))
+  expect_identical(convert_array(array), boring_strings)
+
+  # Save the original array
+  original_array <- array
+
+  # Modify one of the array buffers
+  cool_string_bytes <- charToRaw("cooool string1")
+  first_buffer <- as.raw(array$buffers[[3]])
+  first_buffer[seq_along(cool_string_bytes)] <- cool_string_bytes
+
+  array$buffers[[3]] <- first_buffer
+
+  cool_strings <- boring_strings
+  cool_strings[1] <- "cooool string1"
+  expect_identical(convert_array(array), cool_strings)
+
+  variadic_sizes <- convert_buffer(array$buffers[[9]])
+  expect_identical(variadic_sizes, c(32757, 32759, 32759, 32759, 32759, 5101))
+
+  # Check that the original was unmodified
+  expect_identical(convert_array(original_array), boring_strings)
+})
+
 test_that("array modify checks buffer sizes", {
   array <- as_nanoarrow_array(1:5)
   expect_error(