docs: clarify error semantics of in-place plan rewriting

Address review feedback on the in-place optimizer rewrite:

- Document the error contract of `rewrite_plan_in_place`: on `Err` the
  plan is left in an unspecified state because `rule.rewrite()` consumes
  it by value, and explain why it cannot be recovered without the clone
  the function exists to avoid. Note how `Optimizer::optimize` handles it.
- Move the `mem::take` bridge explanation from the doc comment into an
  inline comment next to the code it describes.
- Drop the inaccurate "Arc refcount is 1 is always true" claim.
- Document that `LogicalPlan::map_children_mut` does not roll back
  partial mutations when `f` fails.

Comment-only changes; no behavior change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: adriangb

datafusion/expr/src/logical_plan/tree_node.rs

@@ -402,6 +402,14 @@ impl LogicalPlan {
     /// When the refcount is >1, it clones the inner value first.
     ///
     /// Returns `Ok(true)` if any child was modified by `f`.
+    ///
+    /// # Error semantics
+    ///
+    /// If `f` returns `Err` for a child, this method returns that error
+    /// immediately. Children visited earlier in the same call keep whatever
+    /// modifications `f` already applied to them — they are **not** rolled
+    /// back. Callers that need the pre-call tree on error must save a copy
+    /// beforehand.
     pub fn map_children_mut<F: FnMut(&mut Self) -> Result<bool>>(
         &mut self,
         mut f: F,

datafusion/optimizer/src/optimizer.rs

@@ -365,13 +365,22 @@ impl TreeNodeRewriter for Rewriter<'_> {
 ///
 /// This avoids the `Arc::unwrap_or_clone` + `Arc::new` cycle that the
 /// ownership-based `TreeNode::rewrite` performs at every child node.
-/// When the `Arc` refcount is 1 (always true in the optimizer),
-/// `Arc::make_mut` is essentially free.
 ///
-/// The `rule.rewrite()` API takes ownership, so we bridge the `&mut` to an
-/// owned value with [`std::mem::take`]. `LogicalPlan::default()` is a cheap
-/// empty placeholder (shared empty schema, no allocation) and is overwritten
-/// with the rule's output on the very next line.
+/// # Error semantics
+///
+/// On `Err`, `*plan` is left in an **unspecified** state and must not be used.
+/// Because `rule.rewrite()` consumes the plan by value, a failing rule drops
+/// the node it was handed and the [`std::mem::take`] placeholder
+/// (`LogicalPlan::default()`, an `EmptyRelation`) is left in its place — at the
+/// root for a top-level failure, or somewhere in a subtree for a failure deeper
+/// in the recursion. The pre-rule plan is **not** recoverable here: restoring it
+/// would require cloning it before every rule invocation, which is exactly the
+/// allocation this function exists to avoid.
+///
+/// Callers must therefore discard `*plan` on `Err`, or restore it from a copy
+/// saved beforehand. [`Optimizer::optimize`] does this: with `skip_failed_rules`
+/// it restores `new_plan` from the `prev_plan` clone it already holds, and
+/// without it the error aborts the pass and the plan is dropped unobserved.
 #[cfg_attr(feature = "recursive_protection", recursive::recursive)]
 fn rewrite_plan_in_place(
     plan: &mut LogicalPlan,
@@ -382,6 +391,10 @@ fn rewrite_plan_in_place(
     // f_down phase
     let mut changed = false;
     if apply_order == ApplyOrder::TopDown {
+        // `rule.rewrite()` takes the plan by value, so bridge the `&mut` to an
+        // owned value with `std::mem::take`. `LogicalPlan::default()` is a cheap
+        // empty placeholder (shared empty schema, no allocation) and is
+        // overwritten with the rule's output on the next line.
         let owned = std::mem::take(plan);
         let result = rule.rewrite(owned, config)?;
         *plan = result.data;
@@ -514,6 +527,12 @@ impl Optimizer {
                             // No subqueries: use in-place rewriting
                             // with Arc::make_mut for zero-cost CoW on
                             // children, avoiding Arc unwrap/rewrap.
+                            //
+                            // On error `new_plan` is left in an unspecified
+                            // state (see `rewrite_plan_in_place`); the result
+                            // handling below discards it, restoring `prev_plan`
+                            // when `skip_failed_rules` is set or propagating
+                            // the error otherwise.
                             rewrite_plan_in_place(
                                 &mut new_plan,
                                 apply_order,