[branch-55] fix: preserve provided arguments during FFI object construction (#24723) - #24752
Merged
timsaucer merged 1 commit intoAug 28, 2026
Conversation
…he#24723) ## Which issue does this PR close? - Closes apache#24722. ## Rationale for this change Three `datafusion-ffi` constructors unwrap an already-foreign input and return its original handle, dropping the arguments passed alongside without an error or a warning: - `FFI_LogicalExtensionCodec::new` — discards `task_ctx_provider` - `FFI_PhysicalExtensionCodec::new` — discards `task_ctx_provider` - `FFI_TableProvider::new_with_ffi_codec` — discards `logical_codec` The consequence is that a consumer which imports a foreign codec can never rebind it. Re-wrapping with a different provider compiles, runs, and has no effect, so the handle keeps resolving against whatever session it was first built with. In `datafusion-python` that shows up as decode callbacks resolving names against a pre-fork session: a UDF registered after the fork is invisible to them, and the config they see is a stale snapshot. There is a second failure mode with the same root cause. The provider is held as a `Weak`, so a consumer that cannot rebind must keep the original session alive artificially or the capsule starts failing with `TaskContextProvider went out of scope over FFI boundary`. The two sibling constructors that hit the same case already do the opposite — `FFI_QueryPlanner::new_with_ffi_codecs` and `FFI_SessionRef::new_with_ffi_codecs` both adopt the supplied codecs on the unwrap path, and the former documents that guarantee explicitly. This PR makes the other three consistent with them. ## What changes are included in this PR? On the already-foreign path, each of the three constructors now clones the original handle and overwrites the relevant `#[repr(C)]` field before returning it, matching `FFI_QueryPlanner::new_with_ffi_codecs`: ```rust if let Some(codec) = (Arc::clone(&codec) as Arc<dyn Any>) .downcast_ref::<ForeignLogicalExtensionCodec>() { let mut codec = codec.0.clone(); codec.task_ctx_provider = task_ctx_provider.into(); return codec; } ``` The `runtime` argument is a deliberate exception. Unlike the codecs and the task context provider, `runtime` lives in `private_data`, which belongs to the library that owns the handle — this side cannot write it without an ABI change. `FFI_SessionRef::new_with_ffi_codecs` already takes the same position ("retaining its original private data and runtime"). Rather than leave that silent, all three constructors now document it, alongside the new adopt-on-unwrap guarantee. No public signatures change, and no behavior changes on the non-foreign path. ## Are these changes tested? Yes — five new unit tests, one per behavior, in each affected module's own test module. All five fail on `main` and pass here. - `ffi_logical_extension_codec_rebind_adopts_task_ctx_provider` - `ffi_logical_extension_codec_rebind_releases_original_session` — covers the dangling-`Weak` failure mode: session A is dropped after the rebind, and the handle stays usable - `ffi_physical_extension_codec_rebind_adopts_task_ctx_provider` - `test_rebind_foreign_table_provider_adopts_logical_codec` - `test_rebind_foreign_query_planner_adopts_codecs` — a control over the already-correct sibling, so the two paths stay in agreement Worth flagging for reviewers, since it is easy to write a test here that silently proves nothing: `impl From<&FFI_LogicalExtensionCodec> for Arc<dyn LogicalExtensionCodec>` compares `library_marker_id` first and returns the original local `Arc` on a match, so within one library the foreign branch is never reached. Each test overrides `library_marker_id` with `crate::mock_foreign_marker_id` and asserts the import really did produce a `Foreign*` wrapper before exercising the rebind. `cargo test -p datafusion-ffi --all-features` passes (152 tests). ## Are there any user-facing changes? Yes, a behavior change, though it replaces a silent no-op with the documented intent. Callers that pass a `task_ctx_provider` or `logical_codec` to these constructors alongside an already-foreign input previously had that argument ignored; it now takes effect. Anything relying on the old handle being returned untouched would see the change — but since the old path gave no way to observe or opt into that, it is hard to depend on deliberately. Downstream, this lets `datafusion-python` drop the workaround in apache/datafusion-python#1677, which retains the pre-fork `SessionContext` purely to keep the `Weak` valid. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## branch-55 #24752 +/- ##
===========================================
Coverage 81.17% 81.17%
===========================================
Files 1110 1110
Lines 386935 387043 +108
Branches 386935 387043 +108
===========================================
+ Hits 314077 314173 +96
- Misses 54366 54376 +10
- Partials 18492 18494 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
23 tasks
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
3 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.
This is a back port of #24723 onto
branch-55to supportdatafusion-pythonupgrade to 55.1.0. The details can be found in the linked PR. This is needed for apache/datafusion-python#1677