zhuqi-lucas opened a new issue, #25361:
URL: https://github.com/apache/datafusion/issues/25361
### Describe the bug
`EnsureRequirements` returns `Transformed::yes` unconditionally at three
places, so `transform_up` rebuilds every ancestor node and recomputes its
`PlanProperties` even on a pass that changes nothing, and the rule always hands
back a fresh `Arc`.
```rust
// physical-optimizer/src/ensure_requirements/mod.rs
plan.transform_up(|p| Ok(Transformed::yes(reorder_join_keys_to_inputs(p)?)))
.transform_up(|p| Ok(Transformed::yes(replace_with_partial_sort(p)?)))
// physical-optimizer/src/ensure_requirements/enforce_distribution.rs, end
of ensure_distribution
Ok(Transformed::yes(optimized_context))
```
The flag is what decides whether a node is rebuilt:
```rust
// common/src/tree_node.rs
if new_children.transformed {
self.with_new_arc_children(arc_self, new_children) // rebuild,
recompute properties
} else {
Ok(Transformed::new(self, false, new_children.tnr)) // reuse the
original Arc
}
```
Reporting `yes` when nothing changed therefore costs a full rebuild of the
plan, and removes the caller's ability to tell that nothing happened.
### To Reproduce
Run `EnsureRequirements` on a plan it has already settled and compare the
result with `Arc::ptr_eq`. It is never equal.
### Expected behavior
Report what actually happened. A pointer comparison against the input is
enough at each of the three sites:
```rust
let before = Arc::clone(&p);
let after = reorder_join_keys_to_inputs(p)?;
Ok(if Arc::ptr_eq(&before, &after) { Transformed::no(after) } else {
Transformed::yes(after) })
```
### Additional context
Measured on a real 34-node plan through a chain with six enforcement passes.
Of 23 calls that left the plan byte-identical, fixing these three sites makes 7
also return the input object, which callers can then detect for free.
The remaining 16 still rebuild, because the distribution and sorting phases
inside the rule change the plan and then change it back. That is a separate
problem, filed as #25360.
Found while measuring #25355 / #25356, where this is why plans have to be
compared by rendered form rather than by pointer.
--
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]
