fix(dataset)!: conflict a NOT NULL tightening with concurrent writes

[wkalt]

alter_columns proves a column holds no nulls by scanning, then commits the
change as a Project, which conflicts with nothing. A write racing the scan
can land nulls in the conflict window, leaving data that fails to read
under the tightened schema.

Project and Merge now carry preserves_nullability, asserting the operation
makes no nullability-affecting schema change. An operation without the
assertion conflicts with any concurrent value-write in either commit
order, and the loser retries against the new state. False is the
no-assertion default, so a transaction from a writer that predates the
field can only over-conflict. alter_columns asserts it for a rename and
drop_columns for a drop; a tightening withholds it.

A merge is included because it can introduce a field that stale data does
not supply: add_columns derives new-column nullability from the expression
or takes the stream schema verbatim, subcolumn additions merge new
children into existing structs, and a cast rewrites a column under a new
field id. An append is the one value-write that rebases across a committed
merge, and its fragments read as null for fields they predate. The
assertion derives from the schema delta: a non-nullable first-new node on
any path withholds it, while a nullable new node masks its whole subtree,
the same rule the AllNulls transform enforces at the top level. Any new
node under a non-nullable top-level column also withholds it, because the
reader synthesizes missing subcolumns against the column's declared
nullability and could not read the stale fragment at all. The cast path
withholds it when any recast field is non-nullable. The python and java
operations default to no assertion. Tightening and casting in the same
alter_columns call is rejected.

The check relies on read_version being the version the data was validated
against, the same contract every other conflict rule already depends on.

Marked breaking due to new fields on public enum variants.

[lance-gatekeeper]

❌ Gate recommendation: request changes.

The NOT NULL transition is a snapshot-validated constraint and needs a symmetric barrier: neither it nor a stale value-writing transaction may rebase across the other. The current revision still has independent false-negative paths under that shared contract.

A viable revision should derive tightening from the transaction's read-version schema, reject both commit orderings for every value-writing operation, and resolve written-field ancestry for structural files. Regression coverage should exercise both winners, latest-destination commits, and nested fields.

[lance-gatekeeper]

Append cannot use an empty base-nullability set here. Conflict checks dispatch on the transaction currently rebasing, so if a null append is built on nullable v1 but the tightening Project wins v2, check_append_txn accepts that Project and the append commits v3 under the non-null schema. Update and DataOverlay have the same reverse-order hole: their constructors also retain no read-schema nullability and their handlers accept Project. Later operation validation checks layout, not stored null counts.

Please make the tightening a symmetric barrier: retain the read-version schema context for every value-writing operation and reject an already-committed tightening Project in each corresponding handler.

Reproducer

I added this test beside the new integration test and ran cargo test -p lance repro_project_first_rejects_stale_null_append -- --nocapture:

#[tokio::test]
async fn repro_project_first_rejects_stale_null_append() {
    let batch = arrow_array::record_batch!(("value", Int32, [1, 2])).unwrap();
    let dataset = Arc::new(
        Dataset::write(
            RecordBatchIterator::new(vec![Ok(batch.clone())], batch.schema()),
            "memory://",
            None,
        )
        .await
        .unwrap(),
    );
    let read_version = dataset.version().version;
    let with_null = arrow_array::record_batch!(("value", Int32, [None, Some(3)])).unwrap();
    let append = InsertBuilder::new(dataset.clone())
        .with_params(&WriteParams {
            mode: WriteMode::Append,
            ..Default::default()
        })
        .execute_uncommitted(vec![with_null])
        .await
        .unwrap();

    let mut schema = dataset.schema().clone();
    schema.fields[0].nullable = false;
    let tightened = Dataset::commit(
        WriteDestination::Dataset(dataset),
        Operation::Project { schema },
        Some(read_version),
        None,
        None,
        Arc::new(Default::default()),
        false,
    )
    .await
    .unwrap();

    let result = Dataset::commit(
        WriteDestination::Dataset(Arc::new(tightened)),
        append.operation,
        Some(append.read_version),
        None,
        None,
        Arc::new(Default::default()),
        false,
    )
    .await;
    assert!(matches!(result, Err(Error::RetryableCommitConflict { .. })));
}

The assertion failed because the second commit returned Ok(Dataset { version: 3, ... }).

[lance-gatekeeper]

This records nullability from the destination dataset, which is not guaranteed to be the transaction's read-version snapshot. commit_transaction checks out read_version into its local dataset, but calls try_new(&original_dataset, ...); a URI or latest-dataset destination may therefore already contain the non-null v2 schema. A stale DataReplacement then gets an empty nullable set and this new Project check returns compatible.

Please source this comparison from the checked-out read-version schema (or pass that schema explicitly), so the result is independent of which destination version the caller supplied.

Reproducer

I added this focused test in the resolver module and ran cargo test -p lance repro_data_replacement_uses_read_version_schema -- --nocapture:

let dataset = nullable_test_dataset().await; // v1
let field_id = dataset.schema().fields[0].id;
let replacement = Transaction::new(
    1,
    Operation::DataReplacement {
        replacements: vec![DataReplacementGroup(
            0,
            DataFile::new_legacy_from_fields("replacement.lance", vec![field_id], None),
        )],
    },
    None,
);
let mut projected = dataset.schema().clone();
projected.fields[0].nullable = false;
let project = Transaction::new(1, Operation::Project { schema: projected }, None);
let mut latest = dataset;
latest
    .apply_commit(project.clone(), &Default::default(), &Default::default())
    .await
    .unwrap(); // latest is now non-null v2

let mut rebase = TransactionRebase::try_new(&latest, replacement, None)
    .await
    .unwrap();
assert!(matches!(
    rebase.check_txn(&project, 2),
    Err(Error::RetryableCommitConflict { .. })
));

The assertion failed: check_txn returned Ok(()).

[lance-gatekeeper]

Exact field-ID intersection does not describe what a replacement can change in structural files. V2.1 DataFile.fields records physical leaf/blob/packed IDs, not nullable non-leaf struct or list IDs. Replacing a leaf column can also replace the ancestor definition/nullability state, yet tightening that parent never intersects this set and is accepted.

Please expand replacement coverage through schema ancestry/atomic-field semantics before testing for tightening.

Reproducer

I added and ran cargo test -p lance repro_data_replacement_covers_struct_parent_nullability -- --nocapture. The test writes a nullable parent: struct<child: int32>, constructs a V2.1 replacement whose DataFile.fields contains only the child ID, then tightens the parent:

let parent_id = dataset.schema().fields[0].id;
let child_id = dataset.schema().fields[0].children[0].id;
let replacement = Transaction::new(
    dataset.version().version,
    Operation::DataReplacement {
        replacements: vec![DataReplacementGroup(
            0,
            DataFile::new(
                "replacement.lance",
                vec![child_id],
                vec![0],
                ConcreteFileVersion::V2_1,
                None,
                None,
            ),
        )],
    },
    None,
);
let mut projected = dataset.schema().clone();
projected.mut_field_by_id(parent_id).unwrap().nullable = false;
let project = Transaction::new(
    dataset.version().version,
    Operation::Project { schema: projected },
    None,
);
let mut rebase = TransactionRebase::try_new(&dataset, replacement, None)
    .await
    .unwrap();
assert!(matches!(
    rebase.check_txn(&project, dataset.version().version + 1),
    Err(Error::RetryableCommitConflict { .. })
));

The assertion failed: check_txn returned Ok(()).

[lance-gatekeeper]

❌ Gate recommendation: request changes.

Nullability tightening needs exact read-snapshot classification and complete logical coverage of every physical write. The current revision still has two independent gaps under that contract: an atomic structural field can bypass the barrier, while benign projections are rejected as if they were tightenings.

A viable revision should resolve the current transaction's read-version context whenever its projection classification is needed, and map each replacement's physical fields to the full logical field set it can supply, including descendants of atomically encoded composites. Add positive and negative controls for both commit orderings.

[lance-gatekeeper]

This expansion still misses the reverse structural relationship for atomically encoded fields. In V2.1 a packed struct is represented by one physical parent ID, but replacing that field supplies every child value. If a Project tightens a nullable child first, covered contains the parent and its ancestors but not that child, so the stale replacement is accepted and can put a null beneath the non-null schema.

Please expand an atomic composite ID through the logical descendants it supplies (or conservatively treat its whole subtree as covered), and retain the existing upward expansion for leaf-backed structures.

Reproducer

I added this test beside the resolver tests and ran cargo test -p lance repro_data_replacement_covers_packed_struct_children -- --nocapture:

#[tokio::test]
async fn repro_data_replacement_covers_packed_struct_children() {
    use arrow_array::{ArrayRef, StructArray};
    use arrow_schema::Fields;

    let child = Arc::new(Field::new("child", DataType::Int32, true));
    let mut parent = Field::new(
        "parent",
        DataType::Struct(Fields::from(vec![child.clone()])),
        false,
    );
    parent.set_metadata(HashMap::from([(
        "lance-encoding:packed".to_string(),
        "true".to_string(),
    )]));
    let arrow_schema = Arc::new(Schema::new(vec![parent]));
    let batch = RecordBatch::try_new(
        arrow_schema.clone(),
        vec![Arc::new(StructArray::from(vec![(
            child,
            Arc::new(Int32Array::from(vec![Some(1), Some(2)])) as ArrayRef,
        )]))],
    )
    .unwrap();
    let dataset = Dataset::write(
        arrow_array::RecordBatchIterator::new(vec![Ok(batch)], arrow_schema),
        "memory://",
        Some(WriteParams {
            data_storage_version: Some(LanceFileVersion::V2_1),
            ..Default::default()
        }),
    )
    .await
    .unwrap();

    let parent_id = dataset.schema().fields[0].id;
    let child_id = dataset.schema().fields[0].children[0].id;
    assert!(dataset.schema().fields[0].is_packed_struct());
    let replacement = Operation::DataReplacement {
        replacements: vec![DataReplacementGroup(
            0,
            DataFile::new(
                "replacement.lance",
                vec![parent_id],
                vec![0],
                ConcreteFileVersion::V2_1,
                None,
                None,
            ),
        )],
    };
    let mut projected = dataset.schema().clone();
    projected.mut_field_by_id(child_id).unwrap().nullable = false;

    let read_version = dataset.manifest.version;
    let mut rebase = TransactionRebase::try_new(
        &dataset,
        Transaction::new(read_version, replacement, None),
        None,
    )
    .await
    .unwrap();
    rebase.resolve_nullability_context(&dataset).await.unwrap();
    let result = rebase.check_txn(
        &Transaction::new(read_version, Operation::Project { schema: projected }, None),
        read_version + 1,
    );
    assert!(matches!(result, Err(Error::RetryableCommitConflict { .. })));
}

The assertion failed because check_txn returned Ok(()).

[lance-gatekeeper]

This lazy-load condition does not cover the case where the transaction being rebased is itself Project. With a rename, drop, or nullability relaxation built at v1 and an intervening Append/Update/replacement at v2, there is no other Project, so the context stays None; project_tightens_nullability treats that as true, and the ? below exits with a retryable conflict. This regresses previously compatible schema-only rebases and can starve them under ongoing writes.

Please also resolve the read-version context when the current operation is Project, while retaining the unknown-context conflict fallback for callers that genuinely skip resolution.

Reproducer

I added this test beside the new integration tests and ran cargo test -p lance repro_benign_project_remains_compatible_with_append -- --nocapture:

#[tokio::test]
async fn repro_benign_project_remains_compatible_with_append() {
    let batch = arrow_array::record_batch!(("value", Int32, [1, 2])).unwrap();
    let reader = RecordBatchIterator::new(vec![Ok(batch.clone())], batch.schema());
    let dataset = Dataset::write(reader, "memory://", None).await.unwrap();
    let read_version = dataset.version().version;

    let mut renamed = dataset.schema().clone();
    renamed.fields[0].name = "renamed".to_string();
    let appended = InsertBuilder::new(WriteDestination::Dataset(Arc::new(dataset)))
        .with_params(&WriteParams {
            mode: WriteMode::Append,
            ..Default::default()
        })
        .execute(vec![batch])
        .await
        .unwrap();

    let result = Dataset::commit(
        WriteDestination::Dataset(Arc::new(appended)),
        Operation::Project { schema: renamed },
        Some(read_version),
        None,
        None,
        Arc::new(Default::default()),
        false,
    )
    .await;
    assert!(result.is_ok());
}

The assertion failed with RetryableCommitConflict against the append at version 2.

[lance-gatekeeper]

✅ Gate recommendation: approve.

The revision now enforces nullability tightening symmetrically across value-writing operations while preserving benign schema-only rebases. Read-version classification comes from the transaction snapshot, and physical field coverage includes both structural ancestors and atomic-composite descendants.

[westonpace]

I definitely agree that a tightening projection should conflict with these operations.

I'm a little ambivalent on the rebasing. Is it something you need? First, it adds a fair amount of complexity. Second, our nullability enforcement probably isn't all that solid today. Although I think this is more existing bug than a problem with the new code.

Regardless of the ambivalence, the rebasing make the assumption that nullability is enforced based on the "commit dataset". This isn't true. If anything, the nullability would be based on the "read dataset".

Append is probably the easiest example. Consider the following :

import lance
from lance.fragment import write_fragments

data = ...
dataset_uri = ...

dataset = lance.dataset(dataset_uri)
read_version = dataset.version

# POINT A
# By passing dataset and not dataset_uri we should be using
# dataset's schema for nullability validation.
#
# Unrelated note: I'm not sure what
# we do for validation if only dataset_uri is passed.  It's
# possible we don't do any nullability validation in that case.
# It's also possible the write_fragments call opens the dataset
# as part of the call.
# However, that is a different, pre-existing, bug.
fragments = write_fragments(data, dataset)

# POINT B
# At commit time we can re-open the dataset.  This is common in
# the distributed write case.  A tightening
# projection may have already happened on a different worker.
dataset = lance.dataset(dataset_uri)
op = lance.LanceOperation.Append(fragments)
lance.LanceDataset.commit(
    dataset_uri,
    op
    read_version=read_version
)

Currently the rebasing logic is using the schema at point B to determine if tightening has happened since the data was written. It should be using the schema at point A instead.

[wkalt]

@westonpace thanks, I agree with your comment.

Is it something you need?

I can drop the rebasing and scope this down to just the conflict.

[lance-gatekeeper]

❌ Gate recommendation: request changes.

The metadata-based barrier is sound only when both the tightening claim and the write provenance are trustworthy. This revision still treats legacy persisted tightenings as non-claims, and supported Rust/Java staged-write paths can omit or misstate the snapshot where data was validated. Null data can therefore still commit under a NOT NULL schema.

Please preserve claim/provenance across persisted history and automatically carry the actual write snapshot through every supported staged-fragment producer.

Please mark this PR with the breaking-change label.

[lance-gatekeeper]

Persisted Projects written before this field decode an absent proto3 boolean as false. Some of those Projects tightened nullability, so the new barrier misclassifies them as rename/drop operations and lets stale value writes cross them. Please preserve presence (for example with optional bool) and safely classify absent legacy Projects from their read-version schema, or provide an equivalent compatibility mechanism.

Reproducer

I applied a tightening Project in its legacy representation (no claim), then committed a null append from the pre-tightening snapshot. The append committed successfully, and scanning the resulting dataset failed because null data was stored beneath the non-null schema. A disposable repro_legacy_tightening_without_claim_accepts_null_append test passed with this failure state asserted.

[lance-gatekeeper]

This conflates the reconciliation/read version with the snapshot where staged data was actually validated, and the builder offers no setter. A caller can write fragments at v1, reconcile at v2, then build at v2; written_against_version == read_version makes check_tightening_since_write return early even if v2 introduced NOT NULL. Please default unknown or expose explicit provenance, while having internal file-producing builders stamp the real write snapshot.

Reproducer

In a disposable variant of test_write_rejected_when_data_predates_tightening, I supplied null append data written at v1 but claimed v2. The commit succeeded, confirming that false builder provenance suppresses the historical-tightening check.

[lance-gatekeeper]

The manual field/setter does not protect Java's normal fragment workflow. WriteFragmentBuilder/JNI returns bare FragmentMetadata, and SourcedTransaction.Builder sets only readVersion, so fragment-write → append records 0 and check_tightening_since_write returns early. Please carry the write version with Java fragment metadata and derive it automatically for append transactions.

Reproducer

I temporarily added assertEquals(0, appendTxn.transaction().writtenAgainstVersion()) to SourcedTransactionTest#testSourcedTransaction. ./mvnw -Djava.io.tmpdir=/home/repo/java/target/jnitmp -Dtest=SourcedTransactionTest#testSourcedTransaction test passed, confirming the ordinary append path remains unstamped.

[lance-gatekeeper]

❌ Gate recommendation: request changes.

Historical provenance must reject any current required field that staged data was never validated against, including fields absent from the write snapshot. Treating a missing historical field like a nullable one preserves the NOT NULL contract without changing the PR's conservative operation-wide barrier.

[lance-gatekeeper]

This predicate ignores fields that did not exist in the write snapshot. A staged fragment can therefore predate a newly added field, then rebase after that field is tightened; modern fragment validation accepts the missing field and readers synthesize nulls, so the append commits under a NOT NULL schema. Treat None here as unsafe as well (missing or nullable), and cover it with the regression.

Reproducer

I added repro_stale_append_missing_new_non_null_field and ran cargo test -p lance repro_stale_append_missing_new_non_null_field -- --nocapture.

The test stages an append at v1, adds a populated nullable new_value at v2, tightens it to NOT NULL at v3, then commits the staged append with read_version = 3 and written_against_version = 1. It expects RetryableCommitConflict; current HEAD returned Ok(Dataset { ... version: 4 ... }).

[lance-gatekeeper]

❌ Gate recommendation: request changes.

The absent-field revision closes the previous gap. Write provenance still needs per-write ownership: return the resolved snapshot with each fragment result, or require exclusive builder use, so overlapping writes cannot exchange the schema version that the conflict barrier trusts.

[lance-gatekeeper]

resolved_written_against() reports one builder-wide most-recent value, while both public write entry points take &self. Two overlapping writes can therefore resolve different dataset snapshots and the first can read the second's version after it completes. Stamping the first fragment set with that newer version can make check_tightening_since_write return early even though those fragments were validated against the older schema. Bind provenance to each returned write result (or make stateful writes require &mut self) instead of a post-hoc shared accessor.

Reproducer

I added repro_concurrent_builder_misattributes_provenance and ran cargo test -p lance repro_concurrent_builder_misattributes_provenance -- --nocapture.

The test blocks write A after it resolves schema v1, advances the dataset to v2, completes write B through the same builder, and then releases A. A expects its resolved version to remain 1; current HEAD returned 2 (left: 2, right: 1).

[lance-gatekeeper]

❌ Gate recommendation: request changes.

The per-write result fixes overlapping builder reuse, but every staged-write wrapper must preserve that provenance. Thread the returned version through FileFragment::create* and the Python metadata conversion so documented distributed appends remain protected by the historical-tightening barrier.

[lance-gatekeeper]

Discarding the version here leaves Python LanceFragment.create(..., mode="append") unstamped. LanceDataset.commit then treats provenance as unknown, so a stale fragment containing nulls can commit after the column becomes NOT NULL. Return the version through this wrapper and stamp the exported FragmentMetadata; the same applies to create_fragments.

Reproducer

I added test_repro_lance_fragment_create_rejects_historical_tightening and ran uv run pytest python/tests/test_fragment.py::test_repro_lance_fragment_create_rejects_historical_tightening -q.

The test creates nullable v1 data, stages a null fragment through LanceFragment.create(..., mode="append"), tightens the field at v2, and expects the stale append to raise a historical conflict. Current HEAD committed successfully: Failed: DID NOT RAISE <class 'Exception'>.

[github-actions]

Important

This PR touches the Lance format specification.

Substantive changes to the format specification — the .proto definitions
and the spec docs under docs/src/format/ — require a PMC vote before merge.
Minor edits such as typo fixes, wording, or formatting are excluded; use your
judgment.

If this is a meaningful format change:

• Start a vote following the Lance community voting process.
  Format specification modifications need 3 binding +1 votes (excluding the
  proposer), held on GitHub Discussions, with a minimum voting period of 1 week.
• Once the vote passes, link the completed vote in this PR. It should not be
  merged until the vote is linked.

[westonpace]

I think written_against_version is overkill. Is there any reason we can't use read_version? Are there legitimate reasons for writers to set it later than the write point? Or just a fear that writers might do that?

I think read_version makes sense. Before you append to a dataset you must "read" the schema so you know how to validate the write. This schema capture should be consistent across writers and should serve as the basis for read_version.

If examples or callers are doing otherwise (I think they are) then we should fix that. But not as part of this PR.

[lance-gatekeeper]

✅ Gate recommendation: approve.

The write snapshot now stays attached to each staged result through the Rust wrapper and Python metadata conversion, closing the remaining distributed-append path while preserving per-write ownership and unknown-provenance semantics.

[lance-gatekeeper]

❌ Gate recommendation: request changes.

The Project fence works, but several remaining operation and binding paths can still commit data outside the schema snapshot that validated it.

A viable revision should preserve the tightening claim across every transaction representation, enforce stamped validation versions at every append commit entry point, and retain the minimum known stamp when a batch also contains unknown provenance.

[lance-gatekeeper]

This validation is confined to SourcedTransaction.Builder. The documented Transaction.Builder → CommitBuilder.execute path never reads FragmentMetadata.getWrittenAgainstVersion(), and CommitBuilder forwards the transaction unchanged. A coordinator reopened after tightening can therefore declare v2 for v1-stamped fragments and skip the claim in the conflict window. Move the derivation/rejection to a transaction or commit boundary shared by both Java entry points.

Reproducer run on e035b6b1ee69bde891bb8d7e8e4dea25793d6b3c

@Test
void reproDirectCommitBuilderRejectsStampedStaleAppend(@TempDir Path tempDir) {
  String path = tempDir.resolve("directCommit").toString();
  try (RootAllocator allocator = new RootAllocator(Long.MAX_VALUE)) {
    TestUtils.SimpleTestDataset source = new TestUtils.SimpleTestDataset(allocator, path);
    try (Dataset dataset = source.createEmptyDataset()) {
      FragmentMetadata fragment = source.createNewFragment(5); // stamped v1
      try (Transaction claim = new Transaction.Builder()
              .readVersion(dataset.version())
              .operation(Project.builder().schema(dataset.getSchema())
                  .assertsNonNull(true).build())
              .build();
          Dataset tightened = new CommitBuilder(dataset).execute(claim);
          Transaction stale = new Transaction.Builder()
              .readVersion(tightened.version()) // v2, later than the stamp
              .operation(Append.builder().fragments(List.of(fragment)).build())
              .build()) {
        assertThrows(IllegalArgumentException.class,
            () -> new CommitBuilder(tightened).execute(stale));
      }
    }
  }
}

JAVA_TOOL_OPTIONS=-Djava.io.tmpdir=/home/agent/tmp ./mvnw -Dtest=org.lance.SourcedTransactionTest#reproDirectCommitBuilderRejectsStampedStaleAppend test reported: expected IllegalArgumentException, but nothing was thrown.

[lance-gatekeeper]

Returning unknown as soon as one fragment is unstamped discards a trusted older stamp from another fragment. For [known v1, unknown], the caller can consequently declare v2 and bypass the v1 fragment's conflict window even though that fragment may contain nulls. Unknown provenance cannot make known provenance safe: retain the minimum known stamp, and return unknown only when no stamp is known. The Python all(s is not None ...) gate has the same issue.

Reproducer run on e035b6b1ee69bde891bb8d7e8e4dea25793d6b3c

@Test
void reproUnknownStampDoesNotEraseKnownStamp() throws Exception {
  FragmentMetadata known = new FragmentMetadata(0, List.of(), 0L, null, null);
  known.setWrittenAgainstVersion(1);
  FragmentMetadata unknown = new FragmentMetadata(1, List.of(), 0L, null, null);
  Append append = Append.builder().fragments(List.of(known, unknown)).build();

  Method method = SourcedTransaction.Builder.class.getDeclaredMethod(
      "stampedWriteVersion", Operation.class);
  method.setAccessible(true);
  assertEquals(1L, method.invoke(null, append));
}

JAVA_TOOL_OPTIONS=-Djava.io.tmpdir=/home/agent/tmp ./mvnw -Dtest=org.lance.SourcedTransactionTest#reproUnknownStampDoesNotEraseKnownStamp test observed 0 instead of the known minimum 1.

[lance-gatekeeper]

❌ Gate recommendation: request changes.

The explicit Project barrier works, but required fields can still be introduced through Merge, and an honest stale append is allowed to rebase across an already-committed Merge.

A viable revision should make required-field changes carried by Merge part of the same symmetric barrier—either carry a tightening claim on Merge or conservatively conflict stale appends with Merge—and add deterministic nested-field coverage.

[lance-gatekeeper]

❌ Gate recommendation: request changes.

The top-level Merge claim closes the previous blocker for required top-level additions and required-field casts, but nested subcolumn additions can still introduce a required child without setting the claim.

Derive the Merge claim from the old/new schema delta: if the first newly introduced node on a path is required beneath an existing ancestor, the merge must conflict with stale value writes. Preserve compatibility for a wholly new nullable container, and add deterministic V2.2 nested-subcolumn coverage.

[lance-gatekeeper]

This only checks top-level nullability, but V2.2 allows adding a child beneath an existing struct. Adding required s.b while the existing top-level s remains nullable leaves asserts_non_null false. A stale append can then commit a valid s value that omits b; the reader synthesizes a null child inside a valid struct and panics because b is non-nullable.

Classify the old/new schema delta instead: a required node first introduced beneath an existing supplied path must set the claim, while a wholly new nullable root remains safe even if its descendants are required.

Reproducer run on this head

I added this regression to dataset_merge_update.rs:

#[tokio::test]
async fn repro_nested_required_add_conflicts_with_stale_append() {
    let child_a = Arc::new(ArrowField::new("a", DataType::Int32, false));
    let schema = Arc::new(ArrowSchema::new(vec![ArrowField::new(
        "s",
        DataType::Struct(Fields::from(vec![child_a.as_ref().clone()])),
        true,
    )]));
    let initial = StructArray::from(vec![(
        child_a.clone(),
        Arc::new(Int32Array::from(vec![1, 2])) as ArrayRef,
    )]);
    let batch = RecordBatch::try_new(schema.clone(), vec![Arc::new(initial)]).unwrap();
    let mut dataset = Dataset::write(
        RecordBatchIterator::new(vec![Ok(batch)], schema.clone()),
        "memory://",
        Some(WriteParams {
            data_storage_version: Some(LanceFileVersion::V2_2),
            ..Default::default()
        }),
    )
    .await
    .unwrap();

    let stale = StructArray::from(vec![(
        child_a,
        Arc::new(Int32Array::from(vec![3])) as ArrayRef,
    )]);
    let stale_batch = RecordBatch::try_new(schema, vec![Arc::new(stale)]).unwrap();
    let append = InsertBuilder::new(WriteDestination::Dataset(Arc::new(dataset.clone())))
        .with_params(&WriteParams {
            mode: WriteMode::Append,
            ..Default::default()
        })
        .execute_uncommitted(vec![stale_batch])
        .await
        .unwrap();

    let child_b = Arc::new(ArrowField::new("b", DataType::Int32, false));
    let output_schema = Arc::new(ArrowSchema::new(vec![ArrowField::new(
        "s",
        DataType::Struct(Fields::from(vec![child_b.as_ref().clone()])),
        true,
    )]));
    let output_schema_ref = output_schema.clone();
    let mapper = move |batch: &RecordBatch| {
        let added = StructArray::from(vec![(
            child_b.clone(),
            Arc::new(Int32Array::from_iter_values(0..batch.num_rows() as i32)) as ArrayRef,
        )]);
        Ok(RecordBatch::try_new(
            output_schema_ref.clone(),
            vec![Arc::new(added)],
        )?)
    };
    dataset
        .add_columns(
            NewColumnTransform::BatchUDF(BatchUDF {
                mapper: Box::new(mapper),
                output_schema,
                result_checkpoint: None,
            }),
            None,
            None,
        )
        .await
        .unwrap();

    let committed = CommitBuilder::new(Arc::new(dataset))
        .execute(append)
        .await
        .unwrap();
    let scanned = committed.scan().try_into_batch().await.unwrap();
    let structs = scanned
        .column_by_name("s")
        .unwrap()
        .as_any()
        .downcast_ref::<StructArray>()
        .unwrap();
    let stale_row = structs.len() - 1;
    assert!(structs.is_valid(stale_row));
    assert!(structs.column_by_name("b").unwrap().is_null(stale_row));
}

Command:

CARGO_TARGET_DIR=/home/agent/tmp/gate-nested.KmTR3N/target cargo test -p lance repro_nested_required_add_conflicts_with_stale_append -- --nocapture

The stale append committed as version 3, then scanning panicked:

Found unmasked nulls for non-nullable StructArray field "b"

[lance-gatekeeper]

⚠️ Gate recommendation: approve with a non-blocking risk.

The old/new schema classifier closes the nested required-subcolumn race for materializing Merge paths while preserving safe nullable-container rebases.

The metadata-only AllNulls path still allows schema-based Python/Java add-columns to extend an existing nullable struct with a required child without writing child values, so a later scan can panic. This predates the revision; a follow-up should reject any AllNulls delta that asserts non-null (or materialize those values) and add binding coverage.

[westonpace]

I think we can move forward with this. It's pessimistic with the option for the user to say it's safe.

Though now you've touched a .proto file so it classifies as a spec change and needs a vote. I think this would quality as an experimental feature btw (see here). We haven't adopted the policy yet but if you want to chime in there it would be helpful.

As a minor nit I'd argue that asserts_non_null is a bit of an unintuitive name. I think the optimization is more that you'd "assert no nullability changes made". Maybe like assert_preserves_nullability or something like that. You'd have to invert the logic though. You wouldn't even need the optional at that point since the default (false) would be the safe "no assertion" case.