Copilot commented on code in PR #20449:
URL: https://github.com/apache/datafusion/pull/20449#discussion_r2847872006
##########
datafusion/ffi/src/proto/physical_extension_codec.rs:
##########
@@ -27,6 +24,9 @@ use datafusion_expr::{
};
use datafusion_physical_plan::ExecutionPlan;
use datafusion_proto::physical_plan::PhysicalExtensionCodec;
+use std::any::Any;
+use std::ffi::c_void;
+use std::sync::Arc;
Review Comment:
The imports are now split with `use std::any::Any;` on line 27 and other
`std` imports on lines 28-29, creating an inconsistent import organization.
Group all `std` imports together for better readability.
```suggestion
use std::{any::Any, ffi::c_void, sync::Arc};
```
##########
datafusion/ffi/src/execution_plan.rs:
##########
@@ -96,37 +112,69 @@ unsafe extern "C" fn properties_fn_wrapper(
unsafe extern "C" fn children_fn_wrapper(
plan: &FFI_ExecutionPlan,
) -> RVec<FFI_ExecutionPlan> {
- unsafe {
- let private_data = plan.private_data as *const
ExecutionPlanPrivateData;
- let plan = &(*private_data).plan;
- let runtime = &(*private_data).runtime;
+ let runtime = plan.runtime();
+ let plan = plan.inner();
- let children: Vec<_> = plan
- .children()
- .into_iter()
- .map(|child| FFI_ExecutionPlan::new(Arc::clone(child),
runtime.clone()))
- .collect();
+ let children: Vec<_> = plan
+ .children()
+ .into_iter()
+ .map(|child| FFI_ExecutionPlan::new(Arc::clone(child),
runtime.clone()))
+ .collect();
- children.into()
- }
+ children.into()
+}
+
+unsafe extern "C" fn with_new_children_fn_wrapper(
+ plan: &FFI_ExecutionPlan,
+ children: RVec<FFI_ExecutionPlan>,
+) -> FFIResult<FFI_ExecutionPlan> {
+ let runtime = plan.runtime();
+ let plan = Arc::clone(plan.inner());
+ let children = rresult_return!(
+ children
+ .iter()
+ .map(<Arc<dyn ExecutionPlan>>::try_from)
+ .collect::<Result<Vec<_>>>()
+ );
+
+ let new_plan = rresult_return!(plan.with_new_children(children));
+
+ RResult::ROk(FFI_ExecutionPlan::new(new_plan, runtime))
}
unsafe extern "C" fn execute_fn_wrapper(
plan: &FFI_ExecutionPlan,
partition: usize,
context: FFI_TaskContext,
) -> FFIResult<FFI_RecordBatchStream> {
- unsafe {
- let ctx = context.into();
- let private_data = plan.private_data as *const
ExecutionPlanPrivateData;
- let plan = &(*private_data).plan;
- let runtime = (*private_data).runtime.clone();
-
- rresult!(
- plan.execute(partition, ctx)
- .map(|rbs| FFI_RecordBatchStream::new(rbs, runtime))
- )
- }
+ let ctx = context.into();
+ let runtime = plan.runtime();
+ let plan = plan.inner();
+
+ let _guard = runtime.as_ref().map(|rt| rt.enter());
Review Comment:
The variable `_guard` is prefixed with an underscore but is actually used to
maintain the runtime guard's lifetime. Consider renaming it to `guard` or
`_runtime_guard` to better reflect its purpose.
```suggestion
let _runtime_guard = runtime.as_ref().map(|rt| rt.enter());
```
--
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]
