feat: Add FFI_QueryPlanner to support foreign query planners across shared-library boundaries#22151
Open
milenkovicm wants to merge 1 commit into
Open
feat: Add FFI_QueryPlanner to support foreign query planners across shared-library boundaries#22151milenkovicm wants to merge 1 commit into
FFI_QueryPlanner to support foreign query planners across shared-library boundaries#22151milenkovicm wants to merge 1 commit into
Conversation
FFI_QueryPlanner to support foreign query planners across shared-library boundariesFFI_QueryPlanner to support foreign query planners across shared-library boundaries
|
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 |
ee61d0f to
8e23f2c
Compare
Co-authored-by: Tim Saucer <timsaucer@gmail.com>
8e23f2c to
d49d1bc
Compare
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
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.
Which issue does this PR close?
This is follow up on #20249 which was co-authored with @timsaucer
Closes #. (pending)
Rationale for this change
DataFusion's FFI layer allows plugins and foreign libraries to extend query processing across shared-library boundaries. However, there was no way for a foreign library to participate in the physical planning pipeline —
QueryPlannerrequired a concrete &SessionState, which cannot cross an FFI boundary.What changes are included in this PR?
FFI_QueryPlanner— an FFI-saferepr-Cstruct that wraps a foreignQueryPlannerimplementation, serializing theLogicalPlanvia datafusion-proto across the boundary and deserializing the resultingExecutionPlan.QueryPlannerWeaktrait — a variant ofQueryPlannerthat accepts&dyn Sessioninstead of&SessionState, making it implementable by foreign code. It make more sense to create new interface rather than changingQueryPlanner. The change would introduce big backward incompatibility, also we can argue that havingSessionStateas parameter makes sense.QueryPlannerwith anAnybound to allow downcasting (needed byDynamicForeignQueryPlanner).FFI_QueryPlannerviaQueryPlannerWeakand the deferred-injection pattern viaDynamicForeignQueryPlanner.The
DynamicForeignQueryPlannerhelper (in the integration tests) shows how to break the construction-time dependency cycle:SessionContextneeds aQueryPlannerat build time, but theFFIplanner needs the codec which needs the context. The placeholder is registered at build time and swapped for the real FFI planner once both sides exist.Are these changes tested?
Yes. Integration tests are added in
datafusion/ffi/tests/ffi_planner.rs:test_ffi_query_planner— verifies that a foreign planner loaded from a shared library can produce a valid ExecutionPlan from a LogicalPlan.test_ffi_dynamic_query_planner— verifies the deferred-injection pattern where the FFI planner is installed into an already-constructed SessionContext viaDynamicForeignQueryPlanner.Are there any user-facing changes?
Yes, one change to a public trait:
QueryPlannernow requires Any as a supertrait (enables downcasting). This is not required for this implementation but it might be needed for implementorsOpen Questions
SessionContextis needed to create aQueryPlannerbutQueryPlanneris required to create aSessionContext(I hope I did not get that horrible wrong)QueryPlannerWeakis debatable, I cant come up with better name