feat: Implement FFI_QueryPlanner - #24028
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.
|
|
||
| #[cfg(not(feature = "parquet_encryption"))] | ||
| #[expect(dead_code)] | ||
| #[expect(clippy::unused_async)] |
There was a problem hiding this comment.
Drive by fix to when you run clippy and don't have the parquet_encryption feature enabled.
| } | ||
|
|
||
| #[cfg(not(feature = "parquet_encryption"))] | ||
| #[expect(clippy::unused_async)] |
There was a problem hiding this comment.
Drive by fix to when you run clippy and don't have the parquet_encryption feature enabled.
| } | ||
|
|
||
| #[cfg(not(feature = "parquet_encryption"))] | ||
| #[expect(clippy::unused_async)] |
There was a problem hiding this comment.
Drive by fix to when you run clippy and don't have the parquet_encryption feature enabled.
|
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 |
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24028 +/- ##
==========================================
+ Coverage 80.85% 80.87% +0.01%
==========================================
Files 1101 1102 +1
Lines 374933 375192 +259
Branches 374933 375192 +259
==========================================
+ Hits 303166 303435 +269
+ Misses 53671 53644 -27
- Partials 18096 18113 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
paleolimbot
left a comment
There was a problem hiding this comment.
I will try to circle back to take a closer look at the details, but I did take two passes and didn't spot anything out of place here. You've documented the motivation nicely here and conceptually I think this is a great approach that allows quite a lot of flexibility for multiple datafusion-based libraries to interact. The FFI pattern you've established from previous PRs works well here.
I believe this will allow our (SedonaDB) spatial join to be used in datafusion-python plans (injected via a combination of logical optimizer rules and the query planner). Exciting!
milenkovicm
left a comment
There was a problem hiding this comment.
Hey @timsaucer first of all appologies for late review and thank you for sticking with this. Can't wait for this release to finally get py ballista support.
I have fiew minor questions, more for my understanding than blockers
|
|
||
| /// Library A's logical codec stores library B's provider while the logical | ||
| /// plan crosses into library C. A real application would encode enough | ||
| /// metadata to reconstruct or locate the provider instead. |
There was a problem hiding this comment.
just for my reference, which metadata is needed? will need some help to integrate this with py ballista
| &Self, | ||
| logical_plan_serialized: SVec<u8>, | ||
| session: FFI_SessionRef, | ||
| ) -> FfiFuture<FFI_Result<SVec<u8>>>, |
There was a problem hiding this comment.
One question, why FFI_Result<FFI_ExecutionPlan> not used instad of serialized plan?
Which issue does this PR close?
datafusion-distributedwithdatafusion-pythondatafusion-python#1612Rationale for this change
This is the last in a series of PRs that would enable FFI
Sessionto support aQueryPlanner. The prior work was inPhysicalPlanningContextexplicitly through planner traits #23649With those changes in place we now have the dependencies correct that we can expose a
FFI_QueryPlanneron aFFI_Session. With this we can enable foreign libraries such asdatafusion-distributedandballistato provide a query planner in Python and connect it directly to adatafusion-python'sSessionContext.What changes are included in this PR?
Addition only. Adds these functions to
FFI_Sessionand their supporting structures:query_planner()optimize()physical_optimizers()Are these changes tested?
Unit and integration tests are provided.
Are there any user-facing changes?
This is addition, but it does break the FFI ABI, which is already evolving in DF55.