kosiew commented on code in PR #22584:
URL: https://github.com/apache/datafusion/pull/22584#discussion_r3332792593
##########
datafusion/ffi/src/physical_optimizer.rs:
##########
@@ -98,6 +198,38 @@ unsafe extern "C" fn optimize_fn_wrapper(
FFI_Result::Ok(FFI_ExecutionPlan::new(optimized_plan, runtime))
}
+unsafe extern "C" fn optimize_with_context_fn_wrapper(
+ rule: &FFI_PhysicalOptimizerRule,
+ plan: &FFI_ExecutionPlan,
+ context: &FFI_PhysicalOptimizerContext,
+) -> FFI_Result<FFI_ExecutionPlan> {
+ let runtime = rule.runtime();
+ let inner = rule.inner();
+ let plan: Arc<dyn ExecutionPlan> = sresult_return!(plan.try_into());
+ let config = sresult_return!(ConfigOptions::try_from(unsafe {
+ (context.config_options)(context)
+ }));
+ let has_registry = unsafe { (context.has_statistics_registry)(context) };
+ let registry = if has_registry {
+ Some(
+ context
+ .inner()
Review Comment:
`optimize_with_context_fn_wrapper` calls `context.inner()` on the
rule-provider side, which dereferences `context.private_data`.
In the real FFI path, `FFI_PhysicalOptimizerContext` is created by the
caller/consumer side, so `private_data` is foreign Rust memory from this
crate's perspective. It should only be accessed through the stable function
pointers on the FFI struct.
As written, the `StatisticsRegistry` path looks ABI-unsafe. Could we either
plumb the needed registry behavior through an FFI-safe callback/wrapper, or
avoid advertising/passing the registry across the boundary until it has a
stable ABI surface?
##########
datafusion/ffi/src/physical_optimizer.rs:
##########
@@ -31,6 +32,99 @@ use crate::execution_plan::FFI_ExecutionPlan;
use crate::util::FFI_Result;
use crate::{df_result, sresult_return};
+/// A stable struct for sharing [`PhysicalOptimizerContext`] across FFI
boundaries.
+///
+/// This provides access to configuration options and an optional statistics
registry
+/// for optimizer rules that need extended context.
Review Comment:
This adds a new function pointer that passes non-trivial FFI types:
`FFI_ExecutionPlan`, `FFI_PhysicalOptimizerContext`, and
`FFI_Result<FFI_ExecutionPlan>`.
Right now the cross-library integration test still only exercises
`optimize`, while `optimize_with_context` is covered only by in-process tests.
For FFI layout changes and new method plumbing like this, we really need the
`integration-tests` path to catch ABI, layout, and private-data issues.
Please add an integration constructor/rule that overrides
`optimize_with_context`, fails if plain `optimize` is called, and then calls
`foreign_rule.optimize_with_context(...)` from
`tests/ffi_physical_optimizer.rs`.
##########
datafusion/ffi/src/physical_optimizer.rs:
##########
@@ -41,6 +135,12 @@ pub struct FFI_PhysicalOptimizerRule {
config: FFI_ConfigOptions,
) -> FFI_Result<FFI_ExecutionPlan>,
+ pub optimize_with_context: unsafe extern "C" fn(
Review Comment:
Adding `optimize_with_context` changes the `#[repr(C)]` layout of
`FFI_PhysicalOptimizerRule`.
Since this is an ABI-breaking public API change and the PR currently only
has the `ffi` label, could you please add the required `api change` label
before merge?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
