askalt commented on code in PR #19792:
URL: https://github.com/apache/datafusion/pull/19792#discussion_r2697895540
##########
datafusion/ffi/src/execution_plan.rs:
##########
@@ -192,7 +192,7 @@ impl Drop for FFI_ExecutionPlan {
pub struct ForeignExecutionPlan {
name: String,
plan: FFI_ExecutionPlan,
- properties: PlanProperties,
+ properties: Arc<PlanProperties>,
Review Comment:
As `ExecutionPlan` is implemented for `ForeignExecutionPlan` then we must
return `&Arc<PlanProperties>` from `properties(...)`. So Arc should be owned by
someone to return its borrowing. We can change a signature of `properties(...)`
to return owned `Arc` but cloning seems redundant for me, as the most plans
store own `Arc`.
Note that `FFI_ExecutionPlan` is not touched by the patch. Do you sure that
it is important to keep `ForeignExecutionPlan` stable?
```rust
/// This struct is used to access an execution plan provided by a foreign
/// library across a FFI boundary.
///
/// The ForeignExecutionPlan is to be used by the caller of the plan, so it
has
/// no knowledge or access to the private data. All interaction with the plan
/// must occur through the functions defined in FFI_ExecutionPlan.
#[derive(Debug)]
pub struct ForeignExecutionPlan {
name: String,
plan: FFI_ExecutionPlan,
properties: Arc<PlanProperties>,
children: Vec<Arc<dyn ExecutionPlan>>,
}
```
--
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]
