feat: inherit data-file first_row_id on manifest read by anoopj · Pull Request #2922 · apache/iceberg-rust

anoopj · 2026-07-29T05:30:09Z

Which issue does this PR close?

What changes are included in this PR?

Manifest entries inherit snapshot and sequence numbers on read but not first_row_id, leaving the field parse-only. Assign it during load_manifest from the manifest-level first_row_id plus a running record count, skipping DELETED entries and preserving any already-set value. Mirrors Java's ManifestReader.idAssigner.

This is the first step toward reading the _row_id metadata column, which is derived from a data file's first_row_id and each row's position.

Are these changes tested?

added tests

Manifest entries inherit snapshot and sequence numbers on read but not first_row_id, leaving the field parse-only. Assign it during load_manifest from the manifest-level first_row_id plus a running record count, skipping DELETED entries and preserving any already-set value. Mirrors Java's ManifestReader.idAssigner. This is the first step toward reading the _row_id metadata column, which is derived from a data file's first_row_id and each row's position. Refs apache#2879

laskoviymishka

The mechanics here are solid. The counter logic is a faithful port of Java's idAssigner Branch 1 - advance only on assignment, skip pre-assigned and DELETED entries - the overflow and oversized-offset guards are real, and the interleaved test nails the tricky case. But I'd hold this before merge: there are two correctness issues, and the rest of the _row_id stack is going to sit directly on top of them.

The bigger one is that wiring id assignment into load_manifest makes the result snapshot-dependent while the ObjectCache still keys manifests by path alone - so time-travel or branched reads can hand back another snapshot's first_row_id values from cache. The second is that a delete manifest carrying a stray non-null first_row_id now hard-errors the whole scan where Java silently ignores it, which is a one-sided interop break. There's also a missing Java branch (nulling per-file ids when the manifest-level offset is absent) that bites on the v3 upgrade path.

None of these are hard to address, and the shape of the PR is right.

Once the cache keying and the two Java-parity branches are sorted, this is a clean base for the _row_id column read work. Happy to take another pass then.

laskoviymishka · 2026-07-29T14:15:07Z

            entry.inherit_data(self);
        }

+        self.assign_first_row_ids(&mut entries)?;


I think this introduces a stale-cache bug, and it's the thing I'd most want to settle before merge.

Until now load_manifest was a pure function of the Avro bytes, so the ObjectCache keying manifests by manifest_path alone (object_cache.rs:108) was safe. This call makes the result depend on self.first_row_id, the manifest-list-level offset — and the spec lets the same physical manifest carry different offsets across snapshots: the v3 upgrade path (None in S1, Some(X) in S2 after the first post-upgrade commit) and branches (two branches can hand the same manifest disjoint offsets). A time-travel or branched read in one Table lifetime hits the cache on the second request and gets the first request's first_row_id values, silently wrong.

I'd fold first_row_id into the cache key (CachedObjectKey::Manifest((String, Option<u64>))), or split the Avro parse (cached) from the id assignment (applied post-retrieval). There's also no test loading the same manifest_path twice with different offsets — that's exactly the case that catches this, so I'd add it alongside the fix. wdyt?

This was a really good catch. I forgot that we are caching deserialized files unlike Java. Thank you! Folded first_row_id into the cache key so the same manifest path referenced with different offsets no longer shares an entry. Added test_get_manifest_keys_on_first_row_id that loads the same path twice with different offsets and asserts distinct inherited ids.

laskoviymishka · 2026-07-29T14:15:07Z

+
+        // A `first_row_id` is only valid on data manifests; delete files always
+        // have a null `first_row_id`.
+        if self.content != ManifestContentType::Data {


I'd make this a no-op rather than a hard error. The spec's "first_row_id is always null for delete manifests" is a write-side constraint; Java's readDeleteManifest() just omits the firstRowId argument to the reader and silently ignores the field. Rejecting here means a delete manifest written by any other client with a stray non-null first_row_id (buggy writer, partial migration) is readable everywhere except iceberg-rust, where it fails the entire scan — a one-sided interop break.

I'd return Ok(()) here, maybe with a tracing::warn! to surface the anomaly. If we do decide to keep it strict, capitalise the message to match the others in the file. wdyt?

I was on the fence about this one, because it seems like a spec violation. But I agree that we should probably be consistent with Java. Changed to no-op + tracing::warn!, matching Java implementation. Test updated accordingly (test_assign_first_row_ids_ignores_delete_manifest).

laskoviymishka · 2026-07-29T14:15:07Z

+    /// entry's record count. See the row-lineage inheritance rules in
+    /// <https://github.com/apache/iceberg/blob/main/format/spec.md#first-row-id-inheritance>.
+    fn assign_first_row_ids(&self, entries: &mut [ManifestEntry]) -> Result<()> {
+        let Some(manifest_first_row_id) = self.first_row_id else {


I think we're missing Java's third idAssigner branch here. When the manifest-level first_row_id is null on a committed read, Java doesn't just return — it walks the entries and sets each file's first_row_id back to null (ManifestReader.idAssigner, the BaseFile.setFirstRowId(null) branch).

That's the upgrade-path guard: a v3 data manifest with no manifest-level offset but EXISTING entries that carry a previously-inherited first_row_id. Java exposes null there; this early-return leaves the stale per-file values intact, so anything computing _row_id = first_row_id + _pos on the Rust side gets wrong ids. I'd add a data-manifest path that clears entry.data_file.first_row_id when self.first_row_id is None. wdyt?

Good catch. Added the null-clearing path. Covered by test_assign_first_row_ids_clears_without_manifest_first_row_id.

laskoviymishka · 2026-07-29T14:15:07Z

+                    Error::new(
+                        ErrorKind::DataInvalid,
+                        format!(
+                            "Row ID overflow assigning first_row_id in {}. Next Row ID: {next_row_id}, Record Count: {record_count}",


Small wording thing — the value is right but the label reads wrong. At this point next_row_id still holds the id we just stamped onto the current entry (the add hasn't landed), so "Next Row ID" makes an operator read it as the next value to assign when it's actually the triggering file's own first_row_id. I'd reword to something like "file first_row_id={next_row_id} + record_count={record_count} exceeds i64::MAX".

Reworded, and introduced a file_first_row_id binding so the message names the triggering file's own id rather than "next".

laskoviymishka · 2026-07-29T14:15:07Z

+            ));
+        }
+
+        let mut next_row_id = i64::try_from(manifest_first_row_id).map_err(|_| {


This try_from is only necessary because ManifestFile.first_row_id (and ManifestFileV3.first_row_id in _serde.rs) is Option<u64>, while spec field 520 and Java's ManifestFile.firstRowId() are signed long — and DataFile.first_row_id is already Option<i64>. Non-negative values round-trip fine over Avro, so it's low practical risk, but the unsigned type is what forces the conversion and makes it slightly fragile.

Not blocking, but I'd consider moving both fields to Option<i64> to match the spec and dropping the try_from (the checked_add_unsigned would need a revisit). Probably its own cleanup PR.

I recommend leaving it as is. The u64 type is arguably the a better internal model because a first_row_id is non-negative by nature, and the Iceberg field is a signed long in the spec only because Avro has no unsigned type. The i64::try_from is a boundary conversion.

Also the manifest list writer (ManifestListWriter class models it as u64, so we should probably be consistent.

laskoviymishka · 2026-07-29T14:15:07Z

    }
+
+    /// Assigns `first_row_id` to data-file entries that do not already have one,
+    /// starting from the manifest-level `first_row_id` and advancing by each


The doc says the counter advances "by each entry's record count", but the code only advances for entries that actually receive an assignment — pre-assigned and DELETED entries are skipped and don't move it (which the interleaved test verifies). I'd tighten it to say the counter advances by each newly-assigned entry's record count, and note the skips.

Tightened the doc to say the counter advances by each newly-assigned entry's record count, and that pre-assigned and non-live entries are skipped.

laskoviymishka · 2026-07-29T14:15:07Z

+            // must not advance the running counter.
+            data_entry(ManifestStatus::Added, 5, Some(100)),
+            // A deleted entry likewise neither receives nor consumes a row id.
+            data_entry(ManifestStatus::Deleted, 7, None),


Nice interleaved test. One case I'd add while we're here: a DELETED entry arriving with a pre-set first_row_id (data_entry(Deleted, 7, Some(999))) asserting it stays Some(999). The skip-deleted path leaves pre-set values untouched today, and a test makes that invariant explicit rather than incidental.

Added. The interleaved test's deleted entry now carries Some(999) and asserts it survives untouched

laskoviymishka · 2026-07-29T14:15:07Z

+            .file_format(DataFileFormat::Parquet)
+            .file_size_in_bytes(4096)
+            .record_count(record_count);
+        if let Some(id) = first_row_id {


Minor: DataFile's first_row_id is #[builder(default)] without strip_option, so the setter takes Option<i64> directly and defaults to None. This whole block collapses to builder.first_row_id(first_row_id);.

laskoviymishka

This in really nice shape now!

Will keep 🧼-ing for day before merging.

laskoviymishka requested changes Jul 29, 2026

View reviewed changes

fix: address review on first_row_id inheritance

6e115fe

anoopj force-pushed the inherit-first-row-id branch from 724cf92 to 6e115fe Compare July 31, 2026 01:28

anoopj requested a review from laskoviymishka July 31, 2026 01:29

laskoviymishka approved these changes Jul 31, 2026

View reviewed changes

Uh oh!

Conversation

anoopj commented Jul 29, 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?

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anoopj Jul 30, 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!

anoopj Jul 30, 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!

anoopj Jul 30, 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!

anoopj Jul 30, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

anoopj commented Jul 29, 2026 •

edited

Loading

anoopj Jul 30, 2026 •

edited

Loading

anoopj Jul 30, 2026 •

edited

Loading

anoopj Jul 30, 2026 •

edited

Loading

anoopj Jul 30, 2026 •

edited

Loading