laskoviymishka commented on code in PR #3260:
URL: https://github.com/apache/iceberg-rust/pull/3260#discussion_r4148496335
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -282,7 +317,15 @@ impl<'a> TableScanBuilder<'a> {
/// Build the table scan.
pub fn build(self) -> Result<TableScan> {
- let snapshot = match self.snapshot_id {
+ let snapshot_id = match self.snapshot_selection {
+ Some(SnapshotSelection::SnapshotId(id)) => Some(id),
+ Some(SnapshotSelection::AsOfTime(timestamp_ms)) =>
Some(snapshot_id_as_of_time(
Review Comment:
When `as_of_time` resolves to a since-GC'd snapshot, `build()` fails with
the bare `Snapshot with id {X} not found` from the id path — the timestamp that
produced X is gone, so a caller can't tell an expired time-travel target from a
stale `snapshot_id`. I'd wrap the not-found case in this branch to carry the
timestamp, e.g. `Snapshot ... resolved from timestamp {timestamp_ms} ms has
expired`. Non-blocking.
##########
crates/iceberg/src/util/snapshot.rs:
##########
@@ -76,10 +78,57 @@ pub fn ancestors_between(
})
}
+/// Resolve the snapshot ID from the main-history entry with the greatest
timestamp
+/// at or before `timestamp_ms` (milliseconds since the Unix epoch), taking
the first
+/// entry on ties.
+///
+/// This matches Java's ordering. PyIceberg takes the last qualifying log
entry,
+/// so ties or out-of-order timestamps can produce different results.
+///
+/// Returns [`ErrorKind::DataInvalid`] if no matching history exists.
+/// The returned snapshot may have expired, so
+/// [`snapshot_by_id`](crate::spec::TableMetadata::snapshot_by_id) can still
return `None`.
+///
+/// ```
+/// # use iceberg::TableIdent;
+/// # use iceberg::io::FileIO;
+/// # use iceberg::table::StaticTable;
+/// use iceberg::util::snapshot::snapshot_id_as_of_time;
+/// # #[tokio::main]
+/// # async fn main() -> iceberg::Result<()> {
+/// # let location = concat!(env!("CARGO_MANIFEST_DIR"),
"/testdata/example_table_metadata_v2.json");
+/// # let ident = TableIdent::from_strs(["ns", "t"])?;
+/// # let table = StaticTable::from_metadata_file(location, ident,
FileIO::new_with_fs()).await?;
+/// let metadata = table.metadata();
+/// // The snapshot log has entries at 1515100955770 and 1555100955770.
+/// let snapshot_id = snapshot_id_as_of_time(&metadata, 1_555_100_955_770)?;
+/// assert_eq!(snapshot_id, 3055729675574597004);
+/// assert!(snapshot_id_as_of_time(&metadata, 1_515_100_955_769).is_err());
+/// # Ok(())
+/// # }
+/// ```
+pub fn snapshot_id_as_of_time(table_metadata: &TableMetadataRef, timestamp_ms:
i64) -> Result<i64> {
+ table_metadata
+ .history()
+ .iter()
+ .enumerate()
+ .filter(|(_, entry)| entry.timestamp_ms() <= timestamp_ms)
+ // Keep the first entry on ties, matching Java's timestamp selection.
+ .max_by_key(|(idx, entry)| (entry.timestamp_ms(), Reverse(*idx)))
Review Comment:
Still the one place the tie-break leans on the comment rather than the shape
— `Reverse(*idx)` inside `max_by_key` is two hops to read as "first entry
wins." Not gating, but if you're in here anyway, an explicit comparator says it
directly:
```rust
.max_by(|(idx_a, a), (idx_b, b)| {
a.timestamp_ms()
.cmp(&b.timestamp_ms())
.then(idx_b.cmp(idx_a)) // earlier index wins on ties
})
```
##########
crates/iceberg/src/util/snapshot.rs:
##########
@@ -76,10 +78,57 @@ pub fn ancestors_between(
})
}
+/// Resolve the snapshot ID from the main-history entry with the greatest
timestamp
+/// at or before `timestamp_ms` (milliseconds since the Unix epoch), taking
the first
+/// entry on ties.
+///
+/// This matches Java's ordering. PyIceberg takes the last qualifying log
entry,
Review Comment:
This still reads as a flat "matches Java's ordering" — the one spot it
doesn't is `i64::MIN`, where Java's `Long.MIN_VALUE` seed plus strict `>`
matches nothing but we return the entry. The test comment already notes this;
I'd lift a sentence of it up into the docstring so API consumers see the
sentinel edge. Same wording point from last round, still not gating.
--
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]
