fix(ffi): prevent recursive session physical planning

[goutamadwant]

Which issue does this PR close?

• ClosesFFI Session::create_physical_plan could recurse infinitely #24065.

Rationale for this change

ForeignSession::create_physical_plan forwarded through the owning session. If that session had installed the calling library's query planner, the callback re-entered the same planner and could recurse until the stack was exhausted. The callback also returned an FFI_ExecutionPlan, which cannot reconstruct built-in nodes with the receiving library's Rust type identities for downcasting.

The supported delegation boundary is FFI_QueryPlanner: the session owner exports its original planner before installing a foreign planner, and the foreign planner retains that handle. This boundary serializes physical plans and reconstructs them with local type identities.

What changes are included in this PR?

• Retain the legacy create_physical_plan callback at its original position and signature in FFI_SessionRef for DataFusion 55 ABI compatibility, while making it return an actionable NotImplemented error without invoking the installed planner.
• Make ForeignSession::create_physical_plan return the same error without crossing the FFI boundary or invoking the installed planner.
• Document the captured-FFI_QueryPlanner migration in the module documentation and DataFusion 56 upgrade guide.
• Add unit and cross-library regression coverage for the rejected direct-delegation path and the successful captured-planner path.

#24690 added the session's logical extension codec to the callback and fixed planning for logical plans that require custom codec support. It does not address the two remaining problems covered here: forwarding can re-enter the installed foreign planner, and the returned FFI_ExecutionPlan does not reconstruct built-in nodes with the receiving library's Rust type identities. This PR preserves the callback slot for ABI compatibility but makes direct session planning unsupported, so its callback-specific codec wiring is no longer exercised; other FFI codec work remains outside this PR.

Are these changes tested?

Yes.

• The focused unit regression verifies that both ForeignSession::create_physical_plan and the retained legacy callback return NotImplemented with zero planner re-entry.
• The three-library dlopen regression verifies that direct foreign-session delegation is rejected while the captured FFI_QueryPlanner route succeeds and restores local, downcastable physical-plan nodes.
• cargo test -p datafusion-ffi --features integration-tests
• cargo clippy -p datafusion-ffi --all-targets --all-features -- -D warnings
• cargo clippy --all-targets --all-features -- -D warnings
• RUST_BACKTRACE=1 cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-cli --workspace --lib --tests --bins --features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption
• RUSTDOCFLAGS="-D warnings" cargo doc -p datafusion-ffi --all-features --no-deps
• cargo fmt --all -- --check
• ./ci/scripts/doc_prettier_check.sh

Are there any user-facing changes?

Yes. ForeignSession::create_physical_plan and the retained legacy callback now return NotImplemented; callers must retain and invoke the session owner's exported FFI_QueryPlanner. The FFI_SessionRef callback remains at its original ABI position and signature, so this PR does not change the struct layout or require consumers to rebuild solely because of an ABI layout change.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 91.42857% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.61%. Comparing base (35f58f5) to head (b66acf8).

Files with missing lines	Patch %	Lines
datafusion/ffi/src/session/mod.rs	91.42%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #24492 +/- ##
=======================================
Coverage 81.61% 81.61% =======================================
Files 1124 1124 Lines 411978 411977 -1 Branches 411978 411977 -1 =======================================
+ Hits 336236 336241 +5 + Misses 55936 55935 -1 + Partials 19806 19801 -5 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[kosiew]

@goutamadwant
Can you resolve the merge conflicts?

[goutamadwant]

@kosiew done. thanks!

[kosiew]

@goutamadwant,

Thanks for working on this. The planner delegation direction makes sense, but I found one ABI compatibility issue that I think needs to be addressed before this can land. I also left one non-blocking suggestion around strengthening the cross-library coverage.

[kosiew]

I think we need to keep this callback slot for ABI compatibility. Removing it changes the #[repr(C)] FFI_SessionRef layout while the workspace is still at 55.0.0.

A separately compiled 55.x consumer would still interpret this old slot as create_physical_plan, so it could read create_physical_expr as that callback and every field after it would be shifted. That can result in function pointers being called with the wrong signatures, which is UB.

Could we keep the callback field in the struct and have its wrapper return the new NotImplemented error instead? The other option would be to treat this as an explicitly versioned ABI break and add compatible-version gating.

[goutamadwant]

@kosiew Addressed in latest changes. I restored create_physical_plan at its exact original position and signature in FFI_SessionRef, including initialization through the construction and clone paths. The retained callback now returns NotImplemented without invoking the installed planner, preserving the DataFusion 55 layout and preventing shifted function-pointer calls. Let me know if this is good now. thanks!

[kosiew]

Could we consider moving this assertion into the cross-library query-planner integration path, or add a small cross-library case for it? That would verify that a foreign planner gets the expected NotImplemented result when it tries direct session delegation, while the retained-planner path still works correctly across dlopen.

[goutamadwant]

@kosiew I extended the existing three-library dlopen planner-swap integration test to verify that direct foreign-session delegation returns DataFusionError::NotImplemented, then continues through the captured FFI_QueryPlanner and successfully reconstructs local, downcastable execution-plan nodes. The unit regression also invokes the retained callback directly and verifies zero planner re-entry. Hope this covers it. :)

[kosiew]

@goutamadwant
Can you resolve the merge conflicts?

[goutamadwant]

@goutamadwant Can you resolve the merge conflicts?

@kosiew conflicts are resolved. can you re-check it when you have time? thanks!

[kosiew]

@goutamadwant,

Thanks for addressing the follow-up feedback. I reviewed the latest changes and everything looks good from my side.

The ABI layout concern is addressed. FFI_SessionRef::create_physical_plan is back in its original DataFusion 55 position with the original FfiFuture<FFI_Result<FFI_ExecutionPlan>> signature. The retained callback now returns NotImplemented safely without re-entering the installed planner, and the direct-slot regression test covers that behavior.

The cross-library coverage also looks good. The existing three-library dlopen planner-swap test now verifies both sides of the contract: direct foreign-session delegation fails safely with NotImplemented, while delegation through the captured planner still succeeds and round-trips nodes with local type identities so they remain downcastable.

I don't have any additional findings. The unconditional NotImplemented behavior comes from the previously reviewed recursion fix, and this follow-up preserves that intended behavior while restoring ABI compatibility.

Thanks for the updates!

[kosiew]

🚀
@goutamadwant
Thank you for your contribution.