datafusion-ffi

# DataFusion FFI Skill This crate exposes a stable C ABI for DataFusion traits so that independently-compiled libraries (different Rust versions, plugins, `datafusion-python`, etc.) can interoperate at runtime. Stability and correctness here are load-bearing: a missed pattern can cause segfaults, leaks, or silently dropped trait behavior on the consumer side. Read the crate's `README.md` first if you have not — it establishes the vocabulary (`FFI_X` / `ForeignX`, `library_marker_id`, `release`, `TaskContextProvider`, stabby vs `#[repr(C)]`). ## When to use Trigger this skill any time the work touches `datafusion/ffi/`: - Adding a new `FFI_<Trait>` + `Foreign<Trait>` pair - Adding a method to an existing `FFI_X` struct - Reviewing a PR that touches this crate - Changing the codec / proto serialization layer - Bumping the wrapped DataFusion trait surface (e.g. a new default method appeared upstream) ## Hard rules 1. **No `datafusion` dependency.** `datafusion-ffi` must not depend on the umbrella `datafusion` crate. Use the leaf crates (`datafusion-common`, `datafusion-expr`, `datafusion-catalog`, `datafusion-physical-plan`, etc.). `datafusion` is fine in `[dev-dependencies]`. 2. **`#[repr(C)]` on every `FFI_X` struct**, not `#[stabby::stabby]`. Stabby is used for `SString`/`SVec` only. Reasons documented in the README (build time, Arrow types lack `IStable`). 3. **`unsafe extern "C"` on every function-pointer field — including `version`.** The one exception is the `library_marker_id` field, which is plain `extern "C" fn() -> usize`. Plain (safe) `extern "C"` also applies to the standalone function defs in `src/lib.rs` — `pub extern "C" fn version()` and `pub extern "C" fn get_library_marker_id()` — which coerce into the `unsafe extern "C"` `version` field slot at construction. 4. **Match `Send`/`Sync` to the wrapped trait.** Raw `*mut c_void` makes every `FFI_X` `!Send + !Sync` by default — `unsafe impl` whichever bounds the consumer-facing trait requires. Most DataFusion traits (`TableProvider`, `ExecutionPlan`, all UDFs, codecs) need both. `Send`-only: `RecordBatchStream`, `Accumulator`, `GroupsAccumulator`, `PartitionEvaluator` (mutable / stream APIs). The matching `ForeignX` always carries the same bounds — pick consistently. 5. **`#![deny(clippy::clone_on_ref_ptr)]`** is on at the crate root. Use `Arc::clone(&x)`, never `x.clone()` on `Arc`. 6. **Run before pushing:** `cargo fmt --all`, `cargo clippy -p datafusion-ffi --all-targets --all-features -- -D warnings`, `cargo test -p datafusion-ffi`. 7. **`api change` label required.** Any PR that modifies an `FFI_X` struct layout (adds/removes/reorders fields, changes a function-pointer signature, adds a variant to an FFI enum, or changes the `version` extern) must carry the `api change` GitHub label. Layout changes break ABI for already-compiled consumer libraries. The label is the project-wide convention for highlighting breaking public-API changes in release notes — see `docs/source/contributor-guide/api-health.md` §"What to do when making breaking API changes?" (step 1 names the label explicitly). Downstream users (e.g. `datafusion-python`, plugin authors) read the labelled notes to know they must recompile against the new DataFusion major. The `version()` extern in `src/lib.rs` returns the major of workspace `CARGO_PKG_VERSION`; consumers compare it at load time and can refuse mismatched producers. Apply via `gh pr edit <num> --add-label "api change"` (label name contains a space — must be quoted). When reviewing such a PR, block merge until label present. 8. **No FFI struct changes in patch releases.** Patch releases ship from branches matching `^branch-\d+$` (e.g. `branch-53`, `branch-52`). FFI struct layout changes (anything that would earn rule 7's `api change` label) **must not** target a release branch and must not be back-ported. Patch releases are ABI-stable by contract — a consumer compiled against `53.1.0` must keep working against `53.1.1`. Before reviewing/approving an FFI PR, check the PR's base branch: if it matches the regex above, or the PR description / labels indicate patch / back-port, reject the FFI struct change and ask the author to retarget `main`. Bugfixes that do not alter struct layout (e.g. fixing a function-pointer body) are fine to back-port. Quick check: `gh pr view <num> --json baseRefName,labels --jq '.baseRefName'` then match against `^branch-\d+$`. Do **not** glob-match `branch-*` — back-port / cherry-pick working branches (e.g. `branch-53-cherry-pick-1`) also share that prefix but are not release branches; only the strict `branch-<digits>` form is the freeze target. ## The standard wrapper shape A new `FFI_X` for trait `X` must follow this template. Use `FFI_CatalogProvider` (`src/catalog_provider.rs`) as the canonical reference — it shows the full shape (codec field, nested FFI types, `FFI_Option`/`FFI_Result` returns, Arc-backed `PrivateData`) without async or capability-flag noise. `FFI_TableProvider` (`src/table_provider.rs`) covers async (`scan`, `FFI_SessionRef`, `FfiFuture`) and the one `Option<fn>` capability flag (`supports_filters_pushdown`). ### 1. The `FFI_X` struct ```rust #[repr(C)] #[derive(Debug)] pub struct FFI_X { some_method: unsafe extern "C" fn(this: &Self, ...) -> FFI_Result<...>, optional_method: Option<unsafe extern "C" fn(&Self, ...) -> FFI_Result<...>>, pub logical_codec: FFI_LogicalExtensionCodec, clone: unsafe extern "C" fn(&Self) -> Self, release: unsafe extern "C" fn(&mut Self), pub version: unsafe extern "C" fn() -> u64, private_data: *mut c_void, pub library_marker_id: extern "C" fn() -> usize, } unsafe impl Send for FFI_X {} unsafe impl Sync for FFI_X {} ``` Field rules: - **One `unsafe extern "C" fn` per trait method.** Always populate — `Arc<dyn Trait>` dispatch picks override-or-default at call time, so the producer side gets the right answer without the consumer needing to know. See § "Method coverage". - **`Option<fn>` is the capability-flag exception**, not a template. Crate uses it exactly once: `FFI_TableProvider::supports_filters_pushdown`. See § "Method coverage". - **Codec field** (`FFI_LogicalExtensionCodec` / `FFI_PhysicalExtensionCodec`) only if the trait moves `Expr`s / `LogicalPlan`s / `ExecutionPlan`s across the boundary. - **Method function pointers are private by default.** Mark `pub` only if a downstream library needs to invoke them directly (rare — typically only `version`, `library_marker_id`, embedded codecs are `pub`). - **`version: super::version` is mandatory.** Consumers gate compatibility on it. - **`library_marker_id: crate::get_library_marker_id` is mandatory *when the wrapper uses the standard `ForeignX` adapter pattern*.** Two flavors exist: - **Arc-backed (immutable / shareable traits — `TableProvider`, `ExecutionPlan`, all UDFs, codecs):** consumer-side `From<&FFI_X> for Arc<dyn X>` consults the marker to choose `Arc::clone(inner)` vs `Arc::new(ForeignX(...))`. - **Box-backed (mutable / move-only traits — `Accumulator`, `GroupsAccumulator`, `PartitionEvaluator`):** consumer-side `From<FFI_X> for Box<dyn X>` consults the marker to take the inner `Box` directly vs `Box::new(ForeignX(...))`. Producer-side `From<Box<dyn X>> for FFI_X` *also* uses an `is::<ForeignX>()` downcast as an additional re-wrap bypass; the marker check covers the reverse direction. The field is dead ABI surface — and must be omitted with a one-line module-doc rationale — only when **neither** flavor applies: no `ForeignX` adapter, no reverse `From<FFI_X> for {Arc,Box}<dyn X>`, and the trait is impl'd directly on `FFI_X`. Canonical example: `FFI_RecordBatchStream` (`impl RecordBatchStream for FFI_RecordBatchStream` at `record_batch_stream.rs:149`, no `ForeignRecordBatchStream`, no reverse `From`). Before flagging a missing `library_marker_id` as a gap, run all three greps on the wrapper file: `Foreign<wrapper-name>`, `From<&?FFI_X> for Arc<`, `From<FFI_X> for Box<`. Hit on any → marker is required and its absence is a real gap. Zero hits on all three → marker is intentionally not needed, not a gap. - **`Send`/`Sync` bounds match the wrapped trait** (rule 4). Most wrappers want both; `Send`-only for streams / mutable traits. ### 2. `PrivateData` shape Default — for read-only, shareable traits — use `Arc`: ```rust struct XPrivateData { inner: Arc<dyn X>, runtime: Option<Handle>, // include when any async method exists } ``` For traits that require `&mut self` (e.g. `Accumulator`, `GroupsAccumulator`, `PartitionEvaluator`), use `Box<dyn X>`: ```rust struct XPrivateData { inner: Box<dyn X>, runtime: Option<Handle>, // include when any async method exists } ``` A `Box`-backed `FFI_X` **cannot implement `Clone`**; document this and skip the `clone` function pointer, or hand-write a release path that distinguishes producer vs consumer side. Canonical example: `FFI_Accumulator` in `src/udaf/accumulator.rs`. See also `FFI_GroupsAccumulator` (`src/udaf/groups_accumulator.rs`) and `FFI_PartitionEvaluator` (`src/udwf/partition_evaluator.rs`). ### 3. Function-pointer wrappers Naming convention: `<method>_fn_wrapper`. ```rust unsafe extern "C" fn some_method_fn_wrapper(this: &FFI_X, ...) -> FFI_Result<T> { // 1. Recover inner via this.inner() // 2. Translate FFI types → native types // 3. Call native method // 4. Translate native Result → FFI_Result via sresult_return! or .into() } ``` ### 4. `clone` / `release` / `Drop` ```rust unsafe extern "C" fn clone_fn_wrapper(this: &FFI_X) -> FFI_X { /* re-Box new private_data, copy fn ptrs */ } unsafe extern "C" fn release_fn_wrapper(this: &mut FFI_X) { unsafe { debug_assert!(!this.private_data.is_null()); drop(Box::from_raw(this.private_data as *mut XPrivateData)); this.private_data = std::ptr::null_mut(); } } impl Drop for FFI_X { fn drop(&mut self) { unsafe { (self.release)(self) } } } impl Clone for FFI_X { fn clone(&self) -> Self { unsafe { (self.clone)(self) } } } ``` `release` must null `private_data` so a double-free debug-asserts loudly. ### 5. Constructor split ```rust impl FFI_X { pub fn new(inner: Arc<dyn X>, runtime: Option<Handle>, task_ctx_provider: impl Into<FFI_TaskContextProvider>, logical_codec: Option<Arc<dyn LogicalExtensionCodec>>) -> Self { // build FFI_LogicalExtensionCodec from defaults, then forward Self::new_with_ffi_codec(inner, runtime, ffi_codec) } pub fn new_with_ffi_codec(inner: Arc<dyn X>, runtime: Option<Handle>, logical_codec: FFI_LogicalExtensionCodec) -> Self { // Round-trip downcast: if inner is already a ForeignX, return its FFI directly. if let Some(foreign) = inner.downcast_ref::<ForeignX>() { return foreign.0.clone(); } // …allocate XPrivateData and populate fn ptrs… } } ``` The round-trip downcast is **mandatory** — without it, repeated FFI hops nest `ForeignX(FFI_X(ForeignX(...)))` and you pay the boundary cost every layer. ### 6. The `Foreign<X>` consumer ```rust #[derive(Debug)] pub struct ForeignX(pub FFI_X); unsafe impl Send for ForeignX {} unsafe impl Sync for ForeignX {} impl From<&FFI_X> for Arc<dyn X> { fn from(p: &FFI_X) -> Self { if (p.library_marker_id)() == crate::get_library_marker_id() { Arc::clone(unsafe { p.inner() }) } else { Arc::new(ForeignX(p.clone())) } } } impl X for ForeignX { /* call each fn pointer, translate types back */ } ``` The marker-id check is **mandatory** for every `From<&FFI_X> for Arc<dyn X>`. Skipping it breaks the local-bypass optimization and forces the producer's data through serialization. ### 7. Tests The crate has **two distinct test surfaces** and a new wrapper usually needs entries in both. They are not interchangeable; they catch different classes of bug. #### a. In-process unit tests (`#[cfg(test)] mod tests` inside `src/<mod>.rs`) Run on every `cargo test -p datafusion-ffi`. Producer and consumer live in the same compilation unit, so `library_marker_id` returns the same value on both sides. To force the foreign path you must override the marker: ```rust let mut ffi_x = FFI_X::new(provider, …); ffi_x.library_marker_id = crate::mock_foreign_marker_id; // forces the ForeignX branch let arc: Arc<dyn X> = (&ffi_x).into(); assert!(arc.downcast_ref::<ForeignX>().is_some()); ``` Every wrapper must include at minimum: - A **local-bypass test** — build `FFI_X` from a concrete native type, convert to `Arc<dyn X>`, `downcast_ref::<ConcreteType>()` must succeed. - A **forced-foreign test** — set `library_marker_id = crate::mock_foreign_marker_id`, convert, `downcast_ref::<ForeignX>()` must succeed, then exercise every method end-to-end. Templates: `test_ffi_table_provider_local_bypass` and `test_round_trip_ffi_table_provider_scan` in `src/table_provider.rs`. What unit tests catch: Rust-level correctness (translation logic, lifetime bugs, leaks under valgrind/miri, Send/Sync, error propagation, codec round-trips). What they **cannot** catch: real ABI bugs. Both producer and consumer share `#[repr(C)]` layout because they are the exact same struct definition in memory. #### b. Cross-library integration tests (`tests/ffi_*.rs`, gated by the `integration-tests` feature)