ffi: expose array validity, allow creating primitive arrays#7148
ffi: expose array validity, allow creating primitive arrays#7148
Conversation
myrrc
force-pushed
the
myrrc/primitive-ffi
branch
from
b7d2f79 to
516884b
Compare
myrrc
added
changelog/feature
myrrc
force-pushed
the
myrrc/primitive-ffi
branch
from
516884b to
430ec1e
Compare
Signed-off-by: Mikhail Kot <mikhail@spiraldb.com>
myrrc
force-pushed
the
myrrc/primitive-ffi
branch
from
430ec1e to
a3c15ad
Compare
Merging this PR will degrade performance by 15.51%
Performance Changes
Comparing 
Footnotes
|
| ); | ||
|
|
||
| #[unsafe(no_mangle)] | ||
| pub unsafe extern "C-unwind" fn vx_array_is_nullable(array: *const vx_array) -> bool { |
There was a problem hiding this comment.
This wasn't introduced as a pattern in this PR, but we should consider documenting in the code what C-unwind implies / allows for (compared to extern "C"), explaining how the unwinding can be leveraged.
| */ | ||
| void vx_array_free(const vx_array *ptr); | ||
|
|
||
| bool vx_array_is_nullable(const vx_array *array); |
There was a problem hiding this comment.
All public C API should have proper docs.
|
|
||
| bool vx_array_is_nullable(const vx_array *array); | ||
|
|
||
| bool vx_array_is(const vx_array *array, vx_dtype_variant variant); |
| * validity can't be NULL. | ||
| */ | ||
| const vx_array *vx_array_new_primitive(vx_ptype ptype, | ||
| const void *ptr, |
There was a problem hiding this comment.
The ptr parameter name should be more descriptive. Shall we have in the fn docs describing what each param is for, here and elsewhere?
| vx_error | ||
| ); | ||
|
|
||
| pub(crate) fn write_error(error_out: *mut *mut vx_error, error_string: &str) { |
There was a problem hiding this comment.
I guess part of the contract here is that error_out is not set elsewhere before, otherwise the previous error is leaked.
| */ | ||
| bool vx_array_element_is_null(const vx_array *array, size_t index, vx_error **error); | ||
|
|
||
| uint32_t vx_array_null_count(const vx_array *array, vx_error **error_out); |
There was a problem hiding this comment.
Not introduced in this PR but this should be size_t.
There was a problem hiding this comment.
And shall we consider matching the Rust name vx_array_null_count => ..invalid_count.
| @@ -0,0 +1,24 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
| // SPDX-FileCopyrightText: Copyright the Vortex contributors | |||
| #include <catch2/catch_test_macros.hpp> | |||
| return {vx_string_ptr(msg), vx_string_len(msg)}; | ||
| } | ||
|
|
||
| inline std::string_view to_string_view(vx_error *err) { |
There was a problem hiding this comment.
Should they have different fn name prefixes? They kinda do different things no?
| ) -> *const vx_array { | ||
| let slice = unsafe { std::slice::from_raw_parts(ptr, len) }; | ||
| let buffer = Buffer::copy_from(slice); | ||
| let array = PrimitiveArray::new(buffer, validity.into()); |
There was a problem hiding this comment.
In impl From<&vx_validity> for Validity, we clone the arc validity::Array(vx_array::as_ref(value.array).clone()) (aka atomic ref counted pointer).
Saying we don't take ownership of validity as mentioned in the new_primitive docs.
0ax1
left a comment
0ax1
left a comment
There was a problem hiding this comment.
We need to double check the consistency of size types for indices:
vx_array_get_field` | `uint32_t` |
| `vx_array_slice` | `uint32_t` |
| `vx_array_get_u8` etc. | `uint32_t` |
| `vx_array_null_count` return | `uint32_t
should prob all be size_t.
2 participants
directly instead of array -> dtype -> variant [-> ptype] chain
vx_array_element_is_nullfromvx_array_is_null