alamb commented on code in PR #19792:
URL: https://github.com/apache/datafusion/pull/19792#discussion_r2695682719
##########
datafusion/common/src/utils/mod.rs:
##########
@@ -70,7 +70,7 @@ use std::thread::available_parallelism;
/// ```
pub fn project_schema(
schema: &SchemaRef,
- projection: Option<&Vec<usize>>,
+ projection: Option<&[usize]>,
Review Comment:
this is also technically a public API change, but I think it just relaxes
the constraints (you can still pass in a `&Vec` so all downstream code should
continue to work
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -1236,14 +1249,16 @@ impl ExecutionPlan for AggregateExec {
fn with_new_children(
self: Arc<Self>,
- children: Vec<Arc<dyn ExecutionPlan>>,
+ mut children: Vec<Arc<dyn ExecutionPlan>>,
) -> Result<Arc<dyn ExecutionPlan>> {
+ check_if_same_properties!(self, children);
+
let mut me = AggregateExec::try_new_with_schema(
self.mode,
self.group_by.clone(),
- self.aggr_expr.clone(),
- self.filter_expr.clone(),
- Arc::clone(&children[0]),
+ self.aggr_expr.to_vec(),
Review Comment:
we might be able to avoid these clones too by changing the signature of
AggregateExec 🤔
##########
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:
since the FFI is supposed to be a stable boundary, I think we shouldn't
change this (and instead just copy the properties when needed)
So I suggest reverting this change
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -128,7 +128,7 @@ pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
///
/// This information is available via methods on
[`ExecutionPlanProperties`]
/// trait, which is implemented for all `ExecutionPlan`s.
- fn properties(&self) -> &PlanProperties;
+ fn properties(&self) -> &Arc<PlanProperties>;
Review Comment:
This is technically a breaking API change, so it should be documented in
the upgrading.md guide:
https://github.com/apache/datafusion/blob/main/docs/source/library-user-guide/upgrading.md
Per the policy in
https://datafusion.apache.org/contributor-guide/api-health.html
I am happy to help write such an entry
--
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]
