zhang-arvin commented on PR #9363: URL: https://github.com/apache/paimon/pull/9363#issuecomment-5520676065
@JingsongLi Thanks for the detailed review! I've addressed the three correctness gaps you identified: 1. **Lineage validation before empty checker** — The rollback/ABA check (snapshot ID comparison + UUID validation) now runs before the `conflictChecker.isEmpty()` early return, so DV-only and index-only commits are also protected. 2. **Dead null check** — Replaced `snapshotManager.snapshot()` null check with try/catch for `RuntimeException`, since `SnapshotManager.snapshot()` throws when the file is missing, never returns null. 3. **Cache bypass** — Added `snapshotManager.invalidateCache()` before reading the base snapshot to avoid stale cache entries after rollback and snapshot ID reuse. All 52 `ConflictDetectionTest` tests pass. CI is green on the previous commit. PTAL. Re: the API design suggestion to pass `Snapshot` instead of `(snapshotId, snapshotUuid)` — I agree this is cleaner, but implementing it requires touching `ConflictDetection` (base class), `FileStoreCommit`, `FileStoreCommitImpl`, `InnerTableCommit`, `TableCommitImpl`, `BatchWriteBuilderImpl`, `DataEvolutionMergeIntoAction`, `DataEvolutionDeleteSink`, Spark `PaimonSparkWriter`, and both Spark `MergeIntoPaimonDataEvolutionTable` files. I'd prefer to land this fix first and follow up with the API refactor in a separate PR to keep the scope manageable. -- 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]
