Core: Fix time-travel snapshot lookup to not assume snapshot-log orde… - #17360
Core: Fix time-travel snapshot lookup to not assume snapshot-log orde…#17360xndai wants to merge 1 commit into
Conversation
…ring (apache#17358) nullableSnapshotIdAsOfTime previously relied on snapshot-log entries being in chronological order, returning the last entry with timestamp <= requested. The spec does not guarantee ordering, and TableMetadata allows up to 1 minute of clock skew between entries. This could return the wrong snapshot when entries are out of order (e.g. due to WAP cherry-pick workflows). Fix by finding the entry with the maximum timestamp <= requested timestamp, which is correct regardless of iteration order. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
90c63d0 to
8e06915
Compare
| for (HistoryEntry logEntry : table.history()) { | ||
| if (logEntry.timestampMillis() <= timestampMillis) { | ||
| if (logEntry.timestampMillis() <= timestampMillis | ||
| && logEntry.timestampMillis() > bestTimestamp) { |
There was a problem hiding this comment.
Could there be snapshot with equal timestamps? Maybe we should go with logEntry.timestampMillis() >= bestTimestamp
There was a problem hiding this comment.
it doesn't really matter. In this implementation the first snapshot appears in the chain would be picked. Since this is something undefined by the spec, I feel it's ok to have such behavior.
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
we are trying to fix this with having monotonic timestamps of snapshot in v4
https://github.com/apache/iceberg/pull/16294/changes
Can we trust these timestamps in the first place as there is no requirement of true time semantics in first place, so i wonder if its worth fixing the old behaviour vs just saying hey v4 is when its gonna get fixed ?
Thanks for letting me know that. I'd say it still worth fixing as we still have v1, v2 and v3 tables out there. Once v4 becomes official, we could add a short circuit break in this loop for v4 as we can be sure that they are strictly ordered. |
| for (HistoryEntry logEntry : table.history()) { | ||
| if (logEntry.timestampMillis() <= timestampMillis) { | ||
| if (logEntry.timestampMillis() <= timestampMillis | ||
| && logEntry.timestampMillis() > bestTimestamp) { |
4 participants
…ring (#17358)
nullableSnapshotIdAsOfTime previously relied on snapshot-log entries being in chronological order, returning the last entry with timestamp <= requested. The spec does not guarantee ordering, and TableMetadata allows up to 1 minute of clock skew between entries. This could return the wrong snapshot when entries are out of order.
Fix by finding the entry with the maximum timestamp <= requested timestamp, which is correct regardless of iteration order.