feat: carry first_row_id and data sequence number on FileScanTask

[anoopj]

Which issue does this PR close?

Refs #2879

What changes are included in this PR?

Manifest read now inherits a data file's first_row_id (#2922)

Add both fields to FileScanTask and populate them in ManifestEntryContext::into_file_scan_task from the manifest entry's data_file().first_row_id() and sequence_number().

This is step 2 of reading the row-lineage metadata columns; positional materialization in the arrow pipeline follows.

Are these changes tested?

Added tests.

AI Disclosure

Assisted with Claude Code (Opus 4.8). I have reviewed and iterated to get to this shape.

[laskoviymishka]

Nice, focused step 2. Removing the two with_* calls at that point in the builder chain means the task sees the values after inherit_data() and assign_first_row_ids() have run, which is the important part here.

No objection to merging this. I’d just tighten the v3 test first.

The fixture uses a manifest-level first_row_id of 0, so the inherited per-file value is also Some(0). An implementation that always returned Some(0) would still pass. Using a non-zero base would make the inheritance arithmetic visible.

That test also does not assert data_sequence_number, even though both fields are wired in by the same two-line change in context.rs. Right now the v3 path is only partly covered.

Two things to keep in mind for step 3:

• ManifestFile.first_row_id is Option<u64>, while field 520 in the spec is a long, and assign_first_row_ids rejects values above i64::MAX with DataInvalid. The Option<i64> used here on FileScanTask looks right; the manifest-side type is the inconsistent one.
• CachedObjectKey::Manifest is keyed only by manifest path, while assigned row IDs are stored in the cached Arc<Manifest>. If the same manifest path is used by two snapshots with different manifest-level first_row_id values, the second scan can get the IDs assigned during the first scan.

Neither issue is introduced here, but this change makes both visible through scan results, and they will matter once positional materialization is added.

[laskoviymishka]

I'd make this non-zero — Some(42), anything. With a manifest-level first_row_id of 0 the inherited per-file value comes out Some(0) as well, so an implementation that hands back Some(0) unconditionally, or one that confuses None with a zero default, passes this test exactly like the correct one does.

The inheritance arithmetic is the interesting part here, and a non-zero base is what makes it observable. The assertion below then becomes Some(42).

[anoopj]

That is a good point and 42 is a good choice. :) Fixed the assertion

[laskoviymishka]

The index-to-file mapping is implicit — it holds only because 1.parquet sorts ahead of 3.parquet, and the comment describes that rather than asserting it. Rename those fixture files and the two assertions silently swap and still pass. I'd pin the path next to each sequence number:

assert_eq!(tasks[0].data_file_path, format!("{}/1.parquet", &fixture.table_location));
assert_eq!(tasks[0].data_sequence_number, Some(1));
assert_eq!(tasks[1].data_file_path, format!("{}/3.parquet", &fixture.table_location));
assert_eq!(tasks[1].data_sequence_number, Some(0));

While we're here, the tasks.len() assert wants to be above the sort — as written, a scan that returns fewer tasks panics on tasks[0] instead of failing with the length message.

[anoopj]

Done both. Pinned data_file_path next to each data_sequence_number assertion so the file->value mapping is explicit and survives a fixture rename, and moved the task len assert above the sort .

[laskoviymishka]

Worth asserting data_sequence_number here too. Both fields come from the same two-line addition in context.rs, so covering only one of them leaves the v3 path half tested — a regression that broke sequence-number threading specifically under build_v3_data() wouldn't be caught anywhere.

The current snapshot's sequence number is 1, so assert_eq!(task.data_sequence_number, Some(1)); right after this should do it.

[anoopj]

Added an asertion. Renamed the test to test_plan_files_carries_row_lineage_from_v3_manifest

[laskoviymishka]

This reads as though the value comes off DataFile, but it's ManifestEntry::sequence_number() — field 3 on the entry envelope, nothing inside the data file. Worth saying so, and worth naming it as the data sequence number as opposed to file_sequence_number, since the spec carries both and they're easy to swap.

A word on None would help too: inherit_data() only fills this in when the snapshot's sequence number is the initial one, so an Existing entry in a malformed v2 manifest can reach the task with None. Better pinned here than assumed Some by the _last_updated_sequence_number work.

[anoopj]

Reworded to call out the data-vs-file distinction explicitly and noted the null case.