DanielLeens opened a new pull request, #12203:
URL: https://github.com/apache/seatunnel/pull/12203
### Purpose
Adds regression test coverage for the `IMapFileData`/`IMapData` `Comparable`
contract
violation reported in #11462 and fixed by #11465. No production code is
touched by
this PR; it is test-only.
### The bug and its fix
Before #11465, `IMapFileData.compareTo` was:
```java
@Override
public int compareTo(IMapFileData o) {
return o.timestamp - this.timestamp > 0 ? 1 : -1;
}
```
That expression never returns `0`, so for two records sharing the exact same
`timestamp` (realistic under batch writes, since `IMapFileStorage` stamps
every
record with `System.currentTimeMillis()`), both `compare(a, b)` and
`compare(b, a)`
evaluated to `-1`, violating `Comparable`'s antisymmetry requirement
`sgn(compare(x, y)) == -sgn(compare(y, x))`. Two consequences followed:
1. `WALReader.loadAllData`/`loadAllKeys` call `Collections.sort` on every
full WAL
read. Once enough same-timestamp records are present to push the sort past
TimSort's binary-insertion fast path (its `MIN_MERGE` threshold of 32
elements),
the merge-invariant check throws `IllegalArgumentException: Comparison
method
violates its general contract!`, crashing IMap recovery.
2. Even without throwing, the unspecified tie order let `WALReader`'s replay
loop
(which treats the first record it encounters per key, after sorting, as
authoritative) silently drop a same-millisecond delete if a put for the
same key
happened to sort ahead of it -- the "lost delete" in the issue title.
On current `dev`, the fix (`IMapDataComparator#compare`, shared by
`IMapFileData`
and `IMapData`) is:
```java
int keyClassCompare = compareNullableString(leftKeyClassName,
rightKeyClassName);
if (keyClassCompare != 0) {
return keyClassCompare;
}
int keyCompare = compareBytes(leftKey, rightKey);
if (keyCompare != 0) {
return keyCompare;
}
int timestampCompare = Long.compare(rightTimestamp, leftTimestamp);
if (timestampCompare != 0) {
return timestampCompare;
}
// deterministic tie-breaker for genuine same-millisecond collisions:
int deleteCompare = Boolean.compare(rightDeleted, leftDeleted);
if (deleteCompare != 0) {
return deleteCompare;
}
int valueClassCompare = compareNullableString(leftValueClassName,
rightValueClassName);
if (valueClassCompare != 0) {
return valueClassCompare;
}
return compareBytes(leftValue, rightValue);
```
i.e. key class name, then key bytes, then timestamp descending, then (only
for a
genuine same-millisecond collision on the same key)
deleted-before-not-deleted,
then value class name, then value bytes -- a real total order that correctly
returns `0` for equal records.
### Existing coverage vs. what this PR adds
#11465 already added direct comparator-unit coverage
(`IMapFileDataCompareToTest`, including a 4000-random-element
`Collections.sort` crash check) and single-pair WAL replay coverage
(`WALReaderAndWriterTest#testReplayKeepsTombstoneForSameKeyAndTimestamp`).
This PR
does not duplicate either; it adds a new class,
`IMapFileDataTimestampCollisionRegressionTest`, with two complementary
checks built
around one shared 85-record, all-same-timestamp fixture (40 independent
same-key
put/delete pairs plus 5 uncontested control puts -- comfortably over
TimSort's
32-element merge threshold):
- `testCollectionsSortOfLargeSameTimestampBatchSatisfiesComparableContract`
sorts
the batch directly and checks the contract holds across all 40 pairs
*simultaneously* in one real `Collections.sort` call (not just for one
isolated
pair): per-pair antisymmetry, delete-before-put tie-break, and a full
adjacent-element non-decreasing scan of the sorted output (a
transitivity-implying
self-consistency check over all 85 records).
- `testWalReplayDoesNotLoseSameTimestampDeletesAcrossWriteOrders` writes the
same 40
pairs through the production `WALWriter`, deliberately alternating the
physical
append order per pair (put-before-delete for even-indexed keys,
delete-before-put for odd-indexed keys), then replays them through the
production
`WALReader#loadAllData` and `#loadAllKeys` -- the exact methods the issue
names as
the crash site -- and asserts every delete is genuinely observed (the key
is
absent from both results) regardless of which physical order it was
appended in,
while the 5 control keys keep their values. This is the concrete "lost
delete"
proof, exercised at a scale that would have been able to trip the old
crash too,
and it additionally proves the outcome is now independent of WAL append
order --
closing the specific gap the old, contract-violating comparator left open
(an antisymmetry-violating comparator gives `Collections.sort` license to
place
tied records in an unspecified order, so the old bug was not just "one
write order
loses the delete" but "the result was undefined").
### Verification
Per this task's constraints, no local Maven compile/test/verify was run. Only
`./mvnw spotless:apply -pl
seatunnel-engine/seatunnel-engine-storage/imap-storage-plugins/imap-storage-file
-nsu -Dmaven.gitcommitid.skip=true`
was run locally (module-scoped, no `-am`), and the diff is exactly one new
file
under `src/test/java` -- no `src/main` or `pom.xml` changes. Correctness was
established by carefully reading the current `IMapDataComparator`,
`IMapFileData`,
`WALWriter`, `WALReader`, and `IMapFileStorage` source and tracing the exact
sort/dedup logic by hand rather than by running the test locally. GitHub CI
on this
PR's head commit is the verification of record.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
