EnsureRequirements returns Transformed::yes unconditionally (withdrawn: identity is already preserved)

[zhuqi-lucas]

Withdrawn. The premise below is wrong.

replace_children_if_necessary already returns the original plan when the child pointers are unchanged, so an unconditional Transformed::yes does not force a rebuild and does not cost the caller pointer identity. A pass that genuinely does nothing already hands back its input.

I checked this by making the three sites report accurately and comparing input against output across eleven plan shapes, with and without that change. The results were byte-identical, so the change is a no-op.

One claim below is worse than wrong, it was never measured: "Before the fix none of them did". My probe and the three-site change went in as one patch and I never ran the baseline. The 7 of 23 calls that returned their input already did so.

What survives is #25360, and this makes it cleaner: since accurate reporting demonstrably changes nothing, the 16 calls that stayed byte-identical while losing pointer identity can only have done so by genuinely rewriting the tree and rewriting it back.

The real residue is minor. Reporting yes when nothing changed still makes transform_up call with_new_arc_children per ancestor, costing a children() call and a pointer scan rather than being skipped outright. Not worth an issue on its own.

Original text below, left for the record.

---

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.

// 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:

// 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:

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, observing 31 EnsureRequirements invocations across the process. Of the 23 that left the plan byte-identical, fixing these three sites makes 7 also return the input object, which callers can then detect for free. Before the fix none of them did.

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.

[zhuqi-lucas]

It's a false issue since we have replace skip child.