DanielLeens commented on PR #10941:
URL: https://github.com/apache/seatunnel/pull/10941#issuecomment-4528174638
Thanks for the fix. I pulled the latest head locally and traced the real
runtime path.
# What This PR Fixes
- User pain: when the same upstream `Record<SeaTunnelRow>` is fanned out
into sibling transform branches, `ReplaceTransform` mutates the shared row in
place, so one branch can unexpectedly change what the other branch sees.
- Fix approach: `ReplaceTransform.transformRow(...)` now works on
`inputRow.copy()` and returns the copied row instead of mutating the original
shared row.
- One-line summary: this is a precise fix for a real fan-out data-sharing
bug, and it lands at the correct layer.
Simple example:
- Before this change, branch A could replace `title` first, and branch B
would then read the already modified value from the same input row.
- After this change, each `ReplaceTransform` output is derived from its own
copied row, so sibling branches still see the original upstream row.
# Runtime Chain Rechecked
```text
Upstream row fan-out
-> SeaTunnelTransformCollector.collect(record)
[seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/task/SeaTunnelTransformCollector.java:35-41]
-> forwards the same Record instance to every downstream output
Transform execution
-> TransformFlowLifeCycle.received(record)
[seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/task/flow/TransformFlowLifeCycle.java:117-126]
-> passes record.getData() into transform(...)
-> TransformFlowLifeCycle.transform(inputData)
[TransformFlowLifeCycle.java:138-162]
-> each map transform receives the same SeaTunnelRow reference for
that branch input
Replace transform
-> ReplaceTransform.transformRow(inputRow)
[seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/replace/ReplaceTransform.java:125-136]
-> old behavior: mutate inputRow directly
-> new behavior: copy first, mutate the copy, return the copy
```
# Key Findings
- The bug is real because the collector fans out the same `Record` object to
multiple downstream outputs.
- The normal path does hit this change whenever `ReplaceTransform` is used
in a shared-row fan-out topology.
- I do not think this should be pushed down into `TransformFlowLifeCycle` as
a blanket copy for all transforms. That would add unconditional per-row copy
overhead even for transforms that are read-only or already allocate fresh
output rows. The current fix is more targeted and matches the pattern already
used by mutating transforms like `FieldEncryptTransform` and
`TableRenameTransform`.
- The new test in `ReplaceTransformTest.java:272-304` covers the real
regression shape: two sibling transform branches sharing one input row.
# Findings
No blocking issue found in the current head.
# Merge Decision
### Conclusion: can merge
1. Blocking items
- None.
2. Suggested follow-up
- The `Build` check was still running when I reviewed. Please just let that
finish green.
Overall, this looks like a clean and correctly scoped fix. Nice catch on the
shared-row mutation behavior.
--
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]
