JingsongLi commented on code in PR #9363:
URL: https://github.com/apache/paimon/pull/9363#discussion_r3932116453
##########
paimon-core/src/main/java/org/apache/paimon/operation/commit/DataEvolutionConflictDetection.java:
##########
@@ -350,20 +363,58 @@ private Optional<RuntimeException>
checkForRowIdFromSnapshot(
List<SimpleFileEntry> deltaEntries,
List<IndexManifestEntry> deltaIndexEntries,
@Nullable RowIdConflictChecker conflictChecker) {
- if (rowIdCheckFromSnapshot == null
- || conflictChecker == null
- || conflictChecker.isEmpty()) {
+ if (rowIdCheckFromSnapshot == null) {
+ return Optional.empty();
+ }
+
+ // Run lineage validation BEFORE empty checker check so that DV-only
and
+ // index-only commits are also protected against rollback/ABA.
+ // Fail closed when the latest snapshot ID is less than the base
snapshot ID.
+ // This indicates a rollback has deleted newer snapshots, and the
staged update
+ // is based on a snapshot lineage that no longer exists.
+ if (latestSnapshot.id() < rowIdCheckFromSnapshot) {
+ return Optional.of(
+ new RuntimeException(
+
ErrorMessages.DATA_EVOLUTION_SNAPSHOT_LINEAGE_CONFLICT_MESSAGE));
+ }
+
+ // Detect equal snapshot IDs with different snapshot UUIDs (ABA
problem).
+ // A rollback can delete a snapshot and a new commit can reuse the
same numeric ID.
+ // If the base snapshot UUID differs from the current snapshot UUID at
that ID,
+ // the staged update is based on a different snapshot lineage.
+ // Invalidate cache before reading to avoid stale entries after
rollback.
+ Snapshot baseSnapshot;
+ try {
+ snapshotManager.invalidateCache();
+ baseSnapshot = snapshotManager.snapshot(rowIdCheckFromSnapshot);
+ } catch (RuntimeException e) {
+ // snapshotManager.snapshot() throws RuntimeException when file is
missing
+ // (e.g., snapshot was deleted by rollback or expiration).
+ return Optional.of(
+ new RuntimeException(
+
ErrorMessages.DATA_EVOLUTION_SNAPSHOT_LINEAGE_CONFLICT_MESSAGE));
+ }
+ if (baseSnapshotUuid != null &&
!baseSnapshotUuid.equals(baseSnapshot.uuid())) {
Review Comment:
[P1] Preserve ABA protection for legacy snapshots. `Snapshot.uuid()` is
explicitly nullable for snapshots created before UUID support, and every
updated caller passes that null through. This guard then skips identity
validation entirely, so a data-evolution MERGE/DELETE staged from a legacy base
can still be committed after rollback deletes that base and a different
snapshot reuses the same ID—the original corruption scenario. Please carry the
captured `Snapshot` (or another full stable identity) and compare full snapshot
equality when the UUID is null; an end-to-end rollback/ID-reuse test starting
from a null-UUID snapshot would exercise this path.
--
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]
