HyukjinKwon commented on PR #58800:
URL: https://github.com/apache/spark/pull/58800#issuecomment-5672778572
Reviewed as part of a batch pass (read-only). LGTM — careful, well-tested
optimization.
Walked through the semantics against the originals:
- `mapChildren`: the single-pass lazy buffer reproduces
`withNewChildren(children.map(f))`
exactly. It returns `this` when no child changes *materially* (matching
`childrenFastEquals`, and preserving the existing behavior of discarding
equal-but-distinct copies when nothing else changed); when a material
change occurs the
buffer holds precisely what `children.map(f)` would produce (originals for
`eq`
children, replacements otherwise), retaining equal-but-distinct
replacements at earlier
and non-adjacent indices. The backfill runs only once (at the first
distinct child), and
`children.isEmpty` is equivalent to the old `containsChild.nonEmpty` guard
since
`containsChild = children.toSet`. Calling `withNewChildrenInternal`
directly is safe
because the buffer size equals the child count by construction.
- `applyRule` is behaviorally identical to
`CurrentOrigin.withOrigin(origin){ ... }`
(save/set/try-finally-restore), and sharing `TreeNodeIdentity.fn` is safe
since identity
ignores its argument's type.
- The `transformUpWithPruning` reorder only short-circuits `mapChildren` for
leaves (where
it returned `this` anyway); pruning and ineffective-rule tracking are
unchanged.
Test coverage is thorough — the non-adjacent equal-copy replay case and the
origin
save/restore-on-throw case are exactly the ones worth pinning. No findings.
--
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]
