feat(ffi): add an FFI extension codec bundle - #24108
Draft
timsaucer wants to merge 18 commits into
Draft
Conversation
…ncryption feature is not enabled
Add FFI_QueryPlanner and ForeignQueryPlanner with logical and physical plan codec support. Forward query planners, logical optimization, and physical optimizer rules through foreign sessions, with unit and cross-library coverage.\n\nAI Disclosure: This code was written in part by an AI agent.
Keep the standard FFI_SessionRef constructor focused on the required logical codec and derive a default physical codec. Add an explicit constructor for callers that already own matching logical and physical codecs.\n\nAI Disclosure: This code was written in part by an AI agent.
Document the query planner serialization boundary and exercise an A/B/C ownership model with independently loaded cdylib images. Reuse the existing FFI table provider and verify foreign plans are reconstructed as local nodes through A's codecs. AI Disclosure: This code was written in part by an AI agent.
Use one test query planner for both the basic round trip and the three-library table scan scenario. Remove the redundant library C constructor from the integration-test module. AI Disclosure: This code was written in part by an AI agent.
Render the private FFI_SessionRef type as code so public query planner documentation passes rustdoc's private intra-doc link checks. AI Disclosure: This code was written in part by an AI agent.
Library C typically captures library A's query planner, then A installs C's planner on its session. C plans by delegating back to the captured handle. This is the deployment that requires serialized plans in both directions: C must downcast the nodes A produced in order to rewrite them, and A must downcast the nodes C produced to run its own passes over the result. Add an integration test for that topology. Replacing either serialization step with an FFI_ExecutionPlan handoff makes it fail, which the prior tests could not detect on the inbound leg. The test also asserts that after the swap the session reports C's own planner, documenting why C must delegate to the captured handle rather than call Session::query_planner or Session::create_physical_plan, both of which are self-references at that point. Widen the test physical codec to accept an A-local node during encode. FFI_ExecutionPlan::new unwraps a ForeignExecutionPlan back to its origin handle, so when C serializes a node A gave it, A is asked to encode the very plan its own try_decode produced. Take the delegate planner as FFI_Option<FFI_QueryPlanner> on the existing create_query_planner module entry instead of adding a second entry point. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Drop redundant `Send`/`Sync` bounds and remove two codec footguns in the FFI query planner API, per review on apache#24028. - `Session` already requires `Send + Sync`, so `&(dyn Session + Send + Sync)` was noise. Narrowing to `&dyn Session` also widens what callers can pass. - `LogicalExtensionCodec` and `PhysicalExtensionCodec` already require `Send`. The `+ Send` on the codec constructor parameters bought nothing and blocked callers holding an existing `Arc<dyn PhysicalExtensionCodec>`, since Rust will not coerce that to `Arc<dyn PhysicalExtensionCodec + Send>`. - `FFI_QueryPlanner::new_with_ffi_codecs` silently dropped the supplied codecs when re-exporting an already-foreign planner. It now adopts them while keeping the original planner identity. - `FFI_QueryPlanner::new` no longer takes `Option` codecs. Passing `None` used to install the default codecs, clobbering extension-node handling; requiring an explicit codec makes that unrepresentable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AI Disclosure: This code was written in part by an AI agent.:
AI Disclosure: This code was written in part by an AI agent.:
AI Disclosure: This code was written in part by an AI agent.: AI Disclosure: This code was written in part by an AI agent.:
AI Disclosure: This code was written in part by an AI agent.:
AI Disclosure: This code was written in part by an AI agent.:
AI Disclosure: This code was written in part by an AI agent.:
AI Disclosure: This code was written in part by an AI agent.:
Serializing plans across an FFI boundary needs three values that must agree with one another: a task context provider, a logical extension codec, and a physical extension codec. The wrappers passed them separately, and the ones that carried only a logical codec synthesized a DefaultPhysicalExtensionCodec when they had to export a session. A query planner reached through such a session could not round-trip custom physical extension nodes; it failed later with "PhysicalExtensionCodec is not provided". The synthesized codec also borrowed the logical codec's task context provider, which is not guaranteed to represent the exported session. Add FFI_ExtensionCodecBundle, which carries all three as one unit with private fields so the constructors are the only way to pair them. The bundle owns no private data of its own; each member already carries its own lifecycle pointers and marker. The dependency direction is bundle to codecs to task context provider — a bundle inside a codec would make cloning recurse forever. Propagate it through every wrapper that exports a session or builds a nested provider: FFI_TableProvider, FFI_TableProviderFactory, FFI_TableFunction, FFI_CatalogProvider, FFI_CatalogProviderList, FFI_SchemaProvider, FFI_SessionRef, and FFI_QueryPlanner. Each keeps one constructor taking the bundle; the new_with_ffi_codec(s) variants and the Option<Arc<dyn LogicalExtensionCodec>> argument whose None meant "default" are gone, so choosing the defaults is now explicit. The two paths inside FFI_LogicalExtensionCodec that rebuild a table provider receive only the codec, so they pair it with an explicit default physical codec and document what that costs a consumer. Session::create_physical_plan serialized its plan with no extension codec on either side of the boundary; it now uses the bundle's logical codec, matching optimize and create_physical_expr. Tests: a three-library integration test where library A owns the session and a custom physical codec, installs library C's planner, and queries library B's provider, which reaches that planner through the session A handed it and returns a custom physical extension node. Restoring the old default-codec behaviour makes it fail with "PhysicalExtensionCodec is not provided". Plus unit coverage that nested catalog/schema/table construction preserves the bundle and that an expired task context provider reports a clear error. BREAKING CHANGE: these FFI struct layouts changed. Rebuild FFI providers and consumers against this release. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
documentation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24108 +/- ##
==========================================
+ Coverage 80.91% 80.93% +0.01%
==========================================
Files 1103 1105 +2
Lines 377219 377579 +360
Branches 377219 377579 +360
==========================================
+ Hits 305244 305589 +345
+ Misses 53775 53769 -6
- Partials 18200 18221 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
2 participants
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.
Note
This PR is stacked on top of #24028 (
feat: Implement FFI_QueryPlanner) andcontains that PR's commits, because a cross-repository PR cannot use a fork
branch as its base. Only the final commit,
feat(ffi): add an FFI extension codec bundle, is new here. Please review #24028 first; this PR will shrink toa single commit once that one merges.
Which issue does this PR close?
Rationale for this change
Serializing plans across an FFI boundary needs three values that must agree with
one another: a task context provider, a logical extension codec, and a physical
extension codec. The wrappers took them separately, and the ones that carried
only a logical codec synthesized a
DefaultPhysicalExtensionCodecwhenever theyhad to export a session.
The user-visible consequence: a foreign library that provides a query planner
returning a custom physical extension node cannot get that node back across the
boundary. A consumer that reaches
Session::query_plannerthrough an exportedsession gets a planner whose physical codec is the default one, and the query
fails with
PhysicalExtensionCodec is not provided. The synthesized codec alsoborrowed the logical codec's task context provider, which is not guaranteed to
represent the exported session.
What changes are included in this PR?
Adds
FFI_ExtensionCodecBundle, which carries the task context provider and bothcodecs as one unit with private fields, so the constructors are the only way to
pair them. The bundle owns no private data of its own; each member already carries
its own lifecycle pointers and library marker. The dependency direction is bundle
→ codecs → task context provider — a bundle stored inside a codec would make
cloning recurse forever.
The bundle is propagated through every wrapper that exports a session or builds a
nested provider:
FFI_TableProvider,FFI_TableProviderFactory,FFI_TableFunction,FFI_CatalogProvider,FFI_CatalogProviderList,FFI_SchemaProvider,FFI_SessionRef, andFFI_QueryPlanner. Each keeps oneconstructor taking the bundle; the
new_with_ffi_codec(s)variants and theOption<Arc<dyn LogicalExtensionCodec>>argument whoseNonemeant "use thedefault" are gone, so choosing the defaults is now explicit
(
FFI_ExtensionCodecBundle::new_default).The two paths inside
FFI_LogicalExtensionCodecthat rebuild a table providerreceive only the codec, so they pair it with an explicit default physical codec
and document what that costs a consumer.
Session::create_physical_planserialized its plan with no extension codec oneither side of the boundary; it now uses the bundle's logical codec, matching
optimizeandcreate_physical_expr.Are these changes tested?
Yes.
physical codec, installs library C's planner, and queries library B's provider,
which reaches that planner through the session A handed it and returns a custom
physical extension node. Restoring the old default-codec behaviour makes it fail
with
PhysicalExtensionCodec is not provided.preserves the bundle, that cloning does not nest foreign codec wrappers, and
that an expired task context provider reports a clear error rather than
panicking.
Are there any user-facing changes?
Yes — this is a breaking change to the
datafusion-ffipublic API and to theFFI_struct layouts. FFI providers and consumers must both be rebuilt againstDataFusion 55. The ABI is already evolving in DF55.
Documented in
docs/source/library-user-guide/upgrading/55.0.0.mdwithbefore/after migration examples, and in
datafusion/ffi/README.md. Theapi changelabel applies.