Conversation
|
PR title does not match the required pattern. Please ensure you follow the conventional commits spec. Your title should start with Title: Your title should start with Title: Your title should start with Title: Your title should start with Title: |
| @@ -0,0 +1,79 @@ | |||
| syntax = "proto3"; | |||
There was a problem hiding this comment.
Proto is the most convenient option for passing plan nodes across FFI, however we need to figure out how to keep the proto in sync between Rust and Java (i.e some way for Java side to import the proto?)
| /// Construct only as a [`CNextBytes::Some`] payload; the [`Drop`] impl (which invokes | ||
| /// `free`) runs whenever a value of this type goes out of scope on the Rust side. | ||
| #[repr(C)] | ||
| pub struct EngineAllocatedBytes { |
There was a problem hiding this comment.
Custom type that allows us to free the Java-allocated memory for each byte array
| pub struct CEngineDataIterator { | ||
| pub state: NullableCvoid, | ||
| pub next: extern "C" fn(state: NullableCvoid) -> CNextEngineData, | ||
| } | ||
|
|
||
| /// An engine-implemented streaming iterator of byte buffers. See [`CEngineDataIterator`] for | ||
| /// state semantics; cleanup is performed by [`CPlanResultWrapper::free`]. | ||
| #[repr(C)] | ||
| pub struct CBytesIterator { | ||
| pub state: NullableCvoid, | ||
| pub next: extern "C" fn(state: NullableCvoid) -> CNextBytes, | ||
| } |
There was a problem hiding this comment.
Provides state needed for Rust to move the iterator forward. Rather than adding another free function to the iter, this POC chooses to use the free function on the wrapping PlanResultWrapper. Slightly sketchy, but simplifies the code a bit.
| /// recognized -- it maps to [`Error::FileNotFound`] so that kernel code paths which | ||
| /// branch on `FileNotFound` (e.g. snapshot loading retry logic) behave correctly when | ||
| /// the engine reports a missing file. | ||
| fn engine_error_from_message(msg: String) -> Error { |
There was a problem hiding this comment.
Error handling needs to be fleshed out more, currently this is just a workaround. There is kernel logic that expects specific Error types to be returned for things like FileNotFound
| state, | ||
| free, | ||
| } = (self.callback)(self.context, slice); | ||
| let cleanup = PlanResultCleanup { state, free }; |
There was a problem hiding this comment.
A "guard" that gets passed to each iter so that the entire PlanResult (and all nested structs) gets cleaned up when the iterator is dropped.
| /// returned engine is a [`PlanBasedEngine`] backed by it; otherwise it is a [`DefaultEngine`]. | ||
| #[cfg(feature = "default-engine-base")] | ||
| fn get_default_engine_impl( | ||
| fn build_engine_impl( |
There was a problem hiding this comment.
Updated to allow optionally specifying a plan_executor, in which case we will build a PlanBasedExecutor instead.
1 participant
Summary
This PR exposes the ability to provide a plan executor through FFI. Specifically:
FfiPlanExecutorimplements thePlanExecutorinterface and wraps an upcall function pointer to allow plan execution to happen in an external language (i.e Java).repr(c)types that Java-side can use to pass data down to Rust, namelyCPlanResultWrapper,CPlanResult, and nested types allow Java to construct the result of plan execution and return it to Rust side.Memory Management
freeupcall function pointer to Rust that can be invoked when the Rust-side object is dropped.freefunction on thePlanResultWrapperrather than a separatefreefunction on each possible return struct.Other Callouts
How was this change tested?
AI generated tests - just a POC