DanielLeens commented on PR #12881:
URL: https://github.com/apache/seatunnel/pull/12881#issuecomment-6337870516
Sorry for missing this in my previous review. This is a carry-over omission
in my earlier source-clear conclusion, not an issue introduced by a new commit:
the head is still `427c77210473ed8b3665559487949eb8816ed1c9`. +1 to
@zhengxiang378928908-code's concrete schema-change/upsert counterexample, and
thank you for distinguishing the connector-level reproduction from an engine
E2E.
I fetched that head and traced the relevant path:
```text
IcebergSinkWriter.applySchemaChange (IcebergSinkWriter.java:121-133)
-> IcebergRecordWriter.applySchemaUpdate (:155-161)
flush() completes and saves the old task-writer result
resetWriter() creates a new task writer
-> new DeletionVectorTaskWriter.PartitionWriter has a fresh rows map
(:279-283)
UPDATE_AFTER -> delete(key) -> no previous position -> equality
delete (:306-321)
insert(updated row)
prepareCommit -> IcebergRecordWriter.complete (:165-169)
-> both writer results
IcebergAggregatedCommitter.commitFiles / flattenResults (:92-103)
-> IcebergFilesCommitter.commit (:342-353)
one RowDelta adds both results' data and delete files, then commits
once
```
That supports the reported stale-row outcome: the new writer no longer knows
the pre-schema-change row's file/position, and an equality delete does not
remove a data file with the same data sequence number. The [Iceberg
scan-planning
contract](https://iceberg.apache.org/spec/index.html#scan-planning) requires an
older data sequence for an equality delete to apply. Deletion vectors would
handle same-commit positions, but the earlier writer's positions are not
carried across this reset. The v2 case is pre-existing; enabling the analogous
v3 path makes it relevant to this PR as well.
**I withdraw the earlier CI-only/source-clear conclusion. This is a P0
data-correctness blocker for the schema-change + upsert path when those results
reach one commit.** I did not run your reproduction locally, and I am not
claiming every Zeta schema-change sequence reaches the window. The connector
contract is affected, and the Flink sink schema-event handler also calls
`applySchemaChange` directly without an intervening `prepareCommit` in that
handler; an engine regression should pin the intended supported path rather
than relying on an implicit barrier assumption.
Yes, preserving ordered groups and placing each group's result fingerprints
on that group's snapshot is the right direction. Both ordinary commit and
restore must share the publication check; a reused checkpoint number must not
identify a different payload as already published. Please retain the exact
final-row assertion for v2/v3 across a schema change, and cover a crash after
only the first group is published, replay through ordinary commit, and
checkpoint-ID reuse. A combined tree has not been validated by this reply.
For landing order, please coordinate the two PRs so the ordering fix is
present before the newly supported v3 schema-change/upsert combination is
mergeable. Merging this PR first while leaving that correctness gap for a later
PR would not be safe. A prerequisite/common correctness change, or explicitly
keeping the unsupported combination gated until it lands, is preferable. This
reply answers the existing counterexample and corrects my prior conclusion; it
is not a claim that a fresh full review of all 21 files or a combined v3 E2E
has been completed.
--
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]
