ffi: expose array validity, allow creating primitive arrays#7148
ffi: expose array validity, allow creating primitive arrays#7148
Conversation
b7d2f79 to
516884b
Compare
516884b to
430ec1e
Compare
430ec1e to
a3c15ad
Compare
Merging this PR will degrade performance by 15.51%
Performance Changes
Comparing Footnotes
|
vortex-ffi/src/array.rs
Outdated
| ); | ||
|
|
||
| #[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.
| * 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?
| 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?
There was a problem hiding this comment.
They both extract a view of the error string messsage, so they do the same thing
| ) -> *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
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.
Signed-off-by: Mikhail Kot <mikhail@spiraldb.com>
a3c15ad to
353e971
Compare
directly instead of array -> dtype -> variant [-> ptype] chain
Breaking changes:
vx_array_element_is_nullfromvx_array_is_nullvx_array_null_counttovx_array_invalid_count.