mbutrovich commented on code in PR #3260:
URL: https://github.com/apache/iceberg-rust/pull/3260#discussion_r4126150016
##########
crates/iceberg/src/util/snapshot.rs:
##########
@@ -76,10 +77,38 @@ pub fn ancestors_between(
})
}
+/// Resolve the snapshot ID from the latest main-history entry at or before
+/// `timestamp_ms` (milliseconds since the Unix epoch).
+///
Review Comment:
> ties here are first-wins (matching Java), but PyIceberg's
`snapshot_as_of_timestamp` iterates history reversed, so it's last-wins.
The divergence covers out-of-order entries too, not only ties. PyIceberg
returns the last [snapshot
log](https://github.com/apache/iceberg/blob/e689699fa6392a2da9aa10870f0d4de575ae23c1/format/spec.md?plain=1#L1162)
entry at or before the timestamp, in log order
([`snapshot_as_of_timestamp`](https://github.com/apache/iceberg-python/blob/ebbc0ba3e4c633179b4069a70a8f9a4453e75d88/pyiceberg/table/__init__.py#L1573-L1583)).
This PR and Java's
[`nullableSnapshotIdAsOfTime`](https://github.com/apache/iceberg/blob/e689699fa6392a2da9aa10870f0d4de575ae23c1/core/src/main/java/org/apache/iceberg/util/SnapshotUtil.java#L325-L337)
return the entry with the greatest timestamp. For the PR's own case at [line
151](https://github.com/apache/iceberg-rust/blob/ee2cb05c7bed8686e24674187f200a6c5906ebcb/crates/iceberg/src/util/snapshot.rs#L151),
history `[(1000, S1), (3000, S2), (2500, S3)]` queried at 3500, this PR
returns S2 and PyIceberg returns S3. Valid metadata can contain that order,
because `vali
date_chronological_snapshot_logs` accepts an entry up to one minute earlier
than the entry before it
([code](https://github.com/apache/iceberg-rust/blob/86d6618804a902eac10f09c09a565aca5dad0d46/crates/iceberg/src/spec/table_metadata.rs#L705-L712)).
@dhruvarya-db, could the doc comment state the rule itself? "Latest
main-history entry" can be read as the last entry in the log or as the entry
with the greatest timestamp, and those differ in exactly this case. Something
like "the entry with the greatest timestamp at or before `timestamp_ms`, taking
the first such entry on ties", followed by a sentence that PyIceberg takes the
last entry in log order, so the two can pick different snapshots when
timestamps tie or entries are out of order.
--
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]
