feat: coalesce physically-stored _last_updated_sequence_number by anoopj · Pull Request #2985 · apache/iceberg-rust

anoopj · 2026-08-10T15:47:14Z

Which issue does this PR close?

What changes are included in this PR?

The prior change rejected files that physically carry a per-row _last_updated_sequence_number column.

Read such columns instead and coalesce per the spec. i.e. use the per-row value where non-null, fall back to the file's data sequence number only where null.

_row_id is a later PR.

Are these changes tested?

Added tests.

The prior change rejected files that physically carry a per-row _last_updated_sequence_number column. Read such columns instead and coalesce per the spec. i.e. use the per-row value where non-null, fall back to the file's data sequence number only where null. _row_id, whose fallback is positional, is a later PR.

laskoviymishka

Good stuff, unifying the constant, null, and coalesce paths on the run-end-encoded type so batches concat cleanly is the right call, and the three-file concat test is exactly the guard I'd want on it. I'd hold this before merging though, since the _row_id follow-up is going to build directly on this coalesce shape and a couple of the gating decisions are worth settling first.

The one I care most about is the coalesce filter: when first_row_id is None, a file physically carrying non-null per-row _last_updated_sequence_number values comes back all-null, because we drop the leaf before projecting it. Java does the same thing, so it's not an interop regression — but it's silent data loss for a spec-legal file, and nothing in the code marks it as a deliberate Java-parity choice. I'd either pass non-null per-row values through, or keep the behavior and say explicitly that it matches Java plus file a follow-up. Right now the divergence is invisible.

Things I'd like to settle in this PR before the follow-ups build on it:

Decide and document the first_row_id = None / data_sequence_number = None coalesce behavior — pass non-null physical values through, or note it as intentional Java parity.
Hoist the last_updated_seq_present_by_name_only reject before the match so it fires in every state, not just (Some, Some), and add a test for the (Some, None) + physical-column case.
Make the Coalesce variant's Long-only constraint explicit (rename or validate at registration), and switch the source-type mismatch to DataInvalid with Display.
Add an all-null coalesce test to lock the is_not_null mask polarity, and confirm the _pos test's required id field actually resolves against the written file.

The doc/invariant touch-ups are genuinely optional — flagged inline as nits. Once the gating and the Long-pinning are settled, happy to take another pass and approve.

laskoviymishka · 2026-08-11T08:04:01Z

+        // `build_field_id_map` is all-or-nothing (`None` if any column lacks an id), so a
+        // file mixing id-bearing and id-less columns is rejected below rather than
+        // coalesced -- safe, and consistent with the rest of the reader.
+        let phys_last_updated_seq_leaf = if project_last_updated_seq {


build_field_id_map is all-or-nothing, so a single id-less sibling column turns a file that does carry _last_updated_sequence_number by its reserved id into a hard FeatureUnsupported — the name-only branch fires and we reject a file we could actually coalesce.

The comment frames this as safe, and it's not wrong, but it's a behavior choice (reject a readable file), not a pure safety guard. Would a targeted scan of the parquet leaves for just RESERVED_FIELD_ID_LAST_UPDATED_SEQUENCE_NUMBER be worth it, or do we accept the all-or-nothing and just make the comment say plainly that a mixed-id file is rejected? wdyt?

Went with accept + simpler comment. Reworded the comment to state the mixed-id file is rejected as a behavior choice.

laskoviymishka · 2026-08-11T08:04:01Z

+        // is row-lineage-bearing (first_row_id set) with a data sequence number to fall
+        // back to. When first_row_id is null the column is nulled, so we must not read it.
+        let coalesce_last_updated_seq_leaf = phys_last_updated_seq_leaf
+            .filter(|_| task.first_row_id.is_some() && task.data_sequence_number.is_some());


This .filter(...) couples two separate decisions: whether we can compute a fallback (needs data_sequence_number) and whether we should read the physical column at all. When first_row_id is None we drop the leaf entirely, so a file that physically carries non-null per-row values comes back all-null:

file column: [Some(5), None, Some(8)] (first_row_id = None) reader output: [null, null, null] (leaf not projected -> (None,_) arm)

Java does the same (ValueReaders.lastUpdated nulls when the base row id is null), so this isn't an interop regression — but it's silent data loss for a file the spec doesn't forbid. The data_sequence_number = None half is more anomalous, but the same shape: non-null per-row values vanish.

I'd either project the column and pass non-null values through (leaving null rows null when there's no fallback), or keep the current behavior but say in the comment that it matches Java and file a follow-up tracking the divergence. Right now the divergence is invisible.

Kept the behavior, documented it as a deliberate Java-parity choice. Passing values through would diverge from Java because a file with no row lineage has no meaningful per-row sequence numbers, so I'd rather keep them consistent.

One comment about the data_sequence_number=None half: that one isn't silent. It lands in the (Some, None) arm which returns DataInvalid. Added a test for it.

Filed #2991 to track the issue.

laskoviymishka · 2026-08-11T08:04:01Z

+                            RESERVED_FIELD_ID_LAST_UPDATED_SEQUENCE_NUMBER,
+                            datum,
+                        )
+                    } else if last_updated_seq_present_by_name_only {


The last_updated_seq_present_by_name_only reject only fires inside the (Some, Some) arm. For (Some, None) or (None, _) with a name-only column present, the flag is computed but never evaluated, so that file gets silently nulled instead of the loud FeatureUnsupported.

I'd hoist this check to just before the match so it fires regardless of the first_row_id / data_sequence_number state. The (Some, None) arm also isn't exercised — could we add a test for first_row_id = Some, data_sequence_number = None with a physical column, so the intended behavior there is pinned down?

Added the (Some, None) + physical-column test which asserts DataInvalid. We should not hoist because the transformer keys the source column by field id and can't thread an id-less column. But that only matters when we actually read the column, which only the (Some, Some) arm does. In (None, _) we synthesize an all-null column without touching the physical data, so an ordinary v2/null-lineage file that happens to carry a same-named, id-less column is correctly nulled today.

Hoisting the reject would turn that correct null into a hard FeatureUnsupported. So the reject is arm-local by design; I added a comment stating that. If you still want it hoisted I can, but I think arm-local seems correct to me. wdyt?

laskoviymishka · 2026-08-11T08:04:01Z

+            Schema::builder()
+                .with_schema_id(1)
+                .with_fields(vec![
+                    NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(),


This task schema declares id as required and projects field 1, but write_plain_parquet only writes the physical _last_updated_sequence_number column here — no id column. A required field absent from the file usually surfaces as DataInvalid("Missing required field: id"), yet the test expects success.

Either write_plain_parquet is quietly inserting an id column (in which case the test passes for a reason it doesn't state), or this should be optional / carry an initial default. Could we double-check which, so the test verifies what it claims?

Double-checked. write_plain_parquet always writes id=[1,2,3] as its first column, so the required id field does resolve against the file. The test is correctly passing.

laskoviymishka · 2026-08-11T08:04:01Z

                            target_type: arrow_type.clone(),
                        });
                    }
+                    Some(ColumnConstant::Coalesce(datum)) => {


Nit: this arm returns ColumnSource::Coalesce unconditionally, skipping the is_metadata_field || !present_in_file guard the Scalar arm applies. Correct today since _last_updated_sequence_number is always a metadata field — nothing enforces it, though. A one-line comment noting the invariant (or a debug_assert!) would be plenty; not worth more than that.

Added a comment.

laskoviymishka · 2026-08-11T08:04:01Z

                        child_values,
                    } => Self::create_struct_column(fields, child_values, num_rows)?,
+
+                    ColumnSource::Coalesce {


ColumnSource::Coalesce carries a generic fallback: PrimitiveLiteral and target_type: DataType, but create_coalesce_column silently pins both to Long / Int64. Combined with with_coalesced_metadata_column being infallible, a future caller with a non-Long datum gets no signal at registration and instead hits ErrorKind::Unexpected deep in batch processing.

I'd make the constraint explicit — either rename the variant to something like CoalesceLastUpdatedSeq so no one reuses it for another primitive, or validate the datum type in with_coalesced_metadata_column so it fails at the call site. Renaming is less code and states the intent.

That's fair. Renamed both variants and the builder. I didn't see a need to generalize at the moment.

laskoviymishka · 2026-08-11T08:04:01Z

+        fallback: &PrimitiveLiteral,
+        target_type: &DataType,
+    ) -> Result<ArrayRef> {
+        if source.data_type() != &DataType::Int64 {


A file could legitimately carry the reserved field id on an Int32 column — that's a data-validity problem, not an internal invariant violation, so I'd use ErrorKind::DataInvalid here (and {} rather than {:?}, since DataType implements Display).

The PrimitiveLiteral::Long guard just below is genuinely internal wiring, so Unexpected is fine there.

laskoviymishka · 2026-08-11T08:04:01Z

+            ));
+        };
+        let scalar = Int64Array::new_scalar(*seq);
+        let mask = is_not_null(source)?;


The coalesce tests all use mixed [Some(5), None, Some(8)], which would still pass if this mask polarity were flipped to is_null. An all-null [None, None, None] case (expecting [fallback, fallback, fallback]) would lock this line down — it's the only input that distinguishes is_not_null from is_null.

An all-non-null case is worth adding too, to confirm the REE cast produces a valid array when nothing falls back.

Added both.

laskoviymishka · 2026-08-11T08:04:01Z

+        let result = transformer.process_record_batch(batch).unwrap();
+
+        // Per-row value where non-null; the fallback (9) where null.
+        let seq_col = cast(result.column(1), &DataType::Int64).unwrap();


Nit: these assertions index by result.column(1), so they'd silently test the wrong column if projection order ever shifts. result.column_by_name(RESERVED_COL_NAME_LAST_UPDATED_SEQUENCE_NUMBER) would be sturdier — same for assert_last_updated_seq_column if it also goes by position.

Switched 6 tests to a shared helper that looks up column by name.

anoopj · 2026-08-12T06:03:59Z

Thanks for the through review!

anoopj added 2 commits August 10, 2026 08:43

fix: correct typo flagged by lint check

356baa7

laskoviymishka requested changes Aug 11, 2026

View reviewed changes

anoopj added 2 commits August 11, 2026 22:18

refactor: address review on coalesce _last_updated_sequence_number

e77d58a

test: index all _last_updated_sequence_number transformer tests by name

2910302

anoopj mentioned this pull request Aug 12, 2026

_last_updated_sequence_number: physical per-row values discarded when first_row_id is null #2991

Open

anoopj requested a review from laskoviymishka August 12, 2026 06:04

Uh oh!

Conversation

anoopj commented Aug 10, 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!

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!

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!

anoopj Aug 12, 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!

anoopj commented Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

anoopj commented Aug 10, 2026 •

edited

Loading

anoopj Aug 12, 2026 •

edited

Loading