JingsongLi commented on PR #9363: URL: https://github.com/apache/paimon/pull/9363#issuecomment-5474195537
@zhang-arvin Thanks. I do not think this is merely an API improvement that should be deferred. The current PR already propagates two new values through the same callers, so replacing them with one `Snapshot` does not materially increase the scope. The nullable `Snapshot` field can remain in `DataEvolutionConflictDetection`; the base `ConflictDetection` only needs the polymorphic setter, just as it does today. More importantly, the current implementation still has correctness gaps: - Lineage validation runs after the empty `RowIdConflictChecker` return, so DV-only and index-only commits skip it. - `snapshotManager.snapshot(baseId)` is cache-aware, so a cached snapshot from the old lineage can make the UUID comparison pass after rollback and ID reuse. - `SnapshotManager.snapshot()` does not return `null`; it throws when the snapshot is missing, so the current null branch does not provide the claimed handling. Therefore, the current implementation does not fully cover rollback/ABA. Please address these points in this PR and add an end-to-end rollback test rather than deferring them to a follow-up. -- 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]
