fix(ffi): prevent recursive session physical planning - #24492
fix(ffi): prevent recursive session physical planning#24492goutamadwant wants to merge 5 commits into
Conversation
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
|
@goutamadwant |
|
@kosiew done. thanks! |
kosiew
left a comment
There was a problem hiding this comment.
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.
| logical_plan_serialized: SVec<u8>, | ||
| ) -> FFI_Result<SVec<u8>>, | ||
|
|
||
| create_physical_plan: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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!
| let batch = record_batch!(("a", Int32, [1, 2, 3]))?; | ||
| let table = MemTable::try_new(schema, vec![vec![batch]])?; | ||
| ctx.register_table("test_table", Arc::new(table))?; | ||
| async fn test_foreign_session_rejects_create_physical_plan() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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. :)
|
@goutamadwant |
Which issue does this PR close?
Session::create_physical_plancould recurse infinitely #24065.Rationale for this change
ForeignSession::create_physical_planforwarded 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 anFFI_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?
create_physical_plancallback at its original position and signature inFFI_SessionReffor DataFusion 55 ABI compatibility, while making it return an actionableNotImplementederror without invoking the installed planner.ForeignSession::create_physical_planreturn the same error without crossing the FFI boundary or invoking the installed planner.FFI_QueryPlannermigration in the module documentation and DataFusion 56 upgrade guide.#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_ExecutionPlandoes 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.
ForeignSession::create_physical_planand the retained legacy callback returnNotImplementedwith zero planner re-entry.dlopenregression verifies that direct foreign-session delegation is rejected while the capturedFFI_QueryPlannerroute succeeds and restores local, downcastable physical-plan nodes.cargo test -p datafusion-ffi --features integration-testscargo clippy -p datafusion-ffi --all-targets --all-features -- -D warningscargo clippy --all-targets --all-features -- -D warningsRUST_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_encryptionRUSTDOCFLAGS="-D warnings" cargo doc -p datafusion-ffi --all-features --no-depscargo fmt --all -- --check./ci/scripts/doc_prettier_check.shAre there any user-facing changes?
Yes.
ForeignSession::create_physical_planand the retained legacy callback now returnNotImplemented; callers must retain and invoke the session owner's exportedFFI_QueryPlanner. TheFFI_SessionRefcallback 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.