-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ffi): extra FFI improvements #2679
base: develop
Are you sure you want to change the base?
Conversation
a10y
commented
Mar 12, 2025
•
edited
Loading
edited
- Expose projection pushdown over FFI
- Enable object store reads over FFI
- Generate C header file with cbindgen
- Add buildable example for FFI with C
07f2edf
to
3a9f1f9
Compare
@@ -48,7 +48,6 @@ impl Field { | |||
} | |||
|
|||
/// A path through a (possibly nested) struct, composed of a sequence of field selectors | |||
// TODO(ngates): wrap `Vec<Field>` in Option for cheaper "root" path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty Vec doesn't allocate
@@ -0,0 +1,72 @@ | |||
# Directory containing the Rust crate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude wrote 99% of this Makefile 🚀
9231566
to
62e7c0d
Compare
CodSpeed Performance ReportMerging #2679 will degrade performances by 11.2%Comparing Summary
Benchmarks breakdown
|
ed97baa
to
254d76b
Compare
@@ -43,7 +43,7 @@ impl ViewedDType { | |||
|
|||
impl StructDType { | |||
/// Creates a new instance from a flatbuffer-defined object and its underlying buffer. | |||
pub fn from_fb(fb_struct: fbd::Struct_<'_>, buffer: FlatBuffer) -> VortexResult<Self> { | |||
fn from_fb(fb_struct: fbd::Struct_<'_>, buffer: FlatBuffer) -> VortexResult<Self> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was browsing our docs.rs page and this method felt out of place, but not sure
@@ -15,7 +15,6 @@ use vortex::{Array, ArrayRef, ArrayVariants}; | |||
/// Because dyn Trait pointers cannot be shared across FFI, we create a new struct to hold | |||
/// the wide pointer. The C FFI only seems a pointer to this structure, and can pass it into | |||
/// one of the various `FFIArray_*` functions. | |||
#[repr(C)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not actually repr(C), it's only accessed opaquely via pointer
pub struct FFIFile { | ||
pub(crate) inner: VortexFile<GenericVortexFile<ObjectStoreReadAt>>, | ||
} | ||
|
||
/// Options supplied for opening a file. | ||
#[repr(C)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not accessed opaquely, and thus needs to be repr(C) so we can build this struct in other languages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps this would be better opaque, and then build it using a function over FFI that accepts all of these fields as parameters. but that kinda is just the same thing? idk
ccb6a54
to
78710fd
Compare