Skip to content

Commit 6c47cf4

Browse files
timsaucerclaude
andcommitted
docs(ffi-skill): swap canonical example to FFI_CatalogProvider, trim template comments
FFI_TableProvider has async, capability-flag, and session-handling surface that obscures the structural pattern. FFI_CatalogProvider shows the full shape (codec, nested FFI types, FFI_Option/FFI_Result, Arc-backed PrivateData) without that noise; cite FFI_TableProvider only in the sections where its complexity earns its keep. Also strip per-field comments from the struct skeleton — they duplicated source doc-comments and pattern guidance now lives in the "Field rules" bullet list below. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 08b9100 commit 6c47cf4

1 file changed

Lines changed: 10 additions & 17 deletions

File tree

.ai/skills/datafusion-ffi/SKILL.md

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -32,25 +32,16 @@ Trigger this skill any time the work touches `datafusion/ffi/`:
3232

3333
## The standard wrapper shape
3434

35-
A new `FFI_X` for trait `X` must follow this template. Use `FFI_TableProvider` (src/table_provider.rs) and `FFI_CatalogProvider` (src/catalog_provider.rs) as canonical references.
35+
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`).
3636

3737
### 1. The `FFI_X` struct
3838

3939
```rust
4040
#[repr(C)]
4141
#[derive(Debug)]
4242
pub struct FFI_X {
43-
// One unsafe extern "C" fn per trait method (see § "Method coverage" below).
44-
// Always populate this — calling through `Arc<dyn Trait>` dispatches to the
45-
// override if there is one, else to the trait default. The producer side
46-
// gets the right answer either way without the consumer needing to know.
4743
some_method: unsafe extern "C" fn(this: &Self, ...) -> FFI_Result<...>,
48-
49-
// `Option<fn>` is the capability-flag exception — see § "Method coverage".
50-
// Crate uses it exactly once: `FFI_TableProvider::supports_filters_pushdown`.
5144
optional_method: Option<unsafe extern "C" fn(&Self, ...) -> FFI_Result<...>>,
52-
53-
// Codec only if the trait moves Exprs/Plans across the boundary.
5445
pub logical_codec: FFI_LogicalExtensionCodec,
5546

5647
clone: unsafe extern "C" fn(&Self) -> Self,
@@ -61,17 +52,19 @@ pub struct FFI_X {
6152
pub library_marker_id: extern "C" fn() -> usize,
6253
}
6354

64-
// Pick bounds to match the wrapped trait (see rule 4).
65-
// Most wrappers want both; `Send`-only for streams / mutable traits.
6655
unsafe impl Send for FFI_X {}
6756
unsafe impl Sync for FFI_X {}
6857
```
6958

70-
Notes:
59+
Field rules:
7160

72-
- Method function pointers are private by default. Mark `pub` only if a downstream library needs to invoke them directly (rare — usually only `version`, `library_marker_id`, embedded codecs are public).
73-
- `version: super::version` is mandatory. Consumers gate compatibility on it.
74-
- `library_marker_id: crate::get_library_marker_id` is mandatory. It powers local-bypass.
61+
- **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".
62+
- **`Option<fn>` is the capability-flag exception**, not a template. Crate uses it exactly once: `FFI_TableProvider::supports_filters_pushdown`. See § "Method coverage".
63+
- **Codec field** (`FFI_LogicalExtensionCodec` / `FFI_PhysicalExtensionCodec`) only if the trait moves `Expr`s / `LogicalPlan`s / `ExecutionPlan`s across the boundary.
64+
- **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`).
65+
- **`version: super::version` is mandatory.** Consumers gate compatibility on it.
66+
- **`library_marker_id: crate::get_library_marker_id` is mandatory.** Powers local-bypass.
67+
- **`Send`/`Sync` bounds match the wrapped trait** (rule 4). Most wrappers want both; `Send`-only for streams / mutable traits.
7568

7669
### 2. `PrivateData` shape
7770

@@ -341,7 +334,7 @@ When reviewing a PR that touches `datafusion/ffi/`:
341334
## References
342335

343336
- Crate README: `datafusion/ffi/README.md` — vocabulary + memory-model rationale.
344-
- Canonical wrappers to model after: `src/table_provider.rs`, `src/catalog_provider.rs`.
337+
- Canonical wrapper to model after: `src/catalog_provider.rs`. Async + capability-flag variants: `src/table_provider.rs`.
345338
- Mutable-trait variant: `src/udaf/accumulator.rs` (`Box<dyn Accumulator>`).
346339
- Optional-method pattern: `FFI_TableProvider::supports_filters_pushdown`.
347340
- Codec wiring: `src/proto/logical_extension_codec.rs`, `src/proto/physical_extension_codec.rs`.

0 commit comments

Comments
 (0)