peter-toth opened a new pull request, #55482:
URL: https://github.com/apache/spark/pull/55482

   ### What changes were proposed in this pull request?
   
   Remove `cachedPlanMapping` / `cpMapping` from `PlanMerger` by preserving 
ExprIds when wrapping cached-plan expressions in `mergeNamedExpressions`.
   
   Previously, wrapping a cached expression in `Alias(If(filter, expr, null))` 
generated a fresh `ExprId`, making parent nodes that referenced the original 
attribute by ExprId stale. A `cachedPlanMapping` field was threaded through 
`TryMergeResult` and `mergeNamedExpressions` to remap those references at every 
level of `tryMergePlans`.
   
   This PR eliminates the need for that mapping by passing `exprId = 
ce.toAttribute.exprId` to the wrapping `Alias`, preserving the original 
`ExprId`. Since the mapping is now always identity, 
`TryMergeResult.cachedPlanMapping` and the `cachedPlanMapping` parameter of 
`mergeNamedExpressions` are removed entirely, along with all `cpMapping` 
threading throughout `tryMergePlans`.
   
   Additional cleanups:
   - `mappedCPCondition` locals replaced with direct `cp.condition` references 
(no remapping needed after cpMapping removal).
   - `mappedCPGroupingExpression` assigned directly from 
`cp.groupingExpressions`.
   - Cached-expression wrapping loop limited to `cachedPlanExpressions.size` 
entries to avoid accidentally wrapping newly-appended new-plan expressions.
   - Cp-expression wrapping simplified to a single `case Alias(child, _) if 
!child.isInstanceOf[Attribute]` match.
   
   ### Why are the changes needed?
   
   `cachedPlanMapping` was purely mechanical bookkeeping to compensate for 
`Alias(...)()` generating fresh ExprIds. Preserving the original ExprId makes 
the mapping a no-op everywhere, so it can be removed. This simplifies 
`TryMergeResult` from 5 fields to 4, changes `mergeNamedExpressions` to return 
a pair instead of a triple, and removes cpMapping threading from all branches 
of `tryMergePlans`.
   
   ### Does this PR introduce _any_ user-facing change?
   
   No.
   
   ### How was this patch tested?
   
   Existing `MergeSubplansSuite` unit tests.
   
   ### Was this patch authored or co-authored using generative AI tooling?
   
   Generated-by: Claude Sonnet 4.6
   


-- 
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]

Reply via email to