gabotechs commented on code in PR #20449:
URL: https://github.com/apache/datafusion/pull/20449#discussion_r2955684465
##########
datafusion/ffi/src/execution_plan.rs:
##########
@@ -275,12 +385,16 @@ impl ExecutionPlan for ForeignExecutionPlan {
self: Arc<Self>,
children: Vec<Arc<dyn ExecutionPlan>>,
) -> Result<Arc<dyn ExecutionPlan>> {
- Ok(Arc::new(ForeignExecutionPlan {
- plan: self.plan.clone(),
- name: self.name.clone(),
- children,
- properties: Arc::clone(&self.properties),
- }))
+ unsafe {
+ let children = children
+ .into_iter()
+ .map(|child| FFI_ExecutionPlan::new(child, None))
+ .collect::<RVec<_>>();
+ let new_plan =
+ df_result!((self.plan.with_new_children)(&self.plan,
children))?;
+
Review Comment:
Probably the unsafe block can be scoped to this:
```rust
let new_plan =
unsafe { df_result!((self.plan.with_new_children)(&self.plan,
children))? };
```
##########
datafusion/expr-common/src/accumulator.rs:
##########
@@ -48,7 +48,7 @@ use std::fmt::Debug;
/// [`evaluate`]: Self::evaluate
/// [`merge_batch`]: Self::merge_batch
/// [window function]: https://en.wikipedia.org/wiki/Window_function_(SQL)
-pub trait Accumulator: Send + Sync + Debug {
+pub trait Accumulator: Send + Sync + Debug + std::any::Any {
Review Comment:
Other traits of the project, rather than requiring `Any` in the trait, they
have instead an additional `fn as_any(&self) -> &dyn Any` method for upcasting
concrete implementations to `Any`.
Do you think `as_any()` could have been used here instead? I don't see a
strong reason in favor of it, but maybe you actually have a strong reason
against it.
##########
datafusion/functions-aggregate/src/average.rs:
##########
@@ -754,7 +754,7 @@ impl Accumulator for DurationAvgAccumulator {
struct AvgGroupsAccumulator<T, F>
where
T: ArrowNumericType + Send,
- F: Fn(T::Native, u64) -> Result<T::Native> + Send,
+ F: Fn(T::Native, u64) -> Result<T::Native> + Send + 'static,
Review Comment:
:thinking: requiring 'static seems like a fair ask. If this anyways required
`Send` it's likely that pretty much everything was 'static already.
I imagine this requirement comes from the fact that now the
`GroupsAccumulator` implementation requires `Any`, and `Any` is not
automatically satisfied it the type has non 'static references. Is that the
case?
--
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]
