Skip to content

Commit 13937c0

Browse files
authored
make struct_fields boxed, not arc'd (#7257)
StructFields are Arc'd inside, so double Arc'ing it makes no sense. Add C side tests for struct builder Signed-off-by: Mikhail Kot <to@myrrc.dev>
1 parent 59b0083 commit 13937c0

File tree

6 files changed

+91
-65
lines changed

6 files changed

+91
-65
lines changed

docs/api/c/dtypes.rst

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -78,9 +78,6 @@ Struct Fields
7878
.. c:autotype:: vx_struct_fields
7979
:file: vortex.h
8080

81-
.. c:autofunction:: vx_struct_fields_clone
82-
:file: vortex.h
83-
8481
.. c:autofunction:: vx_struct_fields_free
8582
:file: vortex.h
8683

vortex-ffi/cinclude/vortex.h

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -766,7 +766,7 @@ const vx_dtype *vx_dtype_new_fixed_size_list(const vx_dtype *element, uint32_t s
766766
*
767767
* Takes ownership of the `struct_dtype` pointer.
768768
*/
769-
const vx_dtype *vx_dtype_new_struct(const vx_struct_fields *struct_dtype, bool is_nullable);
769+
const vx_dtype *vx_dtype_new_struct(vx_struct_fields *struct_dtype, bool is_nullable);
770770

771771
/**
772772
* Create a new decimal data type.
@@ -1105,18 +1105,10 @@ size_t vx_string_len(const vx_string *ptr);
11051105
*/
11061106
const char *vx_string_ptr(const vx_string *ptr);
11071107

1108-
/**
1109-
* Clone a borrowed [`vx_struct_fields`], returning an owned [`vx_struct_fields`].
1110-
*
1111-
*
1112-
* Must be released with [`vx_struct_fields_free`].
1113-
*/
1114-
const vx_struct_fields *vx_struct_fields_clone(const vx_struct_fields *ptr);
1115-
11161108
/**
11171109
* Free an owned [`vx_struct_fields`] object.
11181110
*/
1119-
void vx_struct_fields_free(const vx_struct_fields *ptr);
1111+
void vx_struct_fields_free(vx_struct_fields *ptr);
11201112

11211113
/**
11221114
* 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,
11671159
*
11681160
* Takes ownership of the `builder`.
11691161
*/
1170-
const vx_struct_fields *vx_struct_fields_builder_finalize(vx_struct_fields_builder *builder);
1162+
vx_struct_fields *vx_struct_fields_builder_finalize(vx_struct_fields_builder *builder);
11711163

11721164
#ifdef __cplusplus
11731165
} // extern "C"

vortex-ffi/src/dtype.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -140,11 +140,11 @@ pub unsafe extern "C-unwind" fn vx_dtype_new_fixed_size_list(
140140
/// Takes ownership of the `struct_dtype` pointer.
141141
#[unsafe(no_mangle)]
142142
pub unsafe extern "C-unwind" fn vx_dtype_new_struct(
143-
struct_dtype: *const vx_struct_fields,
143+
struct_dtype: *mut vx_struct_fields,
144144
is_nullable: bool,
145145
) -> *const vx_dtype {
146-
let struct_dtype = vx_struct_fields::as_ref(struct_dtype).clone();
147-
vx_dtype::new(Arc::new(DType::Struct(struct_dtype, is_nullable.into())))
146+
let struct_dtype = vx_struct_fields::into_box(struct_dtype);
147+
vx_dtype::new(Arc::new(DType::Struct(*struct_dtype, is_nullable.into())))
148148
}
149149

150150
/// Create a new decimal data type.

vortex-ffi/src/struct_fields.rs

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,11 @@ use vortex::dtype::DType;
99
use vortex::dtype::StructFields;
1010
use vortex::error::VortexExpect;
1111

12-
use crate::arc_wrapper;
1312
use crate::box_wrapper;
1413
use crate::dtype::vx_dtype;
1514
use crate::string::vx_string;
1615

17-
arc_wrapper!(
16+
box_wrapper!(
1817
/// Represents a Vortex struct data type, without top-level nullability.
1918
StructFields,
2019
vx_struct_fields
@@ -119,8 +118,8 @@ pub unsafe extern "C-unwind" fn vx_struct_fields_builder_add_field(
119118
#[unsafe(no_mangle)]
120119
pub unsafe extern "C-unwind" fn vx_struct_fields_builder_finalize(
121120
builder: *mut vx_struct_fields_builder,
122-
) -> *const vx_struct_fields {
123-
let builder = vx_struct_fields_builder::into_box(builder);
124-
let struct_dtype = StructFields::new(builder.names.into(), builder.fields);
125-
vx_struct_fields::new(Arc::new(struct_dtype))
121+
) -> *mut vx_struct_fields {
122+
let StructDTypeBuilder { names, fields } = *vx_struct_fields_builder::into_box(builder);
123+
let struct_dtype = StructFields::new(names.into(), fields);
124+
vx_struct_fields::new(Box::new(struct_dtype))
126125
}

vortex-ffi/test/main.cpp

Lines changed: 1 addition & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// SPDX-License-Identifier: Apache-2.0
22
// SPDX-FileCopyrightText: Copyright the Vortex contributors
33
#include <catch2/catch_test_macros.hpp>
4-
4+
#include <unistd.h>
55
#include "vortex.h"
66

77
using namespace std::string_literals;
@@ -57,44 +57,3 @@ TEST_CASE("Creating dtypes", "[dtype]") {
5757

5858
vx_dtype_free(dtype);
5959
}
60-
61-
constexpr size_t STRUCT_LEN = 10;
62-
TEST_CASE("Creating structs", "[struct]") {
63-
vx_struct_fields_builder *builder = vx_struct_fields_builder_new();
64-
REQUIRE(builder != nullptr);
65-
66-
for (size_t i = 0; i < STRUCT_LEN; ++i) {
67-
const std::string target_name = "name"s + std::to_string(i);
68-
const vx_string *name = vx_string_new(target_name.data(), target_name.size());
69-
const vx_dtype *dtype = i % 2 ? vx_dtype_new_binary(false) : vx_dtype_new_primitive(PTYPE_F32, true);
70-
vx_struct_fields_builder_add_field(builder, name, dtype);
71-
}
72-
const vx_struct_fields *fields = vx_struct_fields_builder_finalize(builder);
73-
REQUIRE(fields != nullptr);
74-
75-
const size_t len = vx_struct_fields_nfields(fields);
76-
CHECK(len == STRUCT_LEN);
77-
for (size_t i = 0; i < len; ++i) {
78-
// borrowed
79-
const vx_string *name = vx_struct_fields_field_name(fields, i);
80-
// owned TODO(myrrc): that's weird API
81-
const vx_dtype *dtype = vx_struct_fields_field_dtype(fields, i);
82-
83-
std::string_view name_view {vx_string_ptr(name), vx_string_len(name)};
84-
std::string target_name = "name"s + std::to_string(i);
85-
86-
CHECK(name_view == target_name);
87-
88-
if (i % 2) {
89-
CHECK_FALSE(vx_dtype_is_nullable(dtype));
90-
CHECK(vx_dtype_get_variant(dtype) == DTYPE_BINARY);
91-
} else {
92-
CHECK(vx_dtype_is_nullable(dtype));
93-
CHECK(vx_dtype_get_variant(dtype) == DTYPE_PRIMITIVE);
94-
}
95-
96-
vx_dtype_free(dtype);
97-
}
98-
99-
vx_struct_fields_free(fields);
100-
}

vortex-ffi/test/struct.cpp

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
// SPDX-FileCopyrightText: Copyright the Vortex contributors
3+
#include <catch2/catch_test_macros.hpp>
4+
#include <vortex.h>
5+
6+
using namespace std::string_view_literals;
7+
using namespace std::string_literals;
8+
9+
TEST_CASE("Struct builder", "[struct]") {
10+
vx_struct_fields_builder *builder = vx_struct_fields_builder_new();
11+
12+
constexpr auto col1 = "col1"sv;
13+
const vx_string *col1_name = vx_string_new(col1.data(), col1.size());
14+
const vx_dtype *col1_dtype = vx_dtype_new_primitive(PTYPE_U8, false);
15+
vx_struct_fields_builder_add_field(builder, col1_name, col1_dtype);
16+
17+
constexpr auto col2 = "col2"sv;
18+
const vx_string *col2_name = vx_string_new(col2.data(), col2.size());
19+
const vx_dtype *col2_dtype = vx_dtype_new_binary(true);
20+
vx_struct_fields_builder_add_field(builder, col2_name, col2_dtype);
21+
22+
SECTION("Struct builder free") {
23+
vx_struct_fields_builder_free(builder);
24+
}
25+
26+
SECTION("Struct builder finalize") {
27+
vx_struct_fields *fields = vx_struct_fields_builder_finalize(builder);
28+
29+
SECTION("struct fields free") {
30+
vx_struct_fields_free(fields);
31+
}
32+
33+
SECTION("struct fields finalize") {
34+
const vx_dtype *dtype = vx_dtype_new_struct(fields, false);
35+
vx_dtype_free(dtype);
36+
}
37+
}
38+
}
39+
40+
constexpr size_t STRUCT_LEN = 10;
41+
TEST_CASE("Creating structs", "[struct]") {
42+
vx_struct_fields_builder *builder = vx_struct_fields_builder_new();
43+
REQUIRE(builder != nullptr);
44+
45+
for (size_t i = 0; i < STRUCT_LEN; ++i) {
46+
const std::string target_name = "name"s + std::to_string(i);
47+
const vx_string *name = vx_string_new(target_name.data(), target_name.size());
48+
const vx_dtype *dtype = i % 2 ? vx_dtype_new_binary(false) : vx_dtype_new_primitive(PTYPE_F32, true);
49+
vx_struct_fields_builder_add_field(builder, name, dtype);
50+
}
51+
vx_struct_fields *fields = vx_struct_fields_builder_finalize(builder);
52+
REQUIRE(fields != nullptr);
53+
54+
const size_t len = vx_struct_fields_nfields(fields);
55+
CHECK(len == STRUCT_LEN);
56+
for (size_t i = 0; i < len; ++i) {
57+
// borrowed
58+
const vx_string *name = vx_struct_fields_field_name(fields, i);
59+
// owned TODO(myrrc): that's weird API
60+
const vx_dtype *dtype = vx_struct_fields_field_dtype(fields, i);
61+
62+
std::string_view name_view {vx_string_ptr(name), vx_string_len(name)};
63+
std::string target_name = "name"s + std::to_string(i);
64+
65+
CHECK(name_view == target_name);
66+
67+
if (i % 2) {
68+
CHECK_FALSE(vx_dtype_is_nullable(dtype));
69+
CHECK(vx_dtype_get_variant(dtype) == DTYPE_BINARY);
70+
} else {
71+
CHECK(vx_dtype_is_nullable(dtype));
72+
CHECK(vx_dtype_get_variant(dtype) == DTYPE_PRIMITIVE);
73+
}
74+
75+
vx_dtype_free(dtype);
76+
}
77+
78+
vx_struct_fields_free(fields);
79+
}

0 commit comments

Comments
 (0)