diff --git a/.github/workflows/rust-instrumented.yml b/.github/workflows/rust-instrumented.yml index bf45fc7be13..5430f1512fc 100644 --- a/.github/workflows/rust-instrumented.yml +++ b/.github/workflows/rust-instrumented.yml @@ -194,7 +194,7 @@ jobs: - sanitizer: tsan sanitizer_flags: "-Zsanitizer=thread" name: "Rust/C++ FFI tests (${{ matrix.sanitizer }})" - timeout-minutes: 30 + timeout-minutes: 5 env: ASAN_OPTIONS: "symbolize=1:check_initialization_order=1:detect_leaks=1:leak_check_at_exit=1" LSAN_OPTIONS: "report_objects=1" @@ -217,9 +217,6 @@ jobs: sccache: s3 - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 - uses: ./.github/actions/setup-prebuild - - name: Install rustfilt - run: | - cargo install rustfilt - name: Install Rust nightly toolchain run: | rustup toolchain install $NIGHTLY_TOOLCHAIN @@ -241,8 +238,7 @@ jobs: - name: Run tests run: | set -o pipefail -# re-enable once we can fix this hang -# ./vortex-ffi/build/test/vortex_ffi_test 2>&1 | rustfilt + ./vortex-ffi/build/test/vortex_ffi_test - name: Run examples run: | set -o pipefail @@ -251,11 +247,11 @@ jobs: # error: Unable to walk dir: File system loop found rm -fr vortex-ffi/build/_deps/nanoarrow-src/python - ./vortex-ffi/build/examples/write_sample file.vortex 2>&1 | rustfilt - ./vortex-ffi/build/examples/write_sample file2.vortex 2>&1 | rustfilt - ./vortex-ffi/build/examples/dtype '*.vortex' 2>&1 | rustfilt - ./vortex-ffi/build/examples/scan '*.vortex' 2>&1 | rustfilt - ./vortex-ffi/build/examples/scan_to_arrow '*.vortex' 2>&1 | rustfilt + ./vortex-ffi/build/examples/write_sample file.vortex + ./vortex-ffi/build/examples/write_sample file2.vortex + ./vortex-ffi/build/examples/dtype '*.vortex' + ./vortex-ffi/build/examples/scan '*.vortex' + ./vortex-ffi/build/examples/scan_to_arrow '*.vortex' miri: name: "Rust tests (miri)" diff --git a/vortex-ffi/README.md b/vortex-ffi/README.md index f1273ae5527..ff3b6c94ff2 100644 --- a/vortex-ffi/README.md +++ b/vortex-ffi/README.md @@ -1,4 +1,4 @@ -# Vortex C interface +# Vortex C bindings ## Updating Headers @@ -20,7 +20,7 @@ target_link_libraries(my_target, vortex_ffi_shared) # or target_link_libraries(my_target, vortex_ffi) ``` -## Running C examples: +## Running C examples ```sh cmake -Bbuild -DBUILD_EXAMPLES=1 @@ -99,7 +99,7 @@ cargo +nightly build -Zbuild-std --target= \ 2. Build tests with target triple: ```sh -cmake -Bbuild -DWITH_ASAN=1 -DTARGET_TRIPLE= +cmake -Bbuild -DSANITIZER=asan -DTARGET_TRIPLE= ``` 3. Run the tests (ctest doesn't output failures in detail): diff --git a/vortex-ffi/cbindgen.toml b/vortex-ffi/cbindgen.toml index d1b4ffa5f48..7d28dacda63 100644 --- a/vortex-ffi/cbindgen.toml +++ b/vortex-ffi/cbindgen.toml @@ -11,9 +11,11 @@ header = """ #pragma once #include -// // THIS FILE IS AUTO-GENERATED, DO NOT MAKE EDITS DIRECTLY -// + +// All operations return owned types which need to be freed by calling a +// matching _free() function. This includes all arrays, data sources, scans, +// errors, error messages, and other allocated objects. // https://arrow.apache.org/docs/format/CDataInterface.html#structure-definitions // If you want to use your own Arrow library like nanoarrow, define this macro diff --git a/vortex-ffi/cinclude/vortex.h b/vortex-ffi/cinclude/vortex.h index 0203202e437..277985362b2 100644 --- a/vortex-ffi/cinclude/vortex.h +++ b/vortex-ffi/cinclude/vortex.h @@ -3,9 +3,11 @@ #pragma once #include -// // THIS FILE IS AUTO-GENERATED, DO NOT MAKE EDITS DIRECTLY -// + +// All operations return owned types which need to be freed by calling a +// matching _free() function. This includes all arrays, data sources, scans, +// errors, error messages, and other allocated objects. // https://arrow.apache.org/docs/format/CDataInterface.html#structure-definitions // If you want to use your own Arrow library like nanoarrow, define this macro @@ -410,7 +412,7 @@ typedef struct Primitive Primitive; * array is a cheap operation. * * Unless stated explicitly, all operations with vx_array don't take - * ownership of it, and thus it must be freed by the caller. + * ownership of it, and thus the array must be freed by the caller. */ typedef struct vx_array vx_array; @@ -475,8 +477,6 @@ typedef struct vx_error vx_error; * data. Each expression consists of an encoding (vtable), heap-allocated * metadata, and child expressions. * - * Unless stated explicitly, all expressions returned are owned and must - * be freed by the caller. * Unless stated explicitly, if an operation on const vx_expression* is * passed NULL, NULL is returned. * Operations on expressions don't take ownership of input values, and so @@ -629,8 +629,7 @@ extern "C" { #endif // __cplusplus /** - * Clone a borrowed [`vx_array`], returning an owned [`vx_array`]. - * Must be released with [`vx_array_free`]. + * Clone a vx_array */ const vx_array *vx_array_clone(const vx_array *ptr); @@ -677,10 +676,7 @@ void vx_array_get_validity(const vx_array *array, vx_validity *validity, vx_erro size_t vx_array_len(const vx_array *array); /** - * Get the [`struct@crate::dtype::vx_dtype`] of the array. - * - * The returned pointer is valid as long as the array is valid. - * Do NOT free the returned dtype pointer - it shares the lifetime of the array. + * Get array's dtype */ const vx_dtype *vx_array_dtype(const vx_array *array); @@ -738,9 +734,6 @@ const vx_array *vx_array_new_primitive(vx_ptype ptype, * `nullable` controls the top-level nullability of the resulting array's dtype. For an Arrow * record batch (which has no top-level validity) pass `false`. * - * The imported buffers are referenced zero-copy where possible; the returned array keeps the - * Arrow data alive until it is freed with [`vx_array_free`]. - * * On error, returns NULL and sets `error_out`. * * Example: @@ -820,7 +813,7 @@ const vx_array *vx_array_apply(const vx_array *array, const vx_expression *expre /** * Free an owned [`vx_array_iterator`] object. */ -void vx_array_iterator_free(vx_array_iterator *ptr); +void vx_array_iterator_free(const vx_array_iterator *ptr); /** * Attempt to advance the `current` pointer of the iterator. @@ -833,13 +826,12 @@ void vx_array_iterator_free(vx_array_iterator *ptr); const vx_array *vx_array_iterator_next(vx_array_iterator *iter, vx_error **error_out); /** - * Clone a borrowed [`vx_binary`], returning an owned [`vx_binary`]. - * Must be released with [`vx_binary_free`]. + * Clone a vx_binary. Returned handle must be release with vx_binary_free */ const vx_binary *vx_binary_clone(const vx_binary *ptr); /** - * Free an owned [`vx_binary`] object. + * Free a vx_binary */ void vx_binary_free(const vx_binary *ptr); @@ -859,21 +851,19 @@ size_t vx_binary_len(const vx_binary *ptr); const char *vx_binary_ptr(const vx_binary *ptr); /** - * Clone a borrowed [`vx_data_source`], returning an owned [`vx_data_source`]. - * Must be released with [`vx_data_source_free`]. + * Clone a vx_data_source. Returned handle must be release with vx_data_source_free */ const vx_data_source *vx_data_source_clone(const vx_data_source *ptr); /** - * Free an owned [`vx_data_source`] object. + * Free a vx_data_source */ void vx_data_source_free(const vx_data_source *ptr); /** * Create a data source. * The first matched file is opened eagerly. to read the schema. All other I/O - * is deferred until a scan is requested. The returned pointer is owned by the - * caller and must be freed with vx_data_source_free. + * is deferred until a scan is requested. * * On error, returns NULL and sets "err". */ @@ -887,17 +877,13 @@ vx_data_source_new(const vx_session *session, const vx_data_source_options *opti * The bytes are borrowed, not copied: the caller must keep "buffer" alive and * unmodified until the data source is freed. * - * The returned pointer is owned by the caller and must be freed with - * vx_data_source_free. - * * On error, returns NULL and sets "err". */ const vx_data_source * vx_data_source_new_buffer(const vx_session *session, const void *buffer, size_t buffer_len, vx_error **err); /** - * Return the schema of the data source as a non-owned dtype. - * The returned pointer is valid as long as "ds" is alive. Do not free it. + * Return data source's dtype */ const vx_dtype *vx_data_source_dtype(const vx_data_source *ds); @@ -907,8 +893,7 @@ const vx_dtype *vx_data_source_dtype(const vx_data_source *ds); void vx_data_source_get_row_count(const vx_data_source *ds, vx_estimate *row_count); /** - * Clone a borrowed [`vx_dtype`], returning an owned [`vx_dtype`]. - * Must be released with [`vx_dtype_free`]. + * Clone a vx_dtype */ const vx_dtype *vx_dtype_clone(const vx_dtype *ptr); @@ -994,26 +979,21 @@ uint8_t vx_dtype_decimal_precision(const vx_dtype *dtype); int8_t vx_dtype_decimal_scale(const vx_dtype *dtype); /** - * Return a borrowed reference to the [`vx_struct_fields`] of a struct. - * - * The returned pointer is valid as long as the struct dtype is valid. - * Do NOT free the returned pointer - it shares the lifetime of the struct dtype. + * If "dtype" is DTYPE_STRUCT, return owned vx_struct_fields for this struct, + * return NULL otherwise. Returned vx_struct_fields must be released with + * vx_dtype_free. */ const vx_struct_fields *vx_dtype_struct_dtype(const vx_dtype *dtype); /** - * Returns the element type of a list. - * - * The returned pointer is valid as long as the list dtype is valid. - * Do NOT free the returned dtype pointer - it shares the lifetime of the list dtype. + * If "dtype" is DTYPE_LIST, return its owned element dtype, return NULL + * otherwise. Returned dtype must be released with vx_dtype_free. */ const vx_dtype *vx_dtype_list_element(const vx_dtype *dtype); /** - * Returns the element type of a fixed-size list. - * - * The returned pointer is valid as long as the fixed-size list dtype is valid. - * Do NOT free the returned dtype pointer - it shares the lifetime of the fixed-size list dtype. + * If "dtype" is DTYPE_FIXED_SIZE_LIST, return its owned element dtype, return + * NULL otherwise. Returned dtype must be released with vx_dtype_free. */ const vx_dtype *vx_dtype_fixed_size_list_element(const vx_dtype *dtype); @@ -1069,20 +1049,17 @@ const vx_dtype *vx_dtype_from_arrow_schema(FFI_ArrowSchema *schema, vx_error **e /** * Free an owned [`vx_error`] object. */ -void vx_error_free(vx_error *ptr); +void vx_error_free(const vx_error *ptr); /** - * Returns the error message from the given Vortex error. - * - * The returned pointer is valid as long as the error is valid. - * Do NOT free the returned string pointer - it shares the lifetime of the error. + * Return an error message for this error */ const vx_string *vx_error_get_message(const vx_error *error); /** * Free an owned [`vx_expression`] object. */ -void vx_expression_free(vx_expression *ptr); +void vx_expression_free(const vx_expression *ptr); /** * Create a root expression. A root expression, applied to an array in @@ -1223,8 +1200,7 @@ vx_expression *vx_expression_get_item(const char *item, const vx_expression *chi vx_expression *vx_expression_list_contains(const vx_expression *list, const vx_expression *value); /** - * Clone a borrowed [`vx_file`], returning an owned [`vx_file`]. - * Must be released with [`vx_file_free`]. + * Clone a vx_file */ const vx_file *vx_file_clone(const vx_file *ptr); @@ -1248,22 +1224,17 @@ void vx_set_log_level(vx_log_level level); /** * Free an owned [`vx_scalar`] object. */ -void vx_scalar_free(vx_scalar *ptr); +void vx_scalar_free(const vx_scalar *ptr); /** - * Clone a borrowed scalar handle. - * - * The input scalar handle is not consumed. The returned scalar handle must be - * released with vx_scalar_free. Returns NULL when given a NULL scalar handle. + * Clone a scalar handle. + * If scalar is NULL, returns NULL. */ vx_scalar *vx_scalar_clone(const vx_scalar *scalar); /** - * Return the data type of a scalar. - * - * The returned data type handle borrows storage from the scalar handle, so its - * lifetime is bound to the scalar handle. It MUST NOT be freed separately. - * Returns NULL when given a NULL scalar handle. + * Return scalar's dtype. + * If scalar is NULL, returns NULL. */ const vx_dtype *vx_scalar_dtype(const vx_scalar *scalar); @@ -1358,9 +1329,11 @@ vx_scalar *vx_scalar_new_binary(const uint8_t *ptr, size_t len, bool is_nullable /** * Create a typed null scalar. * - * The data type handle is borrowed, not consumed. The returned scalar uses a - * nullable copy of that logical type, regardless of the input type's top-level - * nullability. A NULL data type handle returns NULL and writes the error output. + * "dtype" is not consumed, you can use it after calling this function. Returned + * scalar uses a nullable copy of that logical type, regardless of the input + * type's top-level nullability. + * + * Returns NULL and sets "err" on error or NULL dtype. */ vx_scalar *vx_scalar_new_null(const vx_dtype *dtype, vx_error **err); @@ -1435,10 +1408,8 @@ vx_scalar *vx_scalar_new_decimal_i256_le(const uint8_t *bytes32, /** * Create a list scalar. * - * The element data type handle is borrowed, not consumed. Child scalar handles - * are cloned into the list value, so the caller keeps ownership of the handle - * array and each scalar in it. A NULL child handle array is allowed only for an - * empty list. Child values are validated against the element logical type. + * "element_dtype" and "elements" are not consumed, you can use them after + * calling this function. If len is 0, you can pass NULL to "elements". */ vx_scalar *vx_scalar_new_list(const vx_dtype *element_dtype, const vx_scalar *const *elements, @@ -1449,27 +1420,21 @@ vx_scalar *vx_scalar_new_list(const vx_dtype *element_dtype, /** * Create a fixed-size list scalar. * - * The element data type handle is borrowed, not consumed. The number of child - * scalars becomes the fixed-size list width and must fit in a 32-bit unsigned - * integer. Child scalar handles are cloned into the list value, so the caller - * keeps ownership of the handle array and each scalar in it. A NULL child - * handle array is allowed only for an empty list. Child values are validated - * against the element logical type. + * "element_dtype" and "elements" are not consumed, you can use them after + * calling this function. If len is 0, you can pass NULL to "elements". + * "len" must fit in uint32_t. */ vx_scalar *vx_scalar_new_fixed_size_list(const vx_dtype *element_dtype, const vx_scalar *const *elements, - size_t len, + uint32_t len, bool is_nullable, vx_error **err); /** * Create a struct scalar. * - * The struct data type handle is borrowed, not consumed. Field scalar handles - * are cloned into the struct value, so the caller keeps ownership of the handle - * array and each scalar in it. Field count and field logical types are validated - * against the struct logical type. A NULL field handle array is allowed only for - * an empty struct value. + * "struct_dtype" and "fields" are not consumed, you can use them after calling + * this function. If len is 0, you can pass NULL to "fields". */ vx_scalar *vx_scalar_new_struct(const vx_dtype *struct_dtype, const vx_scalar *const *fields, @@ -1479,19 +1444,17 @@ vx_scalar *vx_scalar_new_struct(const vx_dtype *struct_dtype, /** * Free an owned [`vx_scan`] object. */ -void vx_scan_free(vx_scan *ptr); +void vx_scan_free(const vx_scan *ptr); /** * Free an owned [`vx_partition`] object. */ -void vx_partition_free(vx_partition *ptr); +void vx_partition_free(const vx_partition *ptr); /** * Scan a data source. * - * Return an owned scan that must be freed with vx_scan_free. A scan may be - * consumed only once. - * + * A scan may be consumed only once. * "options" and "estimate" may be NULL. * * If "options" is NULL, all rows and columns are returned. @@ -1506,17 +1469,14 @@ vx_scan *vx_data_source_scan(const vx_data_source *data_source, vx_error **err); /** - * Return borrowed vx_scan's dtype. + * Return scan's dtype. * This function will fail if called after vx_scan_next_partition. - * Called must not free the returned pointer as its lifetime is bound to the - * lifetime of the scan. * On error returns NULL and sets "err". */ const vx_dtype *vx_scan_dtype(const vx_scan *scan, vx_error **err); /** - * Return an owned partition from a scan. - * The returned partition must be freed with vx_partition_free. + * Return an partition from a scan. * * On success returns a partition. * On exhaustion (no more partitions in scan) returns NULL but doesn't set @@ -1555,8 +1515,7 @@ int vx_partition_scan_arrow(const vx_session *session, vx_error **err); /** - * Return an owned owned array from a partition. - * The returned array must be freed with vx_array_free. + * Return an array from a partition. * * On success returns an array. * On exhaustion (no more arrays in partition) returns NULL but doesn't set @@ -1570,7 +1529,7 @@ const vx_array *vx_partition_next(vx_partition *partition, vx_error **err); /** * Free an owned [`vx_session`] object. */ -void vx_session_free(vx_session *ptr); +void vx_session_free(const vx_session *ptr); /** * Create a new Vortex session. @@ -1608,13 +1567,12 @@ void vx_array_sink_push(vx_array_sink *sink, const vx_array *array, vx_error **e void vx_array_sink_close(vx_array_sink *sink, vx_error **error_out); /** - * Clone a borrowed [`vx_string`], returning an owned [`vx_string`]. - * Must be released with [`vx_string_free`]. + * Clone a vx_string. Returned handle must be release with vx_string_free */ const vx_string *vx_string_clone(const vx_string *ptr); /** - * Free an owned [`vx_string`] object. + * Free a vx_string */ void vx_string_free(const vx_string *ptr); @@ -1641,7 +1599,7 @@ const char *vx_string_ptr(const vx_string *ptr); /** * Free an owned [`vx_struct_column_builder`] object. */ -void vx_struct_column_builder_free(vx_struct_column_builder *ptr); +void vx_struct_column_builder_free(const vx_struct_column_builder *ptr); /** * Create a new column-wise struct array builder with given validity and a @@ -1692,7 +1650,7 @@ const vx_array *vx_struct_column_builder_finalize(vx_struct_column_builder *buil /** * Free an owned [`vx_struct_fields`] object. */ -void vx_struct_fields_free(vx_struct_fields *ptr); +void vx_struct_fields_free(const vx_struct_fields *ptr); /** * Return the number of fields in the struct dtype. @@ -1700,28 +1658,21 @@ void vx_struct_fields_free(vx_struct_fields *ptr); uint64_t vx_struct_fields_nfields(const vx_struct_fields *dtype); /** - * Return a borrowed reference to the name of the field at the given index. - * - * The returned pointer is valid as long as the struct fields is valid. - * Do NOT free the returned string pointer - it shares the lifetime of the struct fields. - * Returns null if the index is out of bounds. + * Return an owned name of the field at a given index. + * If index is out of bounds, returns NULL. */ const vx_string *vx_struct_fields_field_name(const vx_struct_fields *dtype, size_t idx); /** - * Returns an *owned* reference to the dtype of the field at the given index. - * - * The return type is owned since struct dtypes can be lazily parsed from a binary format, in - * which case it's not possible to return a borrowed reference to the field dtype. - * - * Returns null if the index is out of bounds or if the field dtype cannot be parsed. + * Return an owned dtype of the field at a given index. + * Returns NULL if index is out of bounds or if dtype cannot be parsed. */ const vx_dtype *vx_struct_fields_field_dtype(const vx_struct_fields *dtype, size_t idx); /** * Free an owned [`vx_struct_fields_builder`] object. */ -void vx_struct_fields_builder_free(vx_struct_fields_builder *ptr); +void vx_struct_fields_builder_free(const vx_struct_fields_builder *ptr); /** * Create a new struct dtype builder. diff --git a/vortex-ffi/examples/dtype.c b/vortex-ffi/examples/dtype.c index 007c3993e33..1a319a80eb5 100644 --- a/vortex-ffi/examples/dtype.c +++ b/vortex-ffi/examples/dtype.c @@ -61,9 +61,12 @@ void print_struct_dtype(const vx_dtype *dtype) { const vx_string *field_name = vx_struct_fields_field_name(fields, i); printf(" %.*s = ", (int)vx_string_len(field_name), vx_string_ptr(field_name)); print_dtype(field_dtype); + vx_string_free(field_name); vx_dtype_free(field_dtype); } printf(")"); + + vx_struct_fields_free(fields); } void print_list_dtype(const vx_dtype *dtype) { @@ -148,7 +151,9 @@ int main(int argc, char **argv) { } printf("dtype: "); - print_dtype(vx_data_source_dtype(data_source)); + const vx_dtype* dtype = vx_data_source_dtype(data_source); + print_dtype(dtype); + vx_dtype_free(dtype); vx_data_source_free(data_source); vx_session_free(session); diff --git a/vortex-ffi/examples/scan_to_arrow.c b/vortex-ffi/examples/scan_to_arrow.c index ffa83d0b123..988974aab13 100644 --- a/vortex-ffi/examples/scan_to_arrow.c +++ b/vortex-ffi/examples/scan_to_arrow.c @@ -23,7 +23,6 @@ void print_error(const char *what, const vx_error *error) { void execute_scan(vx_session *session, vx_scan *scan) { vx_error *error = NULL; - // Returned dtype is owned and mustn't be freed const vx_dtype *dtype = vx_scan_dtype(scan, &error); if (dtype == NULL) { print_error("Failed to get scan dtype", error); @@ -37,6 +36,7 @@ void execute_scan(vx_session *session, vx_scan *scan) { vx_error_free(error); return; } + vx_dtype_free(dtype); char schema_buf[1024 * 10]; const int schema_len = ArrowSchemaToString(&schema, schema_buf, sizeof schema_buf, 1); diff --git a/vortex-ffi/src/array.rs b/vortex-ffi/src/array.rs index 8ab74ef8262..8b3a59ada74 100644 --- a/vortex-ffi/src/array.rs +++ b/vortex-ffi/src/array.rs @@ -55,7 +55,7 @@ arc_wrapper!( /// array is a cheap operation. /// /// Unless stated explicitly, all operations with vx_array don't take - /// ownership of it, and thus it must be freed by the caller. + /// ownership of it, and thus the array must be freed by the caller. ArrayRef, vx_array ); @@ -206,16 +206,13 @@ pub unsafe extern "C-unwind" fn vx_array_len(array: *const vx_array) -> usize { vx_array::as_ref(array).len() } -/// Get the [`struct@crate::dtype::vx_dtype`] of the array. -/// -/// The returned pointer is valid as long as the array is valid. -/// Do NOT free the returned dtype pointer - it shares the lifetime of the array. +/// Get array's dtype #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn vx_array_dtype(array: *const vx_array) -> *const vx_dtype { - vx_dtype::new_ref(vx_array::as_ref(array).dtype()) + vx_dtype::new(Arc::new(vx_array::as_ref(array).dtype().clone())) } -// Return an owned field for array at index. +// Return a field for array at index. // Returns NULL and sets error_out if index is out of bounds or array doesn't // have dtype DTYPE_STRUCT. #[unsafe(no_mangle)] @@ -358,9 +355,6 @@ pub extern "C-unwind" fn vx_array_new_primitive( /// `nullable` controls the top-level nullability of the resulting array's dtype. For an Arrow /// record batch (which has no top-level validity) pass `false`. /// -/// The imported buffers are referenced zero-copy where possible; the returned array keeps the -/// Arrow data alive until it is freed with [`vx_array_free`]. -/// /// On error, returns NULL and sets `error_out`. /// /// Example: @@ -520,6 +514,7 @@ mod tests { use crate::array::*; use crate::binary::vx_binary_free; + use crate::dtype::vx_dtype_free; use crate::dtype::vx_dtype_get_variant; use crate::dtype::vx_dtype_variant; use crate::error::vx_error_free; @@ -547,6 +542,7 @@ mod tests { assert_eq!(vx_array_get_i32(ffi_array, 1), 2); assert_eq!(vx_array_get_i32(ffi_array, 2), 3); + vx_dtype_free(array_dtype); vx_array_free(ffi_array); } } @@ -865,17 +861,14 @@ mod tests { let vx_arr = vx_array::new(Arc::new(array)); assert!(unsafe { vx_array_has_dtype(vx_arr, vx_dtype_variant::DTYPE_STRUCT) }); - // Get dtype reference - this is valid as long as array lives let dtype_ptr = unsafe { vx_array_dtype(vx_arr) }; let variant = unsafe { vx_dtype_get_variant(dtype_ptr) }; assert_eq!(variant, vx_dtype_variant::DTYPE_STRUCT); - // Proper usage: use dtype while array is still alive - // This demonstrates the correct lifetime pattern unsafe { vx_array_free(vx_arr) }; - - // Note: dtype_ptr is now invalid - this test documents the lifetime pattern - // In real usage, don't access dtype_ptr after freeing the array + unsafe { + vx_dtype_free(dtype_ptr); + } } #[test] diff --git a/vortex-ffi/src/data_source.rs b/vortex-ffi/src/data_source.rs index 8485be285bd..2d596be0675 100644 --- a/vortex-ffi/src/data_source.rs +++ b/vortex-ffi/src/data_source.rs @@ -95,8 +95,7 @@ unsafe fn data_source_new( /// Create a data source. /// The first matched file is opened eagerly. to read the schema. All other I/O -/// is deferred until a scan is requested. The returned pointer is owned by the -/// caller and must be freed with vx_data_source_free. +/// is deferred until a scan is requested. /// /// On error, returns NULL and sets "err". #[unsafe(no_mangle)] @@ -116,9 +115,6 @@ pub unsafe extern "C-unwind" fn vx_data_source_new( /// The bytes are borrowed, not copied: the caller must keep "buffer" alive and /// unmodified until the data source is freed. /// -/// The returned pointer is owned by the caller and must be freed with -/// vx_data_source_free. -/// /// On error, returns NULL and sets "err". #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn vx_data_source_new_buffer( @@ -147,11 +143,10 @@ pub unsafe extern "C-unwind" fn vx_data_source_new_buffer( }) } -/// Return the schema of the data source as a non-owned dtype. -/// The returned pointer is valid as long as "ds" is alive. Do not free it. +/// Return data source's dtype #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn vx_data_source_dtype(ds: *const vx_data_source) -> *const vx_dtype { - vx_dtype::new_ref(vx_data_source::as_ref(ds).dtype()) + vx_dtype::new(Arc::new(vx_data_source::as_ref(ds).dtype().clone())) } /// Write data source's row count estimate into "row_count". @@ -194,6 +189,7 @@ mod tests { use crate::data_source::vx_data_source_new_buffer; use crate::data_source::vx_data_source_options; use crate::dtype::vx_dtype; + use crate::dtype::vx_dtype_free; use crate::scan::vx_estimate; use crate::scan::vx_estimate_type; use crate::session::vx_session_free; @@ -254,7 +250,8 @@ mod tests { assert_no_error(error); assert!(!ds.is_null()); - let dtype = vx_dtype::as_ref(vx_data_source_dtype(ds)); + let ffi_dtype = vx_data_source_dtype(ds); + let dtype = vx_dtype::as_ref(ffi_dtype); assert_eq!(dtype, struct_array.dtype()); let mut row_count = vx_estimate::default(); @@ -262,6 +259,7 @@ mod tests { assert_eq!(row_count.r#type, vx_estimate_type::VX_ESTIMATE_EXACT); assert_eq!(row_count.estimate, SAMPLE_ROWS as u64); + vx_dtype_free(ffi_dtype); vx_data_source_free(ds); vx_session_free(session); } @@ -289,7 +287,8 @@ mod tests { assert_no_error(error); assert!(!ds.is_null()); - let dtype = vx_dtype::as_ref(vx_data_source_dtype(ds)); + let ffi_dtype = vx_data_source_dtype(ds); + let dtype = vx_dtype::as_ref(ffi_dtype); assert_eq!(dtype, struct_array.dtype()); let mut row_count = vx_estimate::default(); @@ -297,6 +296,7 @@ mod tests { assert_eq!(row_count.r#type, vx_estimate_type::VX_ESTIMATE_EXACT); assert_eq!(row_count.estimate, SAMPLE_ROWS as u64); + vx_dtype_free(ffi_dtype); vx_data_source_free(ds); vx_session_free(session); } diff --git a/vortex-ffi/src/dtype.rs b/vortex-ffi/src/dtype.rs index c74fa71b6ed..8f96d9e6e02 100644 --- a/vortex-ffi/src/dtype.rs +++ b/vortex-ffi/src/dtype.rs @@ -208,10 +208,9 @@ pub unsafe extern "C-unwind" fn vx_dtype_decimal_scale(dtype: *const vx_dtype) - .scale() } -/// Return a borrowed reference to the [`vx_struct_fields`] of a struct. -/// -/// The returned pointer is valid as long as the struct dtype is valid. -/// Do NOT free the returned pointer - it shares the lifetime of the struct dtype. +/// If "dtype" is DTYPE_STRUCT, return owned vx_struct_fields for this struct, +/// return NULL otherwise. Returned vx_struct_fields must be released with +/// vx_dtype_free. #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn vx_dtype_struct_dtype( dtype: *const vx_dtype, @@ -219,34 +218,29 @@ pub unsafe extern "C-unwind" fn vx_dtype_struct_dtype( let Some(struct_dtype) = vx_dtype::as_ref(dtype).as_struct_fields_opt() else { return ptr::null(); }; - vx_struct_fields::new_ref(struct_dtype) + vx_struct_fields::new(struct_dtype.clone()) } -/// Returns the element type of a list. -/// -/// The returned pointer is valid as long as the list dtype is valid. -/// Do NOT free the returned dtype pointer - it shares the lifetime of the list dtype. +/// If "dtype" is DTYPE_LIST, return its owned element dtype, return NULL +/// otherwise. Returned dtype must be released with vx_dtype_free. #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn vx_dtype_list_element(dtype: *const vx_dtype) -> *const vx_dtype { let Some(element_dtype) = vx_dtype::as_ref(dtype).as_list_element_opt() else { return ptr::null(); }; - vx_dtype::new_ref(element_dtype) + vx_dtype::new(Arc::clone(element_dtype)) } -/// Returns the element type of a fixed-size list. -/// -/// The returned pointer is valid as long as the fixed-size list dtype is valid. -/// Do NOT free the returned dtype pointer - it shares the lifetime of the fixed-size list dtype. +/// If "dtype" is DTYPE_FIXED_SIZE_LIST, return its owned element dtype, return +/// NULL otherwise. Returned dtype must be released with vx_dtype_free. #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn vx_dtype_fixed_size_list_element( dtype: *const vx_dtype, ) -> *const vx_dtype { - // TODO(joe): propagate this error up instead of expecting - let element_dtype = vx_dtype::as_ref(dtype) - .as_fixed_size_list_element_opt() - .vortex_expect("not a fixed-size list dtype"); - vx_dtype::new_ref(element_dtype) + let Some(element_dtype) = vx_dtype::as_ref(dtype).as_fixed_size_list_element_opt() else { + return ptr::null(); + }; + vx_dtype::new(Arc::clone(element_dtype)) } /// Returns the size of a fixed-size list. @@ -398,6 +392,7 @@ mod tests { use crate::dtype::vx_dtype_variant; use crate::ptype::vx_ptype; use crate::string::vx_string; + use crate::string::vx_string_free; use crate::string::vx_string_len; use crate::string::vx_string_ptr; use crate::struct_fields::vx_struct_fields_builder_add_field; @@ -449,6 +444,9 @@ mod tests { let age = vx_struct_fields_field_name(person, 1); assert_eq!(vx_string::as_str(age), "age"); + vx_string_free(name); + vx_string_free(age); + let dtype0 = vx_struct_fields_field_dtype(person, 0); let dtype1 = vx_struct_fields_field_dtype(person, 1); assert_eq!(vx_dtype_get_variant(dtype0), vx_dtype_variant::DTYPE_UTF8); @@ -457,13 +455,9 @@ mod tests { vx_dtype_variant::DTYPE_PRIMITIVE ); - // Field names are now borrowed references - do not free them - - // Free field dtypes (owned references) vx_dtype_free(dtype0); vx_dtype_free(dtype1); - // Free struct fields vx_struct_fields_free(person); } } @@ -517,6 +511,7 @@ mod tests { vx_dtype_variant::DTYPE_PRIMITIVE ); assert_eq!(vx_dtype_primitive_ptype(element), vx_ptype::PTYPE_I32); + vx_dtype_free(element); vx_dtype_free(list_dtype); } @@ -534,15 +529,14 @@ mod tests { ); assert!(vx_dtype_is_nullable(fsl_dtype)); - // Test element accessor let element = vx_dtype_fixed_size_list_element(fsl_dtype); assert_eq!( vx_dtype_get_variant(element), vx_dtype_variant::DTYPE_PRIMITIVE ); assert_eq!(vx_dtype_primitive_ptype(element), vx_ptype::PTYPE_F64); + vx_dtype_free(element); - // Test size accessor let size = vx_dtype_fixed_size_list_size(fsl_dtype); assert_eq!(size, 3); @@ -565,6 +559,7 @@ mod tests { let element = vx_dtype_fixed_size_list_element(fsl_dtype); assert_eq!(vx_dtype_get_variant(element), vx_dtype_variant::DTYPE_UTF8); assert!(vx_dtype_is_nullable(element)); + vx_dtype_free(element); let size = vx_dtype_fixed_size_list_size(fsl_dtype); assert_eq!(size, 10); @@ -607,6 +602,8 @@ mod tests { ); assert_eq!(vx_dtype_primitive_ptype(innermost), vx_ptype::PTYPE_I32); + vx_dtype_free(innermost); + vx_dtype_free(inner); vx_dtype_free(outer_fsl); } } @@ -703,8 +700,9 @@ mod tests { let n_fields = unsafe { vx_struct_fields_nfields(struct_fields_ptr) }; assert_eq!(n_fields, 2); - // Cleanup in reverse order - this is the safest order unsafe { + vx_struct_fields_free(struct_fields_ptr); + vx_dtype_free(dtype_ptr); vx_array_free(vx_arr); } } @@ -729,9 +727,10 @@ mod tests { let name_str = str::from_utf8(name_slice).unwrap(); assert_eq!(name_str, "nums"); - // Cleanup in careful order unsafe { - // Field name is now a borrowed reference - do not free it + vx_string_free(field_name_ptr); + vx_struct_fields_free(struct_fields_ptr); + vx_dtype_free(dtype_ptr); vx_array_free(vx_arr); } } @@ -762,11 +761,12 @@ mod tests { let expected_name = if i == 0 { "nums" } else { "floats" }; assert_eq!(name_str, expected_name); - // Field name is now a borrowed reference - do not free it + unsafe { vx_string_free(field_name_ptr) }; } - // Cleanup unsafe { + vx_struct_fields_free(struct_fields_ptr); + vx_dtype_free(dtype_ptr); vx_array_free(vx_arr); } } @@ -800,6 +800,7 @@ mod tests { let f1 = vx_struct_fields_field_dtype(fields, 1); assert_eq!(vx_dtype_get_variant(f1), vx_dtype_variant::DTYPE_UTF8); assert!(vx_dtype_is_nullable(f1)); + vx_struct_fields_free(fields); vx_dtype_free(f1); vx_dtype_free(dtype); } diff --git a/vortex-ffi/src/error.rs b/vortex-ffi/src/error.rs index 3ee02c99e21..85d6984d33d 100644 --- a/vortex-ffi/src/error.rs +++ b/vortex-ffi/src/error.rs @@ -103,13 +103,10 @@ pub fn try_or( } } -/// Returns the error message from the given Vortex error. -/// -/// The returned pointer is valid as long as the error is valid. -/// Do NOT free the returned string pointer - it shares the lifetime of the error. +/// Return an error message for this error #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn vx_error_get_message(error: *const vx_error) -> *const vx_string { - vx_string::new_ref(&vx_error::as_ref(error).message) + vx_string::new(Arc::clone(&vx_error::as_ref(error).message)) } #[cfg(test)] @@ -160,6 +157,7 @@ mod tests { vx_string::as_ref(message).as_ref(), "panic in Vortex FFI function: boom" ); + unsafe { crate::string::vx_string_free(message) }; unsafe { vx_error_free(error) }; } } diff --git a/vortex-ffi/src/expression.rs b/vortex-ffi/src/expression.rs index 269e1e5d941..80471537e0d 100644 --- a/vortex-ffi/src/expression.rs +++ b/vortex-ffi/src/expression.rs @@ -38,8 +38,6 @@ crate::box_wrapper!( /// data. Each expression consists of an encoding (vtable), heap-allocated /// metadata, and child expressions. /// - /// Unless stated explicitly, all expressions returned are owned and must - /// be freed by the caller. /// Unless stated explicitly, if an operation on const vx_expression* is /// passed NULL, NULL is returned. /// Operations on expressions don't take ownership of input values, and so diff --git a/vortex-ffi/src/lib.rs b/vortex-ffi/src/lib.rs index 43e108e6c28..dadd5701f6f 100644 --- a/vortex-ffi/src/lib.rs +++ b/vortex-ffi/src/lib.rs @@ -113,6 +113,7 @@ mod tests { use crate::sink::vx_array_sink_open_file; use crate::sink::vx_array_sink_push; use crate::string::vx_string; + use crate::string::vx_string_free; /// Panic if error is NULL. Free the error if it's not pub(crate) fn assert_error(error: *mut vx_error) { @@ -125,7 +126,9 @@ mod tests { if !error.is_null() { let message; unsafe { - message = vx_string::as_str(vx_error_get_message(error)).to_owned(); + let msg_ptr = vx_error_get_message(error); + message = vx_string::as_str(msg_ptr).to_owned(); + vx_string_free(msg_ptr); vx_error_free(error); } panic!("{message}"); diff --git a/vortex-ffi/src/macros.rs b/vortex-ffi/src/macros.rs index d4e58288b30..1e943c17ad9 100644 --- a/vortex-ffi/src/macros.rs +++ b/vortex-ffi/src/macros.rs @@ -21,7 +21,7 @@ //! Each macro provides a `free` function, and the `Arc` variants also provide a `clone` function. //! //! Converting between the raw pointer and the wrapped type is done using the generated `new`, -//! `new_ref`, `as_ref`, `as_mut`, `into_box`, and `into_arc` methods, which internally check for +//! `as_ref`, `as_mut`, `into_box`, and `into_arc` methods, which internally check for //! null pointers. //! //! ## Internals @@ -59,11 +59,6 @@ macro_rules! arc_dyn_wrapper { Box::into_raw(Box::new($ffi_ident(obj))).cast_const() } - /// Wrap a borrowed object into a raw pointer. - pub(crate) fn new_ref(obj: &std::sync::Arc<$T>) -> *const $ffi_ident { - obj as *const std::sync::Arc<$T> as *const $ffi_ident - } - /// Extract a borrowed reference from a const pointer. pub(crate) fn as_ref<'a>(ptr: *const $ffi_ident) -> &'a std::sync::Arc<$T> { use vortex::error::VortexExpect; @@ -82,8 +77,7 @@ macro_rules! arc_dyn_wrapper { } } - #[doc = r" Clone a borrowed [`" $ffi_ident "`], returning an owned [`" $ffi_ident "`].\n\n"] - #[doc = r" Must be released with [`" $ffi_ident "_free`]."] + #[doc = r" Clone a " $ffi_ident ". Returned handle must be release with " $ffi_ident "_free "] #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn [<$ffi_ident _clone>](ptr: *const $ffi_ident) -> *const $ffi_ident { if ptr.is_null() { @@ -93,7 +87,7 @@ macro_rules! arc_dyn_wrapper { $ffi_ident::new($ffi_ident::as_ref(ptr).clone()) } - #[doc = r" Free an owned [`" $ffi_ident "`] object."] + #[doc = r" Free a " $ffi_ident] #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn [<$ffi_ident _free>](ptr: *const $ffi_ident) { if ptr.is_null() { @@ -121,11 +115,6 @@ macro_rules! arc_wrapper { std::sync::Arc::into_raw(obj).cast::<$ffi_ident>() } - /// Wrap a borrowed object into a raw pointer. - pub(crate) fn new_ref(obj: &$T) -> *const $ffi_ident { - obj as *const $T as *const $ffi_ident - } - /// Extract a borrowed reference from a const pointer. pub(crate) fn as_ref(ptr: *const $ffi_ident) -> &'static $T { use vortex::error::VortexExpect; @@ -144,8 +133,7 @@ macro_rules! arc_wrapper { } } - #[doc = r" Clone a borrowed [`" $ffi_ident "`], returning an owned [`" $ffi_ident "`].\n\n"] - #[doc = r" Must be released with [`" $ffi_ident "_free`]."] + #[doc = r" Clone a " $ffi_ident] #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn [<$ffi_ident _clone>](ptr: *const $ffi_ident) -> *const $ffi_ident { if ptr.is_null() { @@ -186,21 +174,6 @@ macro_rules! box_dyn_wrapper { Box::into_raw(Box::new($ffi_ident(obj))) } - /// Wrap a borrowed object into a raw pointer. - pub(crate) fn new_ref(obj: &$T) -> *const $ffi_ident { - obj as *const $T as *const $ffi_ident - } - - /// Extract a borrowed reference from a const pointer. - pub(crate) fn as_ref<'a>(ptr: *const $ffi_ident) -> &'a $T { - use vortex::error::VortexExpect; - // TODO(joe): propagate this error up instead of expecting - unsafe { ptr.as_ref() } - .vortex_expect("null pointer") - .0 - .as_ref() - } - /// Extract a borrowed mutable reference from a mut pointer. pub(crate) fn as_mut<'a>(ptr: *mut $ffi_ident) -> &'a mut $T { use vortex::error::VortexExpect; @@ -222,11 +195,11 @@ macro_rules! box_dyn_wrapper { #[doc = r" Free an owned [`" $ffi_ident "`] object."] #[unsafe(no_mangle)] - pub unsafe extern "C-unwind" fn [<$ffi_ident _free>](ptr: *mut $ffi_ident) { + pub unsafe extern "C-unwind" fn [<$ffi_ident _free>](ptr: *const $ffi_ident) { if ptr.is_null() { vortex::error::vortex_panic!("null pointer"); } - drop($ffi_ident::into_box(ptr)) + drop($ffi_ident::into_box(ptr.cast_mut())) } } }; @@ -253,11 +226,6 @@ macro_rules! box_wrapper { Box::into_raw(Box::new(obj)).cast::<$ffi_ident>() } - /// Wrap a borrowed object into a raw pointer. - pub(crate) fn new_ref(obj: &$T) -> *const $ffi_ident { - obj as *const $T as *const $ffi_ident - } - /// Extract a borrowed reference from a const pointer. pub(crate) fn as_ref<'a>(ptr: *const $ffi_ident) -> &'a $T { use vortex::error::VortexExpect; @@ -291,11 +259,11 @@ macro_rules! box_wrapper { #[allow(clippy::missing_safety_doc)] #[allow(rustdoc::private_intra_doc_links)] #[unsafe(no_mangle)] - pub unsafe extern "C-unwind" fn [<$ffi_ident _free>](ptr: *mut $ffi_ident) { + pub unsafe extern "C-unwind" fn [<$ffi_ident _free>](ptr: *const $ffi_ident) { if ptr.is_null() { vortex::error::vortex_panic!("null pointer"); } - std::mem::drop(unsafe { Box::from_raw(ptr.cast::<$T>()) }) + std::mem::drop(unsafe { Box::from_raw(ptr.cast::<$T>().cast_mut()) }) } } }; diff --git a/vortex-ffi/src/scalar.rs b/vortex-ffi/src/scalar.rs index 2471c2c4c9f..64dd670feb0 100644 --- a/vortex-ffi/src/scalar.rs +++ b/vortex-ffi/src/scalar.rs @@ -37,10 +37,8 @@ crate::box_wrapper!( vx_scalar ); -/// Clone a borrowed scalar handle. -/// -/// The input scalar handle is not consumed. The returned scalar handle must be -/// released with vx_scalar_free. Returns NULL when given a NULL scalar handle. +/// Clone a scalar handle. +/// If scalar is NULL, returns NULL. #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn vx_scalar_clone(scalar: *const vx_scalar) -> *mut vx_scalar { if scalar.is_null() { @@ -49,17 +47,14 @@ pub unsafe extern "C-unwind" fn vx_scalar_clone(scalar: *const vx_scalar) -> *mu vx_scalar::new(vx_scalar::as_ref(scalar).clone()) } -/// Return the data type of a scalar. -/// -/// The returned data type handle borrows storage from the scalar handle, so its -/// lifetime is bound to the scalar handle. It MUST NOT be freed separately. -/// Returns NULL when given a NULL scalar handle. +/// Return scalar's dtype. +/// If scalar is NULL, returns NULL. #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn vx_scalar_dtype(scalar: *const vx_scalar) -> *const vx_dtype { if scalar.is_null() { return ptr::null(); } - vx_dtype::new_ref(vx_scalar::as_ref(scalar).dtype()) + vx_dtype::new(Arc::new(vx_scalar::as_ref(scalar).dtype().clone())) } /// Return whether the scalar is a typed null value. @@ -202,9 +197,11 @@ pub unsafe extern "C-unwind" fn vx_scalar_new_binary( /// Create a typed null scalar. /// -/// The data type handle is borrowed, not consumed. The returned scalar uses a -/// nullable copy of that logical type, regardless of the input type's top-level -/// nullability. A NULL data type handle returns NULL and writes the error output. +/// "dtype" is not consumed, you can use it after calling this function. Returned +/// scalar uses a nullable copy of that logical type, regardless of the input +/// type's top-level nullability. +/// +/// Returns NULL and sets "err" on error or NULL dtype. #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn vx_scalar_new_null( dtype: *const vx_dtype, @@ -342,10 +339,8 @@ pub unsafe extern "C-unwind" fn vx_scalar_new_decimal_i256_le( /// Create a list scalar. /// -/// The element data type handle is borrowed, not consumed. Child scalar handles -/// are cloned into the list value, so the caller keeps ownership of the handle -/// array and each scalar in it. A NULL child handle array is allowed only for an -/// empty list. Child values are validated against the element logical type. +/// "element_dtype" and "elements" are not consumed, you can use them after +/// calling this function. If len is 0, you can pass NULL to "elements". #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn vx_scalar_new_list( element_dtype: *const vx_dtype, @@ -370,30 +365,25 @@ pub unsafe extern "C-unwind" fn vx_scalar_new_list( /// Create a fixed-size list scalar. /// -/// The element data type handle is borrowed, not consumed. The number of child -/// scalars becomes the fixed-size list width and must fit in a 32-bit unsigned -/// integer. Child scalar handles are cloned into the list value, so the caller -/// keeps ownership of the handle array and each scalar in it. A NULL child -/// handle array is allowed only for an empty list. Child values are validated -/// against the element logical type. +/// "element_dtype" and "elements" are not consumed, you can use them after +/// calling this function. If len is 0, you can pass NULL to "elements". +/// "len" must fit in uint32_t. #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn vx_scalar_new_fixed_size_list( element_dtype: *const vx_dtype, elements: *const *const vx_scalar, - len: usize, + len: u32, is_nullable: bool, err: *mut *mut vx_error, ) -> *mut vx_scalar { try_or(err, ptr::null_mut(), || { vortex_ensure!(!element_dtype.is_null(), "element dtype is null"); - let size = u32::try_from(len) - .map_err(|_| vortex_err!("fixed-size list length {len} exceeds u32::MAX"))?; let dtype = DType::FixedSizeList( Arc::new(vx_dtype::as_ref(element_dtype).clone()), - size, + len, Nullability::from(is_nullable), ); - let values = scalar_values_from_raw(elements, len)?; + let values = scalar_values_from_raw(elements, len as usize)?; Ok(vx_scalar::new(Scalar::try_new( dtype, Some(ScalarValue::Tuple(values)), @@ -403,11 +393,8 @@ pub unsafe extern "C-unwind" fn vx_scalar_new_fixed_size_list( /// Create a struct scalar. /// -/// The struct data type handle is borrowed, not consumed. Field scalar handles -/// are cloned into the struct value, so the caller keeps ownership of the handle -/// array and each scalar in it. Field count and field logical types are validated -/// against the struct logical type. A NULL field handle array is allowed only for -/// an empty struct value. +/// "struct_dtype" and "fields" are not consumed, you can use them after calling +/// this function. If len is 0, you can pass NULL to "fields". #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn vx_scalar_new_struct( struct_dtype: *const vx_dtype, @@ -620,10 +607,12 @@ mod tests { vx_dtype_free(dtype); assert_no_error(error); assert!(vx_scalar_is_null(null_scalar)); + let scalar_dtype = vx_scalar_dtype(null_scalar); assert_eq!( - vx_dtype::as_ref(vx_scalar_dtype(null_scalar)), + vx_dtype::as_ref(scalar_dtype), &DType::Primitive(PType::I32, Nullability::Nullable) ); + vx_dtype_free(scalar_dtype); vx_scalar_free(null_scalar); } } @@ -743,11 +732,12 @@ mod tests { ); assert_no_error(error); + let len = u32::try_from(children.len()).unwrap(); assert_scalar( vx_scalar_new_fixed_size_list( element_dtype, children.as_ptr(), - children.len(), + len, false, &raw mut error, ), diff --git a/vortex-ffi/src/scan.rs b/vortex-ffi/src/scan.rs index 619f82e3f3e..2bc748edeb9 100644 --- a/vortex-ffi/src/scan.rs +++ b/vortex-ffi/src/scan.rs @@ -225,9 +225,7 @@ fn write_estimate>(estimate: Precision, out: &mut vx_estimate) { /// Scan a data source. /// -/// Return an owned scan that must be freed with vx_scan_free. A scan may be -/// consumed only once. -/// +/// A scan may be consumed only once. /// "options" and "estimate" may be NULL. /// /// If "options" is NULL, all rows and columns are returned. @@ -256,10 +254,8 @@ pub unsafe extern "C-unwind" fn vx_data_source_scan( }) } -/// Return borrowed vx_scan's dtype. +/// Return scan's dtype. /// This function will fail if called after vx_scan_next_partition. -/// Called must not free the returned pointer as its lifetime is bound to the -/// lifetime of the scan. /// On error returns NULL and sets "err". #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn vx_scan_dtype( @@ -271,12 +267,11 @@ pub unsafe extern "C-unwind" fn vx_scan_dtype( let VxScan::Pending(scan) = scan else { vortex_bail!("dtype unavailable: scan already started"); }; - Ok(vx_dtype::new_ref(scan.dtype())) + Ok(vx_dtype::new(Arc::new(scan.dtype().clone()))) }) } -/// Return an owned partition from a scan. -/// The returned partition must be freed with vx_partition_free. +/// Return an partition from a scan. /// /// On success returns a partition. /// On exhaustion (no more partitions in scan) returns NULL but doesn't set @@ -398,8 +393,7 @@ pub unsafe extern "C-unwind" fn vx_partition_scan_arrow( }) } -/// Return an owned owned array from a partition. -/// The returned array must be freed with vx_array_free. +/// Return an array from a partition. /// /// On success returns an array. /// On exhaustion (no more arrays in partition) returns NULL but doesn't set diff --git a/vortex-ffi/src/struct_fields.rs b/vortex-ffi/src/struct_fields.rs index fa8f10aca83..ec56304cfa6 100644 --- a/vortex-ffi/src/struct_fields.rs +++ b/vortex-ffi/src/struct_fields.rs @@ -29,11 +29,8 @@ pub unsafe extern "C-unwind" fn vx_struct_fields_nfields(dtype: *const vx_struct .nfields() as u64 } -/// Return a borrowed reference to the name of the field at the given index. -/// -/// The returned pointer is valid as long as the struct fields is valid. -/// Do NOT free the returned string pointer - it shares the lifetime of the struct fields. -/// Returns null if the index is out of bounds. +/// Return an owned name of the field at a given index. +/// If index is out of bounds, returns NULL. #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn vx_struct_fields_field_name( dtype: *const vx_struct_fields, @@ -46,16 +43,11 @@ pub unsafe extern "C-unwind" fn vx_struct_fields_field_name( return ptr::null(); } let name = struct_dtype.names()[idx].inner(); - vx_string::new_ref(name) + vx_string::new(Arc::clone(name)) } -/// Returns an *owned* reference to the dtype of the field at the given index. -/// -/// The return type is owned since struct dtypes can be lazily parsed from a binary format, in -/// which case it's not possible to return a borrowed reference to the field dtype. -/// -/// Returns null if the index is out of bounds or if the field dtype cannot be parsed. -// TODO(ngates): should StructDType cache owned fields internally? +/// Return an owned dtype of the field at a given index. +/// Returns NULL if index is out of bounds or if dtype cannot be parsed. #[unsafe(no_mangle)] pub unsafe extern "C-unwind" fn vx_struct_fields_field_dtype( dtype: *const vx_struct_fields, diff --git a/vortex-ffi/test/array.cpp b/vortex-ffi/test/array.cpp index 4fde064a339..f558243525a 100644 --- a/vortex-ffi/test/array.cpp +++ b/vortex-ffi/test/array.cpp @@ -9,7 +9,11 @@ TEST_CASE("Null array creation", "[array]") { REQUIRE(array != nullptr); REQUIRE(vx_array_is_nullable(array)); REQUIRE(vx_array_has_dtype(array, DTYPE_NULL)); - REQUIRE(vx_dtype_get_variant(vx_array_dtype(array)) == DTYPE_NULL); + const vx_dtype *dtype = vx_array_dtype(array); + defer { + vx_dtype_free(dtype); + }; + REQUIRE(vx_dtype_get_variant(dtype) == DTYPE_NULL); REQUIRE(vx_array_len(array) == 1999); vx_array_free(array); } @@ -26,7 +30,11 @@ TEST_CASE("Primitive array creation", "[array]") { require_no_error(error); REQUIRE(array != nullptr); REQUIRE(vx_array_has_dtype(array, DTYPE_PRIMITIVE)); - REQUIRE(vx_dtype_get_variant(vx_array_dtype(array)) == DTYPE_PRIMITIVE); + const vx_dtype *dtype = vx_array_dtype(array); + REQUIRE(vx_dtype_get_variant(dtype) == DTYPE_PRIMITIVE); + defer { + vx_dtype_free(dtype); + }; REQUIRE(vx_array_is_primitive(array, PTYPE_U8)); REQUIRE(vx_array_len(array) == buffer.size()); diff --git a/vortex-ffi/test/common.h b/vortex-ffi/test/common.h index 29087e7a48d..60975d22015 100644 --- a/vortex-ffi/test/common.h +++ b/vortex-ffi/test/common.h @@ -7,22 +7,20 @@ inline std::string to_string(vx_error *err) { const vx_string *msg = vx_error_get_message(err); - return {vx_string_ptr(msg), vx_string_len(msg)}; + const std::string out {vx_string_ptr(msg), vx_string_len(msg)}; + vx_string_free(msg); + return out; } inline std::string_view to_string_view(const vx_string *msg) { return {vx_string_ptr(msg), vx_string_len(msg)}; } -inline std::string_view to_string_view(vx_error *err) { - return to_string_view(vx_error_get_message(err)); -} - inline void require_no_error(vx_error *error, bool assert = true) { if (!error) { return; } - auto message = to_string(error); + std::string message = to_string(error); vx_error_free(error); if (assert) { FAIL(message); diff --git a/vortex-ffi/test/scan.cpp b/vortex-ffi/test/scan.cpp index 6857ea82bc1..b1d4b8ce3f8 100644 --- a/vortex-ffi/test/scan.cpp +++ b/vortex-ffi/test/scan.cpp @@ -265,27 +265,36 @@ TEST_CASE("Write file and read dtypes", "[datasource]") { CHECK(row_count.estimate == SAMPLE_ROWS); const vx_dtype *data_source_dtype = vx_data_source_dtype(ds); + defer { + vx_dtype_free(data_source_dtype); + }; REQUIRE(vx_dtype_get_variant(data_source_dtype) == DTYPE_STRUCT); const vx_struct_fields *fields = vx_dtype_struct_dtype(data_source_dtype); + defer { + vx_struct_fields_free(fields); + }; const size_t len = vx_struct_fields_nfields(fields); REQUIRE(len == 2); const vx_dtype *age_dtype = vx_struct_fields_field_dtype(fields, 0); + const vx_string *age_name = vx_struct_fields_field_name(fields, 0); defer { vx_dtype_free(age_dtype); + vx_string_free(age_name); }; - const vx_string *age_name = vx_struct_fields_field_name(fields, 0); + REQUIRE(vx_dtype_get_variant(age_dtype) == DTYPE_PRIMITIVE); REQUIRE(vx_dtype_primitive_ptype(age_dtype) == PTYPE_U8); REQUIRE_FALSE(vx_dtype_is_nullable(age_dtype)); REQUIRE(to_string_view(age_name) == "age"); const vx_dtype *height_dtype = vx_struct_fields_field_dtype(fields, 1); + const vx_string *height_name = vx_struct_fields_field_name(fields, 1); defer { vx_dtype_free(height_dtype); + vx_string_free(height_name); }; - const vx_string *height_name = vx_struct_fields_field_name(fields, 1); REQUIRE(vx_dtype_get_variant(height_dtype) == DTYPE_PRIMITIVE); REQUIRE(vx_dtype_primitive_ptype(height_dtype) == PTYPE_U16); REQUIRE(vx_dtype_is_nullable(height_dtype)); @@ -294,7 +303,11 @@ TEST_CASE("Write file and read dtypes", "[datasource]") { void verify_age_field(const vx_array *age_field) { REQUIRE(vx_array_has_dtype(age_field, DTYPE_PRIMITIVE)); - REQUIRE(vx_dtype_primitive_ptype(vx_array_dtype(age_field)) == PTYPE_U8); + const vx_dtype *dtype = vx_array_dtype(age_field); + defer { + vx_dtype_free(dtype); + }; + REQUIRE(vx_dtype_primitive_ptype(dtype) == PTYPE_U8); REQUIRE(vx_array_len(age_field) == SAMPLE_ROWS); for (size_t i = 0; i < SAMPLE_ROWS; ++i) { REQUIRE(vx_array_get_u8(age_field, i) == i); @@ -303,7 +316,11 @@ void verify_age_field(const vx_array *age_field) { void verify_height_field(const vx_array *height_field) { REQUIRE(vx_array_has_dtype(height_field, DTYPE_PRIMITIVE)); - REQUIRE(vx_dtype_primitive_ptype(vx_array_dtype(height_field)) == PTYPE_U16); + const vx_dtype *dtype = vx_array_dtype(height_field); + defer { + vx_dtype_free(dtype); + }; + REQUIRE(vx_dtype_primitive_ptype(dtype) == PTYPE_U16); REQUIRE(vx_array_len(height_field) == SAMPLE_ROWS); for (size_t i = 0; i < SAMPLE_ROWS; ++i) { REQUIRE(vx_array_get_u16(height_field, i) > 0); @@ -314,7 +331,9 @@ void verify_sample_array(const vx_array *array) { REQUIRE(vx_array_len(array) == SAMPLE_ROWS); REQUIRE(vx_array_has_dtype(array, DTYPE_STRUCT)); - const vx_struct_fields *fields = vx_dtype_struct_dtype(vx_array_dtype(array)); + const vx_dtype *dtype = vx_array_dtype(array); + const vx_struct_fields *fields = vx_dtype_struct_dtype(dtype); + vx_dtype_free(dtype); size_t len = vx_struct_fields_nfields(fields); REQUIRE(len == 2); @@ -325,6 +344,7 @@ void verify_sample_array(const vx_array *array) { vx_dtype_free(age_dtype); const vx_string *age_name = vx_struct_fields_field_name(fields, 0); REQUIRE(to_string_view(age_name) == "age"); + vx_string_free(age_name); const vx_dtype *height_dtype = vx_struct_fields_field_dtype(fields, 1); REQUIRE(vx_dtype_get_variant(height_dtype) == DTYPE_PRIMITIVE); @@ -332,6 +352,9 @@ void verify_sample_array(const vx_array *array) { vx_dtype_free(height_dtype); const vx_string *height_name = vx_struct_fields_field_name(fields, 1); REQUIRE(to_string_view(height_name) == "height"); + vx_string_free(height_name); + + vx_struct_fields_free(fields); vx_error *error = nullptr; vx_validity validity = {}; diff --git a/vortex-ffi/test/struct.cpp b/vortex-ffi/test/struct.cpp index 6ccdb04c037..4a48e9390d2 100644 --- a/vortex-ffi/test/struct.cpp +++ b/vortex-ffi/test/struct.cpp @@ -54,9 +54,7 @@ TEST_CASE("Creating structs", "[struct]") { 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)}; @@ -73,6 +71,7 @@ TEST_CASE("Creating structs", "[struct]") { } vx_dtype_free(dtype); + vx_string_free(name); } vx_struct_fields_free(fields);