make struct_fields boxed, not arc'd

docs/api/c/dtypes.rst

@@ -78,9 +78,6 @@ Struct Fields
 .. c:autotype:: vx_struct_fields
     :file: vortex.h
 
-.. c:autofunction:: vx_struct_fields_clone
-    :file: vortex.h
-
 .. c:autofunction:: vx_struct_fields_free
     :file: vortex.h
 

vortex-ffi/cinclude/vortex.h

@@ -766,7 +766,7 @@ const vx_dtype *vx_dtype_new_fixed_size_list(const vx_dtype *element, uint32_t s
  *
  * Takes ownership of the `struct_dtype` pointer.
  */
-const vx_dtype *vx_dtype_new_struct(const vx_struct_fields *struct_dtype, bool is_nullable);
+const vx_dtype *vx_dtype_new_struct(vx_struct_fields *struct_dtype, bool is_nullable);
 
 /**
  * Create a new decimal data type.
@@ -1105,18 +1105,10 @@ size_t vx_string_len(const vx_string *ptr);
  */
 const char *vx_string_ptr(const vx_string *ptr);
 
-/**
- * Clone a borrowed [`vx_struct_fields`], returning an owned [`vx_struct_fields`].
- *
- *
- * Must be released with [`vx_struct_fields_free`].
- */
-const vx_struct_fields *vx_struct_fields_clone(const vx_struct_fields *ptr);
-
 /**
  * Free an owned [`vx_struct_fields`] object.
  */
-void vx_struct_fields_free(const vx_struct_fields *ptr);
+void vx_struct_fields_free(vx_struct_fields *ptr);
 
 /**
  * Return the number of fields in the struct dtype.
@@ -1167,7 +1159,7 @@ void vx_struct_fields_builder_add_field(vx_struct_fields_builder *builder,
  *
  * Takes ownership of the `builder`.
  */
-const vx_struct_fields *vx_struct_fields_builder_finalize(vx_struct_fields_builder *builder);
+vx_struct_fields *vx_struct_fields_builder_finalize(vx_struct_fields_builder *builder);
 
 #ifdef __cplusplus
 } // extern "C"

vortex-ffi/src/dtype.rs

@@ -140,11 +140,11 @@ pub unsafe extern "C-unwind" fn vx_dtype_new_fixed_size_list(
 /// Takes ownership of the `struct_dtype` pointer.
 #[unsafe(no_mangle)]
 pub unsafe extern "C-unwind" fn vx_dtype_new_struct(
-    struct_dtype: *const vx_struct_fields,
+    struct_dtype: *mut vx_struct_fields,
     is_nullable: bool,
 ) -> *const vx_dtype {
-    let struct_dtype = vx_struct_fields::as_ref(struct_dtype).clone();
-    vx_dtype::new(Arc::new(DType::Struct(struct_dtype, is_nullable.into())))
+    let struct_dtype = vx_struct_fields::into_box(struct_dtype);
+    vx_dtype::new(Arc::new(DType::Struct(*struct_dtype, is_nullable.into())))
 }
 
 /// Create a new decimal data type.

vortex-ffi/src/struct_fields.rs

@@ -9,12 +9,11 @@ use vortex::dtype::DType;
 use vortex::dtype::StructFields;
 use vortex::error::VortexExpect;
 
-use crate::arc_wrapper;
 use crate::box_wrapper;
 use crate::dtype::vx_dtype;
 use crate::string::vx_string;
 
-arc_wrapper!(
+box_wrapper!(
     /// Represents a Vortex struct data type, without top-level nullability.
     StructFields,
     vx_struct_fields
@@ -119,8 +118,8 @@ pub unsafe extern "C-unwind" fn vx_struct_fields_builder_add_field(
 #[unsafe(no_mangle)]
 pub unsafe extern "C-unwind" fn vx_struct_fields_builder_finalize(
     builder: *mut vx_struct_fields_builder,
-) -> *const vx_struct_fields {
-    let builder = vx_struct_fields_builder::into_box(builder);
-    let struct_dtype = StructFields::new(builder.names.into(), builder.fields);
-    vx_struct_fields::new(Arc::new(struct_dtype))
+) -> *mut vx_struct_fields {
+    let StructDTypeBuilder { names, fields } = *vx_struct_fields_builder::into_box(builder);
+    let struct_dtype = StructFields::new(names.into(), fields);
+    vx_struct_fields::new(Box::new(struct_dtype))
 }

vortex-ffi/test/main.cpp

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: Apache-2.0
 // SPDX-FileCopyrightText: Copyright the Vortex contributors
 #include <catch2/catch_test_macros.hpp>
-
+#include <unistd.h>
 #include "vortex.h"
 
 using namespace std::string_literals;
@@ -57,44 +57,3 @@ TEST_CASE("Creating dtypes", "[dtype]") {
 
     vx_dtype_free(dtype);
 }
-
-constexpr size_t STRUCT_LEN = 10;
-TEST_CASE("Creating structs", "[struct]") {
-    vx_struct_fields_builder *builder = vx_struct_fields_builder_new();
-    REQUIRE(builder != nullptr);
-
-    for (size_t i = 0; i < STRUCT_LEN; ++i) {
-        const std::string target_name = "name"s + std::to_string(i);
-        const vx_string *name = vx_string_new(target_name.data(), target_name.size());
-        const vx_dtype *dtype = i % 2 ? vx_dtype_new_binary(false) : vx_dtype_new_primitive(PTYPE_F32, true);
-        vx_struct_fields_builder_add_field(builder, name, dtype);
-    }
-    const vx_struct_fields *fields = vx_struct_fields_builder_finalize(builder);
-    REQUIRE(fields != nullptr);
-
-    const size_t len = vx_struct_fields_nfields(fields);
-    CHECK(len == STRUCT_LEN);
-    for (size_t i = 0; i < len; ++i) {
-        // borrowed
-        const vx_string *name = vx_struct_fields_field_name(fields, i);
-        // owned TODO(myrrc): that's weird API
-        const vx_dtype *dtype = vx_struct_fields_field_dtype(fields, i);
-
-        std::string_view name_view {vx_string_ptr(name), vx_string_len(name)};
-        std::string target_name = "name"s + std::to_string(i);
-
-        CHECK(name_view == target_name);
-
-        if (i % 2) {
-            CHECK_FALSE(vx_dtype_is_nullable(dtype));
-            CHECK(vx_dtype_get_variant(dtype) == DTYPE_BINARY);
-        } else {
-            CHECK(vx_dtype_is_nullable(dtype));
-            CHECK(vx_dtype_get_variant(dtype) == DTYPE_PRIMITIVE);
-        }
-
-        vx_dtype_free(dtype);
-    }
-
-    vx_struct_fields_free(fields);
-}

vortex-ffi/test/struct.cpp

@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: Apache-2.0
+// SPDX-FileCopyrightText: Copyright the Vortex contributors
+#include <catch2/catch_test_macros.hpp>
+#include <vortex.h>
+
+using namespace std::string_view_literals;
+using namespace std::string_literals;
+
+TEST_CASE("Struct builder", "[struct]") {
+    vx_struct_fields_builder *builder = vx_struct_fields_builder_new();
+
+    constexpr auto col1 = "col1"sv;
+    const vx_string *col1_name = vx_string_new(col1.data(), col1.size());
+    const vx_dtype *col1_dtype = vx_dtype_new_primitive(PTYPE_U8, false);
+    vx_struct_fields_builder_add_field(builder, col1_name, col1_dtype);
+
+    constexpr auto col2 = "col2"sv;
+    const vx_string *col2_name = vx_string_new(col2.data(), col2.size());
+    const vx_dtype *col2_dtype = vx_dtype_new_binary(true);
+    vx_struct_fields_builder_add_field(builder, col2_name, col2_dtype);
+
+    SECTION("Struct builder free") {
+        vx_struct_fields_builder_free(builder);
+    }
+
+    SECTION("Struct builder finalize") {
+        vx_struct_fields *fields = vx_struct_fields_builder_finalize(builder);
+
+        SECTION("struct fields free") {
+            vx_struct_fields_free(fields);
+        }
+
+        SECTION("struct fields finalize") {
+            const vx_dtype *dtype = vx_dtype_new_struct(fields, false);
+            vx_dtype_free(dtype);
+        }
+    }
+}
+
+constexpr size_t STRUCT_LEN = 10;
+TEST_CASE("Creating structs", "[struct]") {
+    vx_struct_fields_builder *builder = vx_struct_fields_builder_new();
+    REQUIRE(builder != nullptr);
+
+    for (size_t i = 0; i < STRUCT_LEN; ++i) {
+        const std::string target_name = "name"s + std::to_string(i);
+        const vx_string *name = vx_string_new(target_name.data(), target_name.size());
+        const vx_dtype *dtype = i % 2 ? vx_dtype_new_binary(false) : vx_dtype_new_primitive(PTYPE_F32, true);
+        vx_struct_fields_builder_add_field(builder, name, dtype);
+    }
+    vx_struct_fields *fields = vx_struct_fields_builder_finalize(builder);
+    REQUIRE(fields != nullptr);
+
+    const size_t len = vx_struct_fields_nfields(fields);
+    CHECK(len == STRUCT_LEN);
+    for (size_t i = 0; i < len; ++i) {
+        // borrowed
+        const vx_string *name = vx_struct_fields_field_name(fields, i);
+        // owned TODO(myrrc): that's weird API
+        const vx_dtype *dtype = vx_struct_fields_field_dtype(fields, i);
+
+        std::string_view name_view {vx_string_ptr(name), vx_string_len(name)};
+        std::string target_name = "name"s + std::to_string(i);
+
+        CHECK(name_view == target_name);
+
+        if (i % 2) {
+            CHECK_FALSE(vx_dtype_is_nullable(dtype));
+            CHECK(vx_dtype_get_variant(dtype) == DTYPE_BINARY);
+        } else {
+            CHECK(vx_dtype_is_nullable(dtype));
+            CHECK(vx_dtype_get_variant(dtype) == DTYPE_PRIMITIVE);
+        }
+
+        vx_dtype_free(dtype);
+    }
+
+    vx_struct_fields_free(fields);
+}