docs: add agent skill for datafusion-ffi crate patterns#22327
Open
timsaucer wants to merge 7 commits into
Open
docs: add agent skill for datafusion-ffi crate patterns#22327timsaucer wants to merge 7 commits into
timsaucer wants to merge 7 commits into
Conversation
Introduces `.ai/skills/` as the repository-wide location for tool-agnostic agent skills and lands a first skill, `datafusion-ffi`, that captures the crate's required wrapper patterns and flags known gaps where trait default methods are not forwarded across the FFI boundary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
timsaucer
added
documentation
github-actions
Bot
removed
documentation
`Arc<dyn Trait>` erases override-vs-default information, so the producer side cannot use `Option<fn>` to signal "trait default is in effect". Always populate the fn pointer; dynamic dispatch already picks override-or-default. Reserve `Option<fn>` for slots gated by an explicit constructor argument like `FFI_TableProvider::new`'s `can_support_pushdown_filters`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`FFI_TableProvider::supports_filters_pushdown` is the only `Option<fn>` slot in `datafusion/ffi/src/`. Earlier wording made it sound like a general template; correct the skill so contributors do not introduce new optional fn pointers without an explicit capability flag and a real reason to skip the FFI hop. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Splits the testing guidance into two clearly-named tracks: in-process unit tests in `src/<mod>.rs` (use `mock_foreign_marker_id` to force the foreign branch, run on every `cargo test`) and cross-library integration tests in `tests/ffi_*.rs` gated by the `integration-tests` feature (libloading-load the crate's own cdylib to exercise the real FFI symbol path). Adds a "which tests does my change need" table that makes integration tests mandatory for any `#[repr(C)]` layout change, since that is exactly the bug class in-process tests cannot catch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 18, 2026
Replaces the inline gap snapshot with links to issues apache#22328-apache#22337, one per wrapper with a current coverage gap. Removes the "already complete" list, which would have silently masked regressions whenever an upstream trait gained a new defaulted method. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
timsaucer
added
documentation
Contributor
|
So cool 🚀 |
github-actions
Bot
removed
documentation
coderfender
reviewed
May 19, 2026
| `Option<fn>` is used exactly once in the crate today: `FFI_TableProvider::supports_filters_pushdown`, gated by the `can_support_pushdown_filters: bool` argument to `FFI_TableProvider::new`. It is an exception, not a template. Reach for it only when (a) the producer's constructor takes an explicit capability flag and (b) skipping the FFI call is meaningfully cheaper than letting the trait default run on the producer side. Otherwise plumb the fn pointer unconditionally. | ||
|
|
||
| ### Known gaps to close | ||
|
|
Contributor
There was a problem hiding this comment.
This list might rot and point to stale github issues eventually . I was wondering if we could point to a link with FFI tag instead ? https://github.com/apache/datafusion/issues?q=is%3Aissue+is%3Aopen+label%3Affi
coderfender
reviewed
May 19, 2026
- Rule 3: split function-pointer field sig (`unsafe extern "C"`) from bare function defs in `lib.rs` (plain `extern "C"`); `version` field is unsafe, `library_marker_id` field is plain. - Rule 4: replace absolute "Send + Sync on every FFI_X" with bounds derived from the wrapped trait. Streams + mutable traits (`Accumulator`, `GroupsAccumulator`, `PartitionEvaluator`) are `Send`-only. - Rule 7: cite `docs/source/contributor-guide/api-health.md` for the release-note convention behind the `api-change` label. - Rule 8 + checklist 15: match release branches with strict `^branch-\d+$` regex; explicitly reject globbing `branch-*` (collides with `branch-53-cherry-pick-1`). - Template (`### 1. The FFI_X struct`): compress the `Option<fn>` block to a one-line cross-ref; add a parallel `Box<dyn X>` code block + canonical example paths under `### 2. PrivateData shape`. - Checklist item 1 + 6: align with new method-coverage and Send/Sync rules. - Integration-test table: replace "pure-stabby-string getters can skip" with a defined allow-list (primitives, FFI enums, bare `SString`) plus an explicit non-skippable list. - Known gaps: drop static issue enumeration; point to the live `label:ffi` GitHub query as source of truth, with a `gh` one-liner and severity classes so the section still teaches without rotting. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
None.
Rationale for this change
When writing FFI code, there are a variety of established patterns that it is important to follow. This includes, but is not limited to, things like ensuring we do not make FFI struct changes on patch releases and ensuring we are following best practices for checking round trip trait implementations. The goal of this PR is to add an agent skill to aid both developers and code reviewers to find issues before they make it into the code base.
What changes are included in this PR?
Are these changes tested?
Yes, I have run the skill and it has already identified gaps in the current implementation. Issues are opened for current gaps and linked back to this PR for originating them.
Are there any user-facing changes?
None