feat(scan): support timestamp-based time travel by dhruvarya-db · Pull Request #3260 · apache/iceberg-rust

dhruvarya-db · 2026-09-22T00:38:15Z

Which issue does this PR close?

None.

What changes are included in this PR?

Adds timestamp-based time travel through snapshot_id_as_of_time and TableScanBuilder::as_of_time:

let scan = table.scan().as_of_time(timestamp_ms).build()?;

The timestamp is in epoch milliseconds with an inclusive cutoff. Selection uses the loaded table's main snapshot history, including rollback events. The first history entry wins equal timestamps, and lookup handles clock-skewed history without assuming timestamp order.

The selected snapshot uses the existing snapshot-ID scan path, including its historical schema, projection, filtering and reader behavior. Unavailable history or a missing snapshot returns an error. A private snapshot-selection enum stores one choice; the last call to either selector takes precedence. Default scans remain unchanged. Public API signatures are included.

Are these changes tested?

Yes. Runnable doctests cover both new public APIs. Unit tests cover timestamp boundaries and extremes, rollback, ties, clock skew, unavailable history, missing snapshots, last-call-wins selection in both orders, historical schemas, and file-task/Arrow-row equivalence with explicit snapshot-ID scans.

The historical-row regression reads real Parquet data through distinct snapshots: timestamp and explicit-ID scans return the older row (x = 100) with a one-column schema, while the default scan returns 2,048 current rows (x = 1) with eight columns. The test was also verified to fail when timestamp selection was temporarily bypassed; the implementation was then restored.

AI Disclosure

Implemented with assistance from OpenAI Codex. All code has been manually reviewed.

laskoviymishka

Nice — this is a clean implementation and the test coverage is better than most first passes I see: unsorted/skewed logs, equal-timestamp ties, rollback records, expired prefixes, schema fallback, and a real end-to-end read against a historical snapshot. I traced the selection logic against Java's SnapshotUtil and it matches exactly — nullableSnapshotIdAsOfTime uses the same strict-> max-timestamp accumulator, so first-wins-on-ties and the ≤60s-skew cases you've encoded are the correct behavior, not a divergence. Good to see those pinned down in tests.

The one thing I'd settle before this merges is the public-API shape. snapshot_id_as_of_time takes &TableMetadata while ancestors_of/ancestors_between in the same module take &TableMetadataRef, and the param is metadata vs their table_metadata. Trivial to align now and semver-breaking once public-api.txt ships, so better before the first publish than after.

A few smaller things, none blocking:

The tie-break is correct but fragile to a well-meaning >→>= edit; I'd restructure it so first-wins is explicit and locally obvious (left an inline).
Worth a docstring line that the returned id may not resolve via snapshot_by_id if the snapshot was later expired.
Cross-engine wrinkle worth a doc note: we match Java (first-wins on ties), but PyIceberg iterates history in reverse and picks last-wins, so duplicate-timestamp queries can select different snapshots across engines. Not something to fix here — just worth acknowledging.
Test gaps I'd close: the builder-level rollback test steps past the equal-timestamp case (timestamp_ms + 1000), so the one path where ties actually matter isn't covered end-to-end; and there's no case where every qualifying log entry points at an evicted snapshot. The vec![1; 2048] fixture constant could use a name too.
New public APIs but no CHANGELOG entry.

Once the signature's locked down I'm happy to take another pass.

laskoviymishka · 2026-09-23T21:28:51Z

+///
+/// Equal timestamps select the first entry. Returns [`ErrorKind::DataInvalid`]
+/// if no matching history exists; does not check whether the snapshot is retained.
+pub fn snapshot_id_as_of_time(metadata: &TableMetadata, timestamp_ms: i64) -> Result<i64> {


I'd switch this to &TableMetadataRef to match ancestors_of/ancestors_between in this module. Right now a caller holding a TableMetadataRef (e.g. StaticTable::metadata()) has to deref for this one fn but not the siblings. &TableMetadata is technically sufficient since we only read an i64 out, but the inconsistency is cheap to fix now and a semver-breaking change once public-api.txt ships. While we're here, the param is metadata where the siblings use table_metadata — worth aligning too. The build() call site can stay self.table.metadata().

Updated to table_metadata: &TableMetadataRef

laskoviymishka · 2026-09-23T21:28:51Z

+pub fn snapshot_id_as_of_time(metadata: &TableMetadata, timestamp_ms: i64) -> Result<i64> {
+    let mut best: Option<&SnapshotLog> = None;
+    for entry in metadata.history() {
+        if entry.timestamp_ms() <= timestamp_ms


The tie-break is correct and matches Java's nullableSnapshotIdAsOfTime (strict > on the best timestamp, so the first entry at the max qualifying timestamp wins). My worry is it's easy to break silently: entry is captured in both the guard and the is_none_or closure, and a maintainer "simplifying" > to >= would flip ties to last-wins with no compile error. I'd make first-wins explicit with a reduce so the invariant stays local:

let best = metadata.history() .filter(|e| e.timestamp_ms() <= timestamp_ms) .reduce(|best, e| if e.timestamp_ms() > best.timestamp_ms() { e } else { best });

and keep a one-line note on why it's > (matches Java; first-appended wins).

The suggestion looks much cleaner. Updated!

laskoviymishka · 2026-09-23T21:28:51Z

+/// Resolve the snapshot ID from the latest main-history entry at or before
+/// `timestamp_ms` (milliseconds since the Unix epoch).
+///
+/// Equal timestamps select the first entry. Returns [`ErrorKind::DataInvalid`]


Since this is public now, worth spelling out that the returned id isn't guaranteed to still resolve — if the snapshot was expired after that log entry, snapshot_by_id returns None even though this returned Ok. build() handles it, but a standalone caller doing snapshot_by_id(snapshot_id_as_of_time(...)?) won't expect a None after an Ok.

Makes sense. Updated the docstring to clarify that the returned snapshot may have expired and snapshot_by_id can still return None.

laskoviymishka · 2026-09-23T21:28:51Z

+        }
+        assert_eq!(rows[0], vec![100]);
+        assert_eq!(rows[0], rows[1]);
+        assert_eq!(rows[2], vec![1; 2048]);


2048 here is 2 live files × 1024 rows from the fixture — worth a named const or a short comment, otherwise this assertion fails opaquely if the fixture ever changes.

dhruvarya-db · 2026-09-23T22:42:37Z

Thanks for the review @laskoviymishka!

Test gaps I'd close: the builder-level rollback test steps past the equal-timestamp case (timestamp_ms + 1000), so the one path where ties actually matter isn't covered end-to-end; and there's no case where every qualifying log entry points at an evicted snapshot

Addressed these test gaps and the other comments.

New public APIs but no CHANGELOG entry.

I didn't realize that we also add unreleased changes to the CHANGELOG (looks like a new process?), I have updated it with this change.

xanderbailey · 2026-09-24T11:09:27Z

Just an idea, I don't love that we fail at runtime if the user configures both an as of timestamp and a snapshot id to read. Is there a world where we should have a delegate pattern and a new scan type for this that does the resolution and produces the existing struct with a pinned snapshot?

Or... and I appreciate it would be a break but rather than having snapshot or timestamp we have some sort of selector enum? This would be more idiomatic in my opinion and would make the invalid stat unrepresentable at compile time? What do you think?

dhruvarya-db · 2026-09-24T23:31:54Z

@xanderbailey Do any of these make sense:

Fail in snapshot_id() or as_of_time() itself if the other has already been called instead of failing during build()
Let the second invocation simply override the first one (i.e. snapshot_id will clear the timestamp while as_of_time will clear the explicitly passed id. This one seems a bit risky.
The enum approach from your suggestion:

enum SnapshotSelection {
    Current,
    SnapshotId(i64),
    AsOfTime(i64),
}
table.scan()
    .with_snapshot_selection(
        SnapshotSelection::AsOfTime(timestamp_ms)
    )
    .build()?;

A problem with this is that the user can first invoke with_snapshot_selection(SnapshotSelection::AsOfTime(timestamp_ms)) followed by with_snapshot_selection(SnapshotSelection::SnapshotId(id)) within the same build chain. So it doesn't really solve our issue.
4. The new scan type solves all of these issues but I am not sure whether it is the most ergonomic solution:

table.scan_as_of_time(timestamp_ms)
    .select(["x"])
    .build()?;
 table.scan_as_of_snapshot_id(snapshot_id)
    .select(["x"])
    .build()?;

Option 1 seems like the simplest one here but I have no strong opinion here. Option 4 seems to solve the main issues.

xanderbailey · 2026-09-25T07:32:09Z

Yeah it's worth thinking about how all the options evolve as we add incremental and change log scans. To me option 4 looks like it would allow that? Or having .scan(ScanSelection) I think also works. I do appreciate either of this is a break but it does set us up for the future better than the current API. In https://github.com/apache/iceberg-rust/pull/2997/files which is a draft of incremental scans I added incremental_append_scan at the top level which is maybe correct? WDYT?

laskoviymishka

Thanks for turning this around — the helper signature now matches its siblings (&TableMetadataRef + table_metadata), which was my one blocker last round, so that's fully resolved. The docstring caveat about a resolved id possibly being expired is in, the CHANGELOG entry landed under Added, and both gaps I flagged are now covered end-to-end: test_table_scan_as_of_time_after_rollback exercises the elapsed_ms == 0 equal-timestamp tie through the builder, and test_table_scan_as_of_time_rejects_all_qualifying_snapshots_expired covers the all-expired path.

A few things are still open from last round, none blocking. The tie-break is still leaning on the comment rather than the structure — that's the one I'd still like to see land, and there's a clean sort-key shape for it (left inline). The PyIceberg cross-engine tie divergence still isn't noted in the docstring, and vec![1; 2048] is still an unnamed literal decoupled from the fixture row count.

One new thing I noticed this pass: the i64::MIN test case diverges from Java's sentinel seed, so I'd soften the "matches exactly" framing there rather than leave it as an implied parity guarantee.

Overall it is LGTM, i'll leave it for a day or two to soak and sort this out and then - happy to merge.

laskoviymishka · 2026-09-25T09:03:55Z

+    let best = table_metadata
+        .history()
+        .iter()
+        .filter(|entry| entry.timestamp_ms() <= timestamp_ms)


This is the tie-break restructure from last round — still leaning on the comment rather than the structure. A one-char >→>= here silently flips first-wins to last-wins and nothing but CI would catch it. I'd encode first-wins in the sort key so misreading the operator can't change semantics:

table_metadata .history() .iter() .enumerate() .filter(|(_, entry)| entry.timestamp_ms() <= timestamp_ms) .max_by_key(|(idx, entry)| (entry.timestamp_ms(), Reverse(*idx))) .map(|(_, entry)| entry.snapshot_id)

max_by_key on its own picks the last max on ties, so Reverse(idx) is doing the real work; the result is still an Option, so it feeds the existing .ok_or_else(...) unchanged.

Switched to max_by_key with Reverse(idx) to preserve first-entry-wins ties, keeping the Java compatibility comment directly above it

laskoviymishka · 2026-09-25T09:03:55Z

+        assert_eq!(rows[0], vec![100]);
+        assert_eq!(rows[0], rows[1]);
+        // Two live fixture files contain 1,024 rows each.
+        assert_eq!(rows[2], vec![1; 2048]);


The 2048 is still decoupled from its source of truth — if write_parquet_data_files' vec![1; 1024] ever changes, this assertion silently goes stale. A const ROWS_PER_FIXTURE_FILE: usize = 1024; with vec![1; 2 * ROWS_PER_FIXTURE_FILE] ties them back together.

Makes sense, updated to use ROWS_PER_FIXTURE_FILE

xanderbailey · 2026-09-25T09:49:52Z

@laskoviymishka what do you think about my suggestions above?

laskoviymishka · 2026-09-26T09:28:08Z

@xanderbailey I think you're right on the direction. If this can only fail at build(), we probably have the wrong API shape.

I'd lean toward separate entry points that resolve to a pinned snapshot and return the existing TableScan. That also lines up with incremental_append_scan in #2997.

I don't think a selector on the builder really fixes it, since it can still be called twice and overwritten. So to me it's mostly between:

scan_as_of_time / scan_as_of_snapshot_id
one scan(selection) entry point

I'd slightly prefer the separate constructors, but no strong opinion.

I also wouldn't block this PR on it. My only ask is that if we're changing the scan API, we try to do it once. This PR and #2997 touch the same surface, so it'd be good to settle them together before the next release.

mbutrovich · 2026-09-28T19:20:30Z

My only ask is that if we're changing the scan API, we try to do it once. This PR and #2997 touch the same surface, so it'd be good to settle them together before the next release.

Could we settle the builder side in this PR by keeping as_of_time on TableScanBuilder and storing the selection in one private field?

enum SnapshotSelection {
    SnapshotId(i64),
    AsOfTime(i64),
}

The builder would hold snapshot_selection: Option<SnapshotSelection>, and snapshot_id and as_of_time would each overwrite it. The builder can't represent the conflicting state, so the error in build() goes away, and the public change stays at the one new method. A branch or tag selector later becomes another variant.

@dhruvarya-db's concern with the enum was that a second call overwrites the first. This builder already works that way. select, select_all, and select_empty all write column_names (code), and this PR documents last-call-wins for repeated snapshot_id calls and for repeated as_of_time calls. Java resolves its mutually exclusive incremental start bounds the same way. fromSnapshotInclusive and fromSnapshotExclusive write the same fromSnapshotId field plus a flag, so the last call wins (TableScanContext). Java does reject a second snapshot selection on a table scan (SnapshotScan). Doing that here would mean changing snapshot_id to return a Result, which breaks every caller.

As I read Java, this shape also lines up with #2997 without new constructors. Point-in-time selection lives on the table scan (useSnapshot, useRef, asOfTime in the SnapshotScan link above), and incremental reads get their own entry point, Table.newIncrementalAppendScan(), which matches Table::incremental_append_scan in #2997. Separate scan_as_of_time and scan_as_of_snapshot_id constructors would also mean deprecating the existing TableScanBuilder::snapshot_id.

mbutrovich

Thanks @dhruvarya-db. The test that reads real rows back through two distinct snapshots shows the time-travel path end to end. Before this merges, I'd like to settle the scan builder's shape together with #2997, since both PRs change the same API.

laskoviymishka

Thanks for turning the round-2 items around — the PyIceberg-divergence note is in the docstring now, ROWS_PER_FIXTURE_FILE replaced the magic vec![1; 2048], and the rollback / expired-prefix / schema-fallback tests all landed. I re-traced the selection against Java's SnapshotUtil once more and it still lines up (max timestamp, first-wins-on-ties), so the logic looks good to me.

On my end this is ready — but I'd hold the merge for @mbutrovich. He asked to settle the scan-builder shape jointly with #2997 (the ScanConfig / shared build_table_scan refactor), and whichever of the two lands first, the other's per-variant resolution in build() will want to move into that shape. I'd rather he takes it over the line once that's sorted than land ahead of it.

Left a few small non-blocking notes inline — an error that could carry the timestamp when a resolved snapshot has expired, plus the same tie-break/docstring wording nits from last round. None of them gate. Nice work.

laskoviymishka · 2026-09-30T19:28:01Z

-        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(


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.

Good point, I have updated the snapshot_selection logic such that we create the error message in advance when the snapshot_id is resolved

laskoviymishka · 2026-09-30T19:28:01Z

+/// 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,


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.

Made the docstring more descriptive

laskoviymishka · 2026-09-30T19:28:01Z

+        .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)))


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:

.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 })

Updated as per your suggestion

Co-authored-by: Isaac <no-reply@databricks.com>

laskoviymishka

@dhruvarya-db -- can you plz fix conflicts?
Once fixed i think this is good to merge

…at-timestamp-time-travel Resolve conflicts with apache#2904: keep both test sets and docstrings, and make as_of_time scans use the selected snapshot's schema like snapshot_id scans. Co-authored-by: Isaac <no-reply@databricks.com>

dhruvarya-db · 2026-10-20T20:52:56Z

Hi @laskoviymishka , I have resolved the merge conflicts. There is one CI failure which is unrelated to this PR - I have raised this PR to resolve that issue #3496

…-travel Co-authored-by: Isaac <no-reply@databricks.com>

laskoviymishka · 2026-10-21T07:58:36Z

let's 🌬️ ⛵

Backport of apache#3424 (net diff of PR head 86e35d3 against its merge base beba39c), adapted to base 5b82ab9. Adds TableScanBuilder::with_column_stats(names) and FileScanTask::column_stats(). Tasks carry the data file's column sizes, value/null/NaN counts and lower/upper bounds only for the named primitive columns. By default no statistics are kept and behaviour is unchanged. Upstream-PR: apache#3424 Upstream-head: 86e35d3 Backport-notes: scan/mod.rs conflicts resolved for the older base: resolve_field_id keeps its Option return (no apache#3070 ambiguity check), TableScanBuilder keeps snapshot_id (no apache#3260 snapshot_selection), and apache#3070's ambiguity tests are not carried. Datum::promote_to (apache#3483) is inlined as the same int->long / float->double widening in task.rs. Tests use Type::Primitive(..) (pre-apache#3527 NestedField signature) and test_utils imports SchemaRef. Details in DFD_FORK.md. Needed-by: https://github.com/Perseus/datafusion-distributed/tree/iceberg-dyn-filter/f2-fork-column-stats Remove-when: the pinned iceberg-rust rev contains apache#3424's merge commit Issue: apache#3422

laskoviymishka requested changes Sep 23, 2026

View reviewed changes

dhruvarya-db requested a review from laskoviymishka September 23, 2026 22:44

laskoviymishka approved these changes Sep 25, 2026

View reviewed changes

mbutrovich reviewed Sep 28, 2026

View reviewed changes

Comment thread crates/iceberg/src/scan/mod.rs Resolved

Comment thread crates/iceberg/src/util/snapshot.rs Resolved

dhruvarya-db requested a review from mbutrovich September 29, 2026 01:11

laskoviymishka approved these changes Sep 30, 2026

View reviewed changes

durovo and others added 13 commits October 12, 2026 18:34

feat(scan): support timestamp-based time travel

a778e60

test(scan): verify timestamp scans read historical rows

f8dfe1c

docs: shorten timestamp time travel docstrings

556d943

fix: refine timestamp selection API and edge-case coverage

dfaee2d

docs: add timestamp time travel changelog entry

4216966

test: consolidate timestamp time travel coverage

651acee

refactor: use a single snapshot selector with last-call-wins semantics

d1b9fb8

docs: clarify timestamp history selection and PyIceberg differences

64c8e37

test: share the Parquet fixture row count

2daedb1

docs: note Java timestamp ordering compatibility

74c9f11

docs: clarify minimum timestamp difference from Java

fd53db9

docs: describe snapshot selection enum

7cee25e

test: adapt timestamp time travel tests to relocated scan fixture

387174e

Co-authored-by: Isaac <no-reply@databricks.com>

dhruvarya-db force-pushed the feat-timestamp-time-travel branch from f44caef to 387174e Compare October 12, 2026 18:39

durovo and others added 2 commits October 12, 2026 19:48

fix: name the timestamp when an as-of-time snapshot is missing

94bd3ff

Co-authored-by: Isaac <no-reply@databricks.com>

refactor: spell out the as-of-time tie-break and i64::MIN edge

52f2a58

Co-authored-by: Isaac <no-reply@databricks.com>

laskoviymishka approved these changes Oct 20, 2026

View reviewed changes

Merge remote-tracking branch 'upstream/main' into feat-timestamp-time…

37dda73

…-travel Co-authored-by: Isaac <no-reply@databricks.com>

laskoviymishka added this pull request to the merge queue Oct 21, 2026

Merged via the queue into apache:main with commit b9d2855 Oct 21, 2026
24 checks passed

Conversation

dhruvarya-db commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

AI Disclosure

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruvarya-db Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhruvarya-db commented Sep 23, 2026

Uh oh!

xanderbailey commented Sep 24, 2026

Uh oh!

dhruvarya-db commented Sep 24, 2026

Uh oh!

xanderbailey commented Sep 25, 2026

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xanderbailey commented Sep 25, 2026

Uh oh!

laskoviymishka commented Sep 26, 2026

Uh oh!

mbutrovich commented Sep 28, 2026

Uh oh!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

dhruvarya-db commented Oct 20, 2026

Uh oh!

laskoviymishka commented Oct 21, 2026

dhruvarya-db commented Sep 22, 2026 •

edited

Loading

dhruvarya-db Sep 23, 2026 •

edited

Loading