zhuqi-lucas commented on code in PR #23903:
URL: https://github.com/apache/datafusion/pull/23903#discussion_r3736272361
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -1402,11 +1437,12 @@ pub fn with_new_children_if_necessary(
}
// Layer 2: same child properties → reuse `PlanProperties` cache.
if has_same_children_properties(plan.as_ref(), &children)? {
- return plan.with_new_children_and_same_properties(children);
+ return plan
+ .replace_children(children,
ChildrenPropertiesHint::SameProperties);
}
}
// Layer 3: full recompute.
- plan.with_new_children(children)
+ plan.replace_children(children, ChildrenPropertiesHint::Recompute)
Review Comment:
Naming-consistency nit (non-blocking): the PR moves everyone's mental model
to `replace_children`, but the one entry point callers should actually use is
still named `with_new_children_if_necessary` — embedding the now-deprecated
`with_new_children`. That reads as "do I use `with_` or `replace_`?".
Since this is a widely-used `pub fn` (~65 in-tree call sites + downstream),
an outright rename would be another breaking change. A pure-additive move
avoids that:
```rust
pub fn replace_children_if_necessary(
plan: Arc<dyn ExecutionPlan>,
children: Vec<Arc<dyn ExecutionPlan>>,
) -> Result<Arc<dyn ExecutionPlan>> { /* current body */ }
#[deprecated(since = "55.0.0", note = "Use `replace_children_if_necessary`")]
pub fn with_new_children_if_necessary(
plan: Arc<dyn ExecutionPlan>,
children: Vec<Arc<dyn ExecutionPlan>>,
) -> Result<Arc<dyn ExecutionPlan>> {
replace_children_if_necessary(plan, children)
}
```
One caveat if you do this: the in-tree call sites must move to the new name
in the same PR, otherwise `-D warnings` fails CI on the self-deprecation.
Downstream keeps compiling on the old name. Could also be a follow-up.
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1335,13 +1335,21 @@ impl ExecutionPlan for HashJoinExec {
/// This method is called during query optimization when the optimizer
creates new
/// plan nodes. Importantly, it creates a fresh bounds_accumulator via
`try_new`
/// rather than cloning the existing one because partitioning may have
changed.
- fn with_new_children(
+ fn replace_children(
self: Arc<Self>,
children: Vec<Arc<dyn ExecutionPlan>>,
+ _: ChildrenPropertiesHint,
) -> Result<Arc<dyn ExecutionPlan>> {
self.builder().with_new_children(children)?.build_exec()
Review Comment:
`HashJoinExec::replace_children` ignores `hint` and always rebuilds via the
builder, so a `SameProperties` signal does no fast-path here. Behaviorally
correct, but this is arguably the single most valuable place for the fast-path
— join equivalence-property computation is exactly the "expensive
`PlanProperties`" this PR set out to skip, yet it's rebuilt even when the
children are proven property-identical.
Is the always-rebuild intentional (builder / dynamic-filter lifecycle), or
an oversight? If it's safe to reuse, a `SameProperties` arm that swaps the two
children into a cloned `self` (like the other joins) would capture the win.
--
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]
