JingsongLi commented on code in PR #9037:
URL: https://github.com/apache/paimon/pull/9037#discussion_r3725188061
##########
paimon-core/src/main/java/org/apache/paimon/utils/SnapshotManager.java:
##########
@@ -361,9 +361,13 @@ public boolean earliestFileNotExists() {
public @Nullable Snapshot earlierOrEqualWatermark(long watermark) {
Long latest = latestSnapshotId();
+ if (latest == null) {
+ return null;
+ }
// If latest == Long.MIN_VALUE don't need next binary search for
watermark
// which can reduce IO cost with snapshot
- if (latest == null || snapshot(latest).watermark() == Long.MIN_VALUE) {
+ Long latestWatermark = snapshot(latest).watermark();
+ if (latestWatermark != null && latestWatermark == Long.MIN_VALUE) {
Review Comment:
**[P1] Treat `Long.MIN_VALUE` as missing throughout the search**
`Long.MIN_VALUE` has the same "missing watermark" semantics as `null`, but
this null-safe guard only filters the sentinel when it happens to be on the
latest snapshot. With a mixed-engine history `[Long.MIN_VALUE, null]`, this now
falls through and `earlierOrEqualWatermark(0)` treats the sentinel as a real
value and returns the first snapshot. The Flink/Spark rollback procedures then
accept that snapshot and roll back, deleting the null-watermark tail; before
this change, the same layout failed with the NPE instead of performing an
incorrect rollback.
Please treat both `null` and `Long.MIN_VALUE` uniformly as missing in the
initial scan and the in-window fallback. This should not be implemented as an
early return whenever the latest value is missing, because `[100, null]` must
still find the earlier valid watermark. A regression test for `[Long.MIN_VALUE,
null]` would cover this case.
--
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]
