zeroshade commented on code in PR #2150:
URL: https://github.com/apache/iceberg-go/pull/2150#discussion_r4258068072
##########
table/snapshots.go:
##########
@@ -548,11 +548,13 @@ type SnapshotLogEntry struct {
TimestampMs int64 `json:"timestamp-ms"`
}
-// snapshotLogEntryAsOf returns the log entry with the greatest eligible
-// timestamp. Snapshot-log entries can be out of chronological order when
-// commits have small clock skews, so the result must not depend on iteration
-// order.
-func snapshotLogEntryAsOf(entries iter.Seq[SnapshotLogEntry], timestampMs
int64, inclusive bool) (SnapshotLogEntry, bool) {
+// SnapshotLogEntryAsOf returns the snapshot-log entry with the greatest
+// timestamp at or before timestampMs (or strictly before it when inclusive is
+// false), and whether one was found. Snapshot-log entries can be out of
+// chronological order when commits have small clock skews, so the result must
+// not depend on iteration order. It is the time-travel resolution used by the
Review Comment:
Nit: "the result must not depend on iteration order" holds only for distinct
timestamps. On a tie the strict `>` keeps the first eligible entry in log
order. Now that this is public API, I'd state the tie-break explicitly ("on
equal timestamps, the earliest entry in log order wins") or switch to `>=` to
take the last, matching Java's `SnapshotUtil` walk, and add a tie case to the
existing tests either way.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
