feat(dataset): per-fragment column writes that survive compaction - #8313
Conversation
a58f118 to
2ca6e3f
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The field-level replacement shape is appropriate, but the public staging boundary does not yet preserve schema, snapshot, or row-lineage contracts.
A viable revision should validate each staged recursive schema against the dataset, bind the staged read version to commit, and publish logical recomputations through a path that advances stable-row-ID update metadata.
| ); | ||
|
|
||
| let writer = self.dataset.object_store.create(&path).await?; | ||
| let mut file_writer = file_versions::create_writer( |
There was a problem hiding this comment.
write_column trusts a caller-provided Schema even when its field IDs already belong to different dataset types. That lets the staged file violate the manifest schema while Dataset::validate still succeeds; scans then decode bytes with the wrong logical type. Validate the full recursive schema against existing dataset fields before writing, and repeat that validation at commit.
Executed regression
I added reproduce_fragment_write_column_accepts_mismatched_existing_schema beside the existing fragment-write tests and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-main-target cargo test --locked -p lance reproduce_ -- --nocapture
The test creates a Float32 value field, stages Int32 [10, 20] under the same field ID, commits, calls validate(), and scans. Expected: staging or commit rejects the mismatched field schema. Observed: commit and validation succeed, and the scan returns Float32 values f32::from_bits(10) and f32::from_bits(20).
| schema.clone(), | ||
| FileWriterOptions::default(), | ||
| )?; | ||
| file_writer.add_schema_metadata( |
There was a problem hiding this comment.
This metadata is written but never enforced, so a caller can present a newer commit read version and publish bytes prepared from an older snapshot. That permits a stale recomputation to overwrite a completed replacement. Make the staged result snapshot-bound: read and validate the recorded version and target backing state during commit, requiring recomputation after a relevant conflict.
Executed regression
I added reproduce_staged_read_version_is_not_enforced_at_commit and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-main-target cargo test --locked -p lance reproduce_ -- --nocapture
At version 1 the test stages stale values [30, 40] and winner values [50, 60]. It commits the winner as version 2, then commits the stale group while passing Some(2) to Dataset::commit. Expected: reject the stale file because its footer records version 1. Observed: the second commit succeeds and the scan returns [30, 40].
| base_id: None, | ||
| }; | ||
|
|
||
| Ok(super::transaction::DataReplacementGroup( |
There was a problem hiding this comment.
Returning only the fragment ID and data file discards the logical-update witness. Operation::DataReplacement does not advance stable-row-ID last_updated_at_version metadata, so delta reads can omit rows whose values this API changed. Route recomputation through the shared update/Merge machinery that records matched offsets, or extend this staged result and commit path to update row-lineage metadata atomically.
Executed regression
I added reproduce_fragment_write_column_does_not_advance_row_lineage and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-main-target cargo test --locked -p lance reproduce_ -- --nocapture
The test creates a version-1 dataset with stable row IDs, replaces both value rows, and commits version 2. Expected: _row_last_updated_at_version is [2, 2]. Observed: the values change, but the lineage column remains [1, 1].
2ca6e3f to
e3a3208
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The primitive type mismatch is fixed and snapshot responsibility now matches Dataset::commit’s low-level contract. Two acceptance contracts remain: staged field identity can still diverge from the current manifest schema, and logical recomputation is still invisible to stable-row lineage.
A viable revision should validate complete recursive field identity at staging and against the commit-time manifest, then publish recomputations through a path that advances _row_last_updated_at_version.
| let Some(existing) = dataset_schema.field_by_id(field.id) else { | ||
| continue; | ||
| }; | ||
| if existing.data_type() != field.data_type() { |
There was a problem hiding this comment.
data_type() does not establish that this is the same dataset field: it omits this field’s nullability and other Lance identity (recursive ID/path and logical metadata), and the comparison only uses the staging snapshot. As a result, nullable staged data is accepted for a non-null field, and a staged nullable file can also be committed after a concurrent Project tightens the manifest. Both produce a committed dataset that validate() accepts but scanning fails with unmasked nulls for non-nullable. Validate complete recursive field identity here and again against the commit-time manifest, or make schema projections conflict with this replacement.
Executed regressions
I added current-head cases beside the fragment write_column tests and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-main-target cargo test --locked -p lance reproduce_fragment_write_column -- --nocapture
One case created a non-null Int32 dataset field and staged nullable values containing a null. The other staged valid nullable data, concurrently projected that field to non-null, then committed the replacement. Expected: staging or commit rejects the incompatible field. Observed in both cases: commit and validate() succeed; scanning fails with unmasked nulls for non-nullable.
| base_id: None, | ||
| }; | ||
|
|
||
| Ok(super::transaction::DataReplacementGroup( |
There was a problem hiding this comment.
Returning only the fragment ID and data file discards the logical-update witness. Operation::DataReplacement does not advance stable-row-ID last_updated_at_version metadata, so delta reads can omit rows whose values this API changed. Route recomputation through the shared Update/Merge lineage machinery that records matched offsets, or extend this staged result and commit path to update row-lineage metadata atomically.
Executed regression
I added a current-head stable-row-ID case beside the fragment write_column tests and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-main-target cargo test --locked -p lance reproduce_fragment_write_column -- --nocapture
The test created a version-1 dataset with stable row IDs, replaced value from [1, 2] to [30, 40], and committed version 2. Expected: _row_last_updated_at_version becomes [2, 2]. Observed: the values change, but the lineage column remains [1, 1].
e3a3208 to
531263d
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The new field/nullability checks and symmetric Project conflict resolve the previous schema-race cases. Two correctness contracts remain: the staged schema still ignores storage-semantic layout metadata, and column recomputation still does not advance stable-row lineage.
A viable revision should derive or validate the writer schema against every layout-defining field attribute, then carry affected row offsets through the replacement commit so _row_last_updated_at_version advances atomically.
| // `validate` and fail later at scan. | ||
| let compare_options = SchemaCompareOptions { | ||
| compare_field_ids: true, | ||
| ..Default::default() |
There was a problem hiding this comment.
Default::default() leaves compare_metadata false. That is unsafe at this file-writing boundary: lance-encoding:packed is field metadata, but it changes physical field coverage. Removing that marker while preserving names, IDs, types, nullability, and children is accepted; the writer then advertises child fields [1, 2] instead of the packed parent [0], so DataReplacement can classify the file as disjoint/all-null coverage rather than replacing the packed field. Validate every layout-defining attribute here, or derive the writer schema from the matched dataset field.
Executed regression
I added reproduce_fragment_write_column_rejects_packed_metadata_mismatch on this head and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-verify-531-target cargo test --locked -p lance reproduce_fragment_write_column_rejects_packed_metadata_mismatch -- --nocapture
The test stages an otherwise-identical V2.1 packed struct after removing only lance-encoding:packed. Expected: write_column rejects the schema. Observed: the assertion result.is_err() fails because it returns Ok(DataReplacementGroup(... DataFile { fields: [1, 2], column_indices: [0, 1], ... })).
| base_id: None, | ||
| }; | ||
|
|
||
| Ok(super::transaction::DataReplacementGroup( |
There was a problem hiding this comment.
Returning only the fragment ID and data file still discards the logical-update witness. The DataReplacement manifest path changes files and indices but never advances stable-row-ID last_updated_at_version, so delta reads can omit rows whose values this API recomputed. Route this through the shared Update/Merge lineage machinery, or carry affected offsets in the staged result and update lineage atomically at commit.
Executed regression
I added reproduce_fragment_write_column_advances_row_lineage on this head and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-verify-531-target cargo test --locked -p lance reproduce_fragment_write_column_advances_row_lineage -- --nocapture
The test creates a version-1 dataset with stable row IDs, writes [30, 40] through write_column, and commits DataReplacement as version 2. Expected: _row_last_updated_at_version is [2, 2]. Observed: the assertion fails because it remains [1, 1].
531263d to
6069021
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The staging checks now cover type, nullability, and row count, but three correctness contracts remain: layout-defining metadata is ignored, schema-changing Project can rebase across a staged replacement, and logical recomputation does not advance stable-row lineage.
A viable revision should normalize the writer schema to the manifest’s physical layout, conflict schema evolution with staged replacements, and carry affected row offsets into atomic lineage updates.
| // `validate` and fail later at scan. | ||
| let compare_options = SchemaCompareOptions { | ||
| compare_field_ids: true, | ||
| ..Default::default() |
There was a problem hiding this comment.
Default::default() leaves compare_metadata false. That is unsafe at this file-writing boundary: lance-encoding:packed is metadata, but it changes physical field coverage. Removing only that marker from an otherwise identical packed struct is accepted, and the staged DataFile advertises child coverage instead of the packed parent, so DataReplacement can classify it as disjoint/all-null coverage rather than replacing the field. Validate every layout-defining attribute here, or derive the writer schema from the matched dataset field.
Executed regression
I added reproduce_fragment_write_column_rejects_packed_metadata_mismatch on this head and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-delta-60690216-target cargo test --locked -p lance reproduce_fragment_write_column_rejects_packed_metadata_mismatch -- --nocapture
Expected: write_column rejects the V2.1 schema after only lance-encoding:packed is removed. Observed: the assertion fails because it returns Ok(DataReplacementGroup(... DataFile { fields: [1], column_indices: [0], ... })).
| &self, | ||
| data: impl Stream<Item = Result<RecordBatch>> + Send, | ||
| schema: &Schema, | ||
| ) -> Result<super::transaction::DataReplacementGroup> { |
There was a problem hiding this comment.
This staged result has no protection from concurrent schema evolution, while the conflict resolver treats Project and DataReplacement as compatible in both directions. Even with the correct read version, a nullable file can therefore commit after Project makes the manifest field non-nullable, leaving a dataset that cannot be scanned. Make these operations conflict symmetrically, or revalidate the staged field against the commit-time manifest.
Executed regression
I added reproduce_data_replacement_conflicts_with_projected_nullability on this head and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-delta-60690216-target cargo test --locked -p lance reproduce_data_replacement_conflicts_with_projected_nullability -- --nocapture
The test stages nullable data at version 1, commits a valid nullability-tightening Project as version 2, then commits the replacement using read version 1. Expected: Error::RetryableCommitConflict. Observed: version 3 commits, then scanning fails with Found unmasked nulls for non-nullable StructArray field "value".
| base_id: None, | ||
| }; | ||
|
|
||
| Ok(super::transaction::DataReplacementGroup( |
There was a problem hiding this comment.
Returning only the fragment ID and data file discards the logical-update witness. The DataReplacement manifest path changes files and indices but never advances stable-row-ID last_updated_at_version, so delta reads can omit rows whose values this API recomputed. Route this through the shared Update/Merge lineage machinery, or carry affected offsets in the staged result and update lineage atomically at commit.
Executed regression
I added reproduce_fragment_write_column_advances_row_lineage on this head and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-delta-60690216-target cargo test --locked -p lance reproduce_fragment_write_column_advances_row_lineage -- --nocapture
The test creates a version-1 dataset with stable row IDs, writes [30, 40] through write_column, and commits DataReplacement as version 2. Expected: _row_last_updated_at_version is [2, 2]. Observed: the assertion fails because it remains [1, 1].
e9fdc57 to
e21601b
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The new row-lineage coverage is sound, but two schema-safety contracts still fail: layout-defining field metadata is ignored at staging, and schema-changing Project commits remain compatible with staged DataReplacement.
A viable revision should derive or validate the staged writer schema against the manifest’s physical layout and either conflict Project/DataReplacement symmetrically or revalidate staged fields against the commit-time manifest.
| // `validate` and fail later at scan. | ||
| let compare_options = SchemaCompareOptions { | ||
| compare_field_ids: true, | ||
| ..Default::default() |
There was a problem hiding this comment.
Default::default() leaves compare_metadata false, so this check accepts a schema after the layout-defining lance-encoding:packed marker is removed. The writer then derives different physical field coverage from an otherwise identical field, so a replacement can be classified against the wrong layout. Derive the writer schema from the matched manifest field, or validate every layout-defining attribute before writing.
Executed regression
On exact head e21601bd4f4bd925b46d93f964678c62fbd30137 I added gate_repro_write_column_rejects_packed_metadata_mismatch beside the existing fragment-write tests and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-main-target cargo test -p lance gate_repro_write_column_rejects_packed_metadata_mismatch -- --nocapture
The test creates a packed struct, removes only lance-encoding:packed while preserving IDs, types, children, and nullability, then expects write_column to reject it. Observed: the command exits 101 because write_column returns Ok and the rejection assertion fails.
| &self, | ||
| data: impl Stream<Item = Result<RecordBatch>> + Send, | ||
| schema: &Schema, | ||
| ) -> Result<super::transaction::DataReplacementGroup> { |
There was a problem hiding this comment.
This staged result is not bound to the commit-time schema, while the conflict resolver treats Project and DataReplacement as compatible in both directions. A nullable file can therefore commit after Project makes the manifest field non-nullable, leaving a committed dataset that cannot be scanned. Make these operations conflict symmetrically, or revalidate the staged field against the final manifest during commit.
Executed regression
On exact head e21601bd4f4bd925b46d93f964678c62fbd30137 I added gate_repro_data_replacement_conflicts_with_nullability_project and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-main-target cargo test -p lance gate_repro_data_replacement_conflicts_with_nullability_project -- --nocapture
The test stages [10, null] at version 1, commits a valid nullability-tightening Project at version 2, then commits the replacement using read version 1. Expected: a retryable conflict. Observed: the replacement commits, and scanning fails with Found unmasked nulls for non-nullable StructArray field "value".
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The packed-layout regression is fixed, but three independent schema-integrity failures remain across the staging and commit boundaries.
A viable revision should establish one effective schema and validate the requested field tree, every incoming batch, and the commit-time manifest against it.
| &self, | ||
| data: impl Stream<Item = Result<RecordBatch>> + Send, | ||
| schema: &Schema, | ||
| ) -> Result<super::transaction::DataReplacementGroup> { |
There was a problem hiding this comment.
This staged result carries no commit-time schema witness, while the conflict resolver treats Project and DataReplacement as compatible in both directions. Even with the correct read version, a nullable replacement can commit after Project makes the field non-nullable, producing a committed dataset that cannot be scanned. Make these operations conflict symmetrically, or revalidate every staged field and its physical layout against the live manifest during commit.
Executed regression
On exact head 15a79a1c04e37291934e157cd4d1b29786ec02d0 I added probe_data_replacement_rebases_across_nullability_project beside the existing fragment-write tests and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate-8313-verify-epo2jl/target cargo test -p lance probe_data_replacement_rebases_across_nullability_project --lib -- --nocapture
The test stages [Some(10), None] at version 1, commits a valid nullability-tightening Project as version 2, then commits the replacement using read version 1. Expected: a retryable conflict. Observed: version 3 commits, then scan fails with Found unmasked nulls for non-nullable StructArray field "value".
| let mut data = std::pin::pin!(data); | ||
| while let Some(batch_result) = data.next().await { | ||
| let batch = batch_result?; | ||
| file_writer.write_batch(&batch).await?; |
There was a problem hiding this comment.
Each batch is forwarded without recursively validating its Arrow schema against the effective writer schema. Top-level columns are selected by name, but nested struct encoders consume children positionally, so same-typed reordered children are silently written under the wrong field IDs. Validate or normalize every batch recursively before writing, and apply nullability checks through that same name/path mapping.
Executed regression
On exact head 15a79a1c04e37291934e157cd4d1b29786ec02d0 I added probe_write_column_reordered_struct_children and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate-8313-verify-epo2jl/target cargo test -p lance probe_write_column_reordered_struct_children --lib -- --nocapture
The manifest and declared schema are point{x, y}, while the incoming batch is point{y, x} with y values [300, 400] and x values [30, 40]. Expected: reject the batch-schema mismatch. Observed: staging and commit succeed; scan returns x=[300, 400] and y=[30, 40], proving the values were swapped.
| /// field the manifest has never seen is genuinely new and keeps the caller's | ||
| /// definition, but its children are still resolved the same way. | ||
| fn field_with_manifest_layout(field: &Field, dataset_schema: &Schema) -> Field { | ||
| if let Some(existing) = dataset_schema.field_by_id(field.id) { |
There was a problem hiding this comment.
This global ID lookup discards the requested field ancestry. Validation compares known nodes in isolation, so an unknown parent can contain a manifest-known child ID; this helper then clones the old field path and the returned file advertises coverage for the existing field instead of the requested tree. Validate the requested schema as a whole before resolving layout, including every known ID's manifest ancestry, and reject known descendants beneath unknown parents.
Executed regression
On exact head 15a79a1c04e37291934e157cd4d1b29786ec02d0 I added probe_write_column_unknown_parent_known_child and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate-8313-verify-epo2jl/target cargo test -p lance probe_write_column_unknown_parent_known_child --lib -- --nocapture
The test wraps the manifest root field value (ID 0) beneath a new wrapper field and stages [10, 20]. Expected: reject the wrong-path reuse. Observed: write_column returns file coverage [0]; committing it silently replaces root value, and scan returns [10, 20].
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The wrong-path fix and struct reordering are sound, but the public write boundary still violates three independent contracts: commit-time schema safety, lossless batch validation, and current Blob V2 preparation.
A viable revision should carry an exact manifest projection into commit and revalidate it on retries, validate each batch recursively before any name-based normalization, and stage through the versioned writer path so physical encodings receive their required preprocessing.
| &self, | ||
| data: impl Stream<Item = Result<RecordBatch>> + Send, | ||
| schema: &Schema, | ||
| ) -> Result<super::transaction::DataReplacementGroup> { |
There was a problem hiding this comment.
This result is validated only against the staging snapshot, while Project and DataReplacement remain compatible in both transaction directions. A nullable file staged at version 1 can therefore commit after version 2 tightens that field to non-nullable, producing a committed dataset that cannot be scanned. Either conflict these operations symmetrically, or carry the exact recursive target schema in the staged transaction and revalidate it against the live manifest on every build/retry.
Executed regression
On exact head 46a0ab104cc2228c8462237cc47a12a915b421a8, I added gate_repro_data_replacement_rejects_concurrent_nullability_project beside the fragment-write tests and ran:
RUST_BACKTRACE=0 CARGO_TARGET_DIR=/home/agent/tmp/gate8313-current-target cargo test --locked -p lance gate_repro_ --lib -- --nocapture
The case stages [10, null] at version 1, commits a valid non-null Project as version 2, then commits the replacement using read version 1. Expected: reject the replacement. Observed: version 3 commits, then scanning fails with Found unmasked nulls for non-nullable StructArray field "value".
| // matches by name at every level of the tree, and rejects a batch | ||
| // that is missing a column, types one differently, or supplies nulls | ||
| // for a field the manifest declares non-nullable. | ||
| let batch = match batch.project_by_schema(&batch_schema) { |
There was a problem hiding this comment.
project_by_schema is a projector, not a complete schema validator. For a fixed-size-list mismatch it rebuilds the source values with the target dimension, silently changing the batch row count; wrong container kinds can also panic in its as_struct/as_list downcasts. Validate every incoming Arrow schema recursively—including container kind, list dimension, and unambiguous field names—before applying a fallible name-based reorder.
Executed regression
On exact head 46a0ab104cc2228c8462237cc47a12a915b421a8, I added gate_repro_write_column_rejects_fixed_size_list_reshape and ran:
RUST_BACKTRACE=0 CARGO_TARGET_DIR=/home/agent/tmp/gate8313-current-target cargo test --locked -p lance gate_repro_ --lib -- --nocapture
The fragment has four FixedSizeList<Int32, 2> rows; the incoming batch has two FixedSizeList<Int32, 4> rows over the same eight values. Expected: reject the batch-schema mismatch. Observed: projection reshapes it into four size-two rows, the final row-count check passes, and write_column returns Ok.
| ); | ||
|
|
||
| let writer = self.dataset.object_store.create(&path).await?; | ||
| let mut file_writer = file_versions::create_writer( |
There was a problem hiding this comment.
This raw current-format writer bypasses Blob V2 preprocessing. V2.2/V2.3's structural encoder accepts only the prepared blob layout, while normal dataset writes convert the manifest's logical data, uri layout through BlobPreprocessor. Route this staging path through the same versioned writer/preprocessor boundary so blob sidecars and descriptors are prepared consistently.
Executed regression
On exact head 46a0ab104cc2228c8462237cc47a12a915b421a8, I added gate_repro_write_column_supports_blob_v2 and ran:
RUST_BACKTRACE=0 CARGO_TARGET_DIR=/home/agent/tmp/gate8313-current-target cargo test --locked -p lance gate_repro_ --lib -- --nocapture
The case creates a V2.3 dataset with a normal logical Blob V2 column and stages two replacement values through write_column. Expected: staging succeeds. Observed: write_batch returns Blob v2 encoder expected prepared array layout, got logical layout.
46a0ab1 to
ddc1ca9
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The fragment-level replacement shape is sound, and the rebased nullability barrier closes the prior schema race. The remaining staging path still violates batch-shape, versioned-writer, and legacy-format boundaries.
A viable revision should validate recursive batch shape before reordering, route staging through the existing per-version update writer, and keep wider-file tombstoning confined to formats whose readers support it.
| // matches by name at every level of the tree, and rejects a batch | ||
| // that is missing a column, types one differently, or supplies nulls | ||
| // for a field the manifest declares non-nullable. | ||
| let batch = match batch.project_by_schema(&batch_schema) { |
There was a problem hiding this comment.
project_by_schema is a lossy projector, not exact validation: it silently drops fields absent from the target schema, and its target-directed nested downcasts panic for the wrong source container. At this public input boundary, validate a unique, exact recursive source shape before projecting only to reorder equivalent structures, so malformed batches return InvalidInput instead of being changed or panicking.
Reproducer
On exact head ddc1ca905d7bfda5b28a9ad2af9352cf5d03386a, I added two focused tests in an isolated worktree and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-target-impl cargo test -p lance repro_ -- --nocapture
One passed columns a and unexpected unrequested against requested schema {a}. Expected: reject the extra field. Observed: write_column returned Ok, so the rejection assertion failed. The other passed an Int32 column s against target Struct{s.x}. Expected: return an input error. Observed: panic struct array from lance_arrow::project_array at lance-arrow/src/lib.rs:815.
| ); | ||
|
|
||
| let writer = self.dataset.object_store.create(&path).await?; | ||
| let mut file_writer = file_versions::create_writer( |
There was a problem hiding this comment.
This raw writer bypasses the versioned update-writer boundary and therefore skips Blob V2 preprocessing. V2.2/V2.3 structural encoders accept only the prepared layout, while callers provide the logical Blob array. Route staging through versions::open_update_writer (and use its returned DataFile) so blob sidecars and descriptors are prepared consistently.
Reproducer
On exact head ddc1ca905d7bfda5b28a9ad2af9352cf5d03386a, I added a V2.2 logical-Blob replacement test in an isolated worktree and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-target-impl cargo test -p lance repro_ -- --nocapture
Expected: staging the replacement succeeds. Observed: write_column returned InvalidInput: Blob v2 encoder expected prepared array layout, got logical layout.
| .file_version() | ||
| .expect("Expected valid file version"); | ||
| new_frag.files.push(new_file.clone()); | ||
| } else if !replaced_in_place |
There was a problem hiding this comment.
This new subset branch also runs for legacy V1, but the V1 reader derives its page-table minimum from the first metadata field. Tombstoning the lowest field changes [0, 1] to [-2, 1] without rewriting the physical page table, so the untouched sibling is decoded from the wrong page. Keep wider-file tombstoning confined to V2; preserve the legacy exact-match behavior or fully rewrite the V1 file.
Reproducer
On exact head ddc1ca905d7bfda5b28a9ad2af9352cf5d03386a, I added a legacy-format case in an isolated worktree and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-target-impl cargo test -p lance repro_ -- --nocapture
The file starts with a=[1,2], b=[10,20]; the test replaces only a with [30,40]. Expected after commit: a=[30,40], b=[10,20]. Observed: validation returned Ok, metadata was fields: [-2, 1], and scan returned a=[30,40], b=[1,2].
f271886 to
7413b87
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The V2 replacement shape is sound and the prior Blob, legacy-format, and basic batch-shape failures are fixed, but the public staging and retry boundaries still do not preserve their input and snapshot invariants.
A viable revision should make recursive validation fully fallible, preserve or conflict writes committed after the staging snapshot, and apply the existing virtual-column reservation at this write boundary.
| match actual.iter().find(|a| a.name() == expected_field.name()) { | ||
| None => Some(format!("column '{name}' is missing")), | ||
| Some(actual_field) => { | ||
| explain_type_difference(actual_field.data_type(), expected_field.data_type(), &name) |
There was a problem hiding this comment.
This comparison drops the nested Arrow Field nullability and keeps only its DataType. A nullable list item containing a null therefore passes validation; project_by_schema then constructs the manifest's non-null list and panics instead of returning InvalidInput. Compare recursive fields, including nullability, and keep normalization fallible so malformed public input cannot unwind the process.
Reproducer
On exact head 7413b8713cec37224b128d45bfbe8276bd90574f, I added repro_rejects_nullable_list_items_without_panicking beside these tests. It creates a dataset whose List<Int32> item is non-null, then stages a batch whose nullable item contains [10, null, 30, 40].
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-impl-2I5HBz/target cargo test --locked -p lance repro_rejects_nullable_list_items_without_panicking --lib -- --nocapture
Expected: write_column returns an input error. Observed: the command exits 101 with Non-nullable field of ListArray "item" cannot contain nulls, reached through project_by_schema.
| new_frag | ||
| .files | ||
| .retain(|file| file.fields.iter().any(|&f| f != TOMBSTONE_FIELD_ID)); | ||
| new_frag.files.push(new_file.clone()); |
There was a problem hiding this comment.
After this subset replacement is appended, manifest building unconditionally tombstones every overlapping overlay, including one committed after the replacement's read version. The conflict resolver explicitly allows this rebase because the newer overlay should win, so the current path silently discards an already committed value. Preserve overlays whose committed_version is newer than the transaction snapshot, or make that overlap a retryable conflict.
Reproducer
On exact head 7413b8713cec37224b128d45bfbe8276bd90574f, I added repro_rebased_replacement_preserves_newer_overlay. A V2 fragment starts with one wider [id, value] file; the test stages value=[30,40] at version 1, commits an overlay setting row 0 to 99 at version 2, then honestly commits the staged replacement from version 1.
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-impl-2I5HBz/target cargo test --locked -p lance repro_rebased_replacement_preserves_newer_overlay --lib -- --nocapture
Expected: the overlay survives and the effective values are [99,40]. Observed: the replacement commits, but overlays.len() is 0 instead of 1; the command exits 101.
| field.name | ||
| ))); | ||
| } | ||
| writer_fields.push(field.clone()); |
There was a problem hiding this comment.
The new-field branch accepts reserved virtual-column names. A caller can stage and publish a stored _rowid; the scanner then injects its synthetic _rowid beside it and projection fails with a duplicate field. Reject every top-level lance_core::is_system_column name before opening the writer, matching the ordinary insert boundary.
Reproducer
On exact head 7413b8713cec37224b128d45bfbe8276bd90574f, I added repro_rejects_reserved_system_name. It stages _rowid: UInt64 under a fresh ID, commits the returned DataReplacement, then commits a Project containing that field.
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-impl-2I5HBz/target cargo test --locked -p lance repro_rejects_reserved_system_name --lib -- --nocapture
Expected: staging rejects the reserved name. Observed: both commits succeed, then scan.project(&["_rowid"]) fails with Duplicate field name "_rowid" in schema; the command exits 101.
7413b87 to
f406798
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The three prior snapshot, system-column, and nested-list failures are fixed, but the new recursive null check rejects valid Arrow values beneath a nullable parent.
A viable revision should validate only logically visible nulls by carrying ancestor validity through nested containers, while retaining the current fallible rejection for genuinely unmasked nulls.
| /// the data rather than the batch's declared nullability, which is routinely | ||
| /// looser than the manifest's and carries no nulls to justify rejecting. | ||
| fn explain_null_violation(array: &dyn Array, expected: &ArrowField, path: &str) -> Option<String> { | ||
| if !expected.is_nullable() && array.null_count() > 0 { |
There was a problem hiding this comment.
This raw null_count loses the parent validity mask. A nullable struct may legally carry a null placeholder in a non-nullable child where the struct itself is null; project_by_schema accepts that input, but write_column now rejects it. This prevents valid nullable-struct columns from being staged. Carry ancestor validity through recursive validation (including fixed-size-list mask expansion) and reject only unmasked child nulls.
Reproducer
On exact head f406798ba5232485e3787045f0a3882afff7502a, I added repro_accepts_child_null_masked_by_struct_parent beside the fragment-write tests. It builds [null, {x: 20}] as a nullable struct with required child x, first confirms that Arrow projection to the dataset schema succeeds, and then stages it.
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-frag-target cargo test -p lance repro_accepts_child_null_masked_by_struct_parent -- --nocapture
Expected: staging succeeds. Observed: the test exits 101 because write_column returns InvalidInput: column 'v.x' contains nulls but the dataset defines it as non-nullable.
c399528 to
48c42ee
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The new visibility propagation fixes recursive validation, but it does not make accepted masked-null batches consumable by the structural-writing stage, so valid nullable-struct columns still cannot be staged.
A viable revision should preserve ancestor visibility through every downstream nullability check, or normalize hidden placeholders before projection and writing, and cover the public write_column path end to end.
| && visible | ||
| .iter() | ||
| .enumerate() | ||
| .any(|(row, seen)| *seen && array.is_null(row)) |
There was a problem hiding this comment.
Skipping null placeholders hidden by an ancestor is correct, but this visibility exists only inside validation; the unchanged batch continues to projection and the versioned writer. StructuralWriter::verify_field_nullability later checks each child null_count() without the ancestor mask, so the same valid [null, {x: 20}] input still fails write_column. Carry visibility into the downstream check or normalize hidden slots before projection and writing.
Executed regression
On exact head 48c42eea5ac36de911620b31c21b3e376455c812, I added repro_accepts_child_null_masked_by_struct_parent beside the fragment write_column tests. It first confirms project_by_schema accepts a nullable struct [null, {x: 20}] with required child x, then stages that batch.
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-mask-target cargo test -p lance repro_accepts_child_null_masked_by_struct_parent -- --nocapture
Expected: staging succeeds. Observed: the command exits 101 because write_column returns InvalidInput: The field x contained null values even though the field is marked non-null in the schema at rust/lance-file/src/writer/structural.rs:451. The deeper nullable-ancestor case fails identically, while the unmasked-null control is rejected at this validator as intended.
48c42ee to
16281a3
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The V2 compaction replacement mechanism is coherent, but the public staging path still violates six independent contracts across schema binding, recursive shape and nullability, format alignment, and failed-write cleanup.
A viable revision should enforce one end-to-end staging contract: bind every staged field to the commit-time schema, reject ambiguous trees, make nested null checks visibility-aware for every supported container, preserve each file version’s batch layout, and clean every physical artifact on failure.
| DataType::List(item) => DataType::List(relax(item)), | ||
| DataType::LargeList(item) => DataType::LargeList(relax(item)), | ||
| DataType::FixedSizeList(item, width) => DataType::FixedSizeList(relax(item), *width), | ||
| other => other.clone(), |
There was a problem hiding this comment.
relax_nullability stops before Map, so valid Map data is rejected solely because the incoming value field declares looser nullability. Add Map-aware recursion and projection while preserving the required entries/key invariants, so actual values—not a looser Arrow declaration—decide acceptance.
Executed regression
On exact head 16281a38c7264e2a58fbe3162154d0f4106ee6e2, I added gate_map_looser_declared_nullability_with_valid_data_is_accepted and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-null-schema-0CatjW/target cargo test -p lance gate_ --no-fail-fast -- --nocapture
The V2.2 target Map has a non-nullable values child; the incoming Map declares that child nullable but contains only [10, 20]. Expected: staging succeeds. Observed: the test exits 101 because write_column returns Incorrect datatype ... expected Map(... values: non-null Int32) got Map(... values: Int32).
| /// Remove a staged file that will not be returned. Best effort: it is | ||
| /// unreachable either way, and must not mask the error that caused it. | ||
| async fn discard_staged_file(&self, path: &Path) { | ||
| if let Err(delete_error) = self.dataset.object_store.delete(path).await { |
There was a problem hiding this comment.
Deleting only the staged .lance path leaves Blob V2 sidecars under data/<file-stem>/, so a routine row-count or write failure can leak arbitrarily large unreferenced objects. Use a failure guard that removes the main file and its sidecar directory on every exit, including stream and finish() errors, matching the existing orphan cleanup contract.
Executed regression
On exact head 16281a38c7264e2a58fbe3162154d0f4106ee6e2, I added probe_blob_row_mismatch_cleans_sidecars and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-impl-riwaOz/target cargo test --locked -p lance probe_ --lib -- --nocapture
The test stages three 70 KiB blobs against a two-row V2.2 fragment. Expected: the row-count error leaves no staged artifacts. Observed: the .lance file is deleted, but data/<staged-key>/10000000000000000000000000000000.blob remains.
| }; | ||
|
|
||
| let mut data = std::pin::pin!(data); | ||
| while let Some(batch_result) = data.next().await { |
There was a problem hiding this comment.
Forwarding caller batch boundaries unchanged can commit a V1 replacement whose batch count differs from sibling files, after which the fragment is unreadable. Rechunk V1 input to the existing reader batch layout, as the legacy update path does, or reject V1 before staging.
Executed regression
On exact head 16281a38c7264e2a58fbe3162154d0f4106ee6e2, I added probe_legacy_write_column_preserves_fragment_batch_alignment and ran:
RUST_BACKTRACE=0 CARGO_TARGET_DIR=/home/agent/tmp/gate8313-impl-riwaOz/target cargo test --locked -p lance probe_legacy_write_column_preserves_fragment_batch_alignment --lib -- --nocapture
A four-row V1 fragment has one four-row batch; the replacement arrives as two two-row batches. Expected: staging preserves a readable fragment or rejects the layout. Observed: staging and commit succeed, then scan fails with InvalidInput: Cannot create FragmentReader from data files with different number of batches.
| &writer_schema, | ||
| &SchemaCompareOptions { | ||
| compare_nullability: NullabilityComparison::Ignore, | ||
| ignore_field_order: true, |
There was a problem hiding this comment.
This order-insensitive comparison does not establish the documented exact tree: duplicate nested sibling names are collapsed during compatibility checking, and projection silently chooses the first child. Reject duplicate names recursively before any name-based reorder.
Executed regression
On exact head 16281a38c7264e2a58fbe3162154d0f4106ee6e2, I added gate_duplicate_nested_name_is_rejected and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-null-schema-0CatjW/target cargo test -p lance gate_ --no-fail-fast -- --nocapture
The input is Struct{x=[10,20], x=[30,40]} against target Struct{x}. Expected: staging rejects the ambiguous tree. Observed: staging and commit succeed, and readback silently keeps the first child [10, 20].
| }; | ||
| // The writer applies the manifest's nullability rule to the data; | ||
| // a batch it turns down leaves a file nothing will ever reference. | ||
| if let Err(err) = writer.write(std::slice::from_ref(&batch)).await { |
There was a problem hiding this comment.
The writer still checks each required child with its raw null_count() and does not carry ancestor visibility, so valid placeholders beneath null parents are rejected. Propagate validity and container spans through writer-side validation, or normalize hidden slots before writing, while retaining rejection for visible nulls.
Executed regression
On exact head 16281a38c7264e2a58fbe3162154d0f4106ee6e2, I added gate_masked_required_nested_value_is_accepted and ran:
RUST_BACKTRACE=0 CARGO_TARGET_DIR=/home/agent/tmp/gate8313-null-schema-0CatjW/target cargo test -p lance --lib gate_masked_required_nested_value_is_accepted -- --nocapture
Expected: masked required descendants stage successfully. Observed: all four Struct, List, LargeList, and V2.2 FixedSizeList cases fail at lance-file/src/writer/structural.rs:451 with required-child null errors. The corresponding visible-null List/LargeList controls are still rejected correctly.
| ))); | ||
| } | ||
|
|
||
| Ok(super::transaction::DataReplacementGroup( |
There was a problem hiding this comment.
This result carries no schema witness, so DataReplacement can append a field absent from the live manifest and commit an invalid fragment; a compatible concurrent Project drop reaches the same state for a field that was valid when staged. Reject fields missing from the commit-time schema and conflict field-removing projections, or carry schema evolution in the transaction and apply it atomically.
Executed regressions
On exact head 16281a38c7264e2a58fbe3162154d0f4106ee6e2, I added and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-replacement-dLetU2/target cargo test -p lance --lib gate_new_field_replacement_commits_invisibly -- --nocapture
CARGO_TARGET_DIR=/home/agent/tmp/gate8313-replacement-dLetU2/target cargo test -p lance --lib gate_replacement_rebased_over_drop_commits_invalid_manifest -- --nocapture
In the first case, a fresh field stages and commits but remains absent from the schema. In the second, a staged existing field rebases over a concurrent drop and commits. Expected: atomic schema evolution or rejection/retry. Observed in both: commit succeeds and validate() returns CorruptFile: ... did not have any fields in common with the dataset schema.
16281a3 to
0790f20
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The compaction replacement shape remains sound, and the new V1, duplicate-batch, and basic Blob cleanup fixes close their targeted failures. The public staging boundary still loses schema, nested Arrow, and artifact-ownership invariants between validation, writing, and commit.
A viable revision should make the staged result carry every commit-time invariant it depends on, normalize each supported nested type losslessly before writing, and own all staged artifacts through one failure path.
| ))); | ||
| } | ||
|
|
||
| Ok(super::transaction::DataReplacementGroup( |
There was a problem hiding this comment.
This result still drops the schema witness required at commit. A fresh field commits while absent from the manifest, and a staged existing field can rebase over a concurrent Project drop because DataReplacement and Project remain compatible; both produce a fragment whose file has no field in the live schema. Either reject undeclared fields and conflict field-removing projections, or carry the exact recursive field projection in DataReplacementGroup and revalidate it against the commit-time manifest.
Reproducer
On exact head 0790f20d108bc94836e225043eccbebdcdcf65e1, I added repro_fresh_field_commits_invalid_manifest and repro_project_drop_races_replacement_into_invalid_manifest, then ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate-target-a cargo test -p lance --lib dataset::tests::fragment_write_column -- --nocapture
Expected: either staging or commit rejects the schema-absent field. Observed: both commits succeeded; the live schema lacked the field and validate() failed because the fragment file referenced no live schema field.
| }; | ||
| // The writer applies the manifest's nullability rule to the data; | ||
| // a batch it turns down leaves a file nothing will ever reference. | ||
| if let Err(err) = writer.write(std::slice::from_ref(&batch)).await { |
There was a problem hiding this comment.
The relaxed projection schema does not change the manifest schema held by StructuralWriter, whose recursive check still uses each child's raw null_count(). A required child null hidden by a null nullable parent is therefore rejected even though that is a valid Arrow value. Carry ancestor visibility through writer validation, or normalize hidden placeholders before this call while retaining rejection for visible nulls.
Reproducer
On exact head 0790f20d108bc94836e225043eccbebdcdcf65e1, I added repro_masked_required_child_is_rejected and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate-target-a cargo test -p lance --lib dataset::tests::fragment_write_column -- --nocapture
The staged nullable struct was [{required: 10}, null]; its hidden child slot was null while the manifest child was required. Expected: staging succeeds. Observed: write_column returned the writer's marked non-null error.
| DataType::Map(entries, sorted) => match entries.data_type() { | ||
| DataType::Struct(kv) if kv.len() == 2 => { | ||
| let value = Arc::new(relax_nullability(&kv[1])); | ||
| let entries = ArrowField::new( |
There was a problem hiding this comment.
Rebuilding the Map entries field with ArrowField::new drops entries.metadata(), which Lance preserves as part of the supported schema. The outer projection then rejects an otherwise valid Map because its array still carries that metadata. Preserve the entries field metadata while relaxing only the value nullability.
Reproducer
On exact head 0790f20d108bc94836e225043eccbebdcdcf65e1, I added repro_map_entries_metadata_projection and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate-target-b cargo test -p lance --lib dataset::tests::fragment_write_column::repro_map_entries_metadata_projection -- --exact --nocapture
The manifest and incoming Map both had entry-semantic=kept on the entries field. Expected: staging succeeds. Observed: projection returned Incorrect datatype: the expected Map had no entries metadata while the incoming Map retained it.
| self.discard_staged_file(&staged_path).await; | ||
| return Err(self.schema_mismatch(mismatch)); | ||
| } | ||
| let batch = match batch.project_by_schema(&projection_schema) { |
There was a problem hiding this comment.
This projection is documented as name-based at every level, but lance_arrow::project_array has no DataType::Map arm. Compatibility accepts a Map value Struct<b, a> against manifest Struct<a, b>, then this call rejects it instead of reordering by name. Rebuild MapArray recursively through its entries/value field while preserving offsets, validity, and the required key/entries invariants.
Reproducer
On exact head 0790f20d108bc94836e225043eccbebdcdcf65e1, I added repro_map_struct_value_reordered_by_name and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate-target-b cargo test -p lance --lib dataset::tests::fragment_write_column::repro_map_struct_value_reordered_by_name -- --exact --nocapture
Expected: the Map value children are reordered and staging succeeds. Observed: projection returned Incorrect datatype, reporting manifest Struct(a, b) versus incoming Struct(b, a).
|
|
||
| let mut data = std::pin::pin!(data); | ||
| while let Some(batch_result) = data.next().await { | ||
| let batch = batch_result?; |
There was a problem hiding this comment.
This ? exits after prior writes without calling discard_staged_file; writer.finish().await? has the same gap. Once a Blob pack rolls and finalizes, a later stream error leaves that sidecar orphaned. Put the writer/path behind one failure cleanup path, drop the writer before deletion, and reuse the shared data/sidecar cleanup so stream, write, finish, and validation errors have the same ownership behavior.
Reproducer
On exact head 0790f20d108bc94836e225043eccbebdcdcf65e1, I added repro_discards_blob_sidecars_on_stream_error and ran:
CARGO_TARGET_DIR=/home/agent/tmp/gate-target-b cargo test -p lance --lib dataset::tests::fragment_write_column::repro_discards_blob_sidecars_on_stream_error -- --exact --nocapture
With a 128 KiB Blob pack threshold, the stream yielded one successful two-blob batch and then a synthetic error. Expected: the artifact count remains 3. Observed: it became 4; the main .lance count stayed 1 while Blob sidecars increased from 2 to 3.
0790f20 to
619654a
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
Five prior findings are fixed; one new correctness issue remains. The new writer-side nullability walk rejects logically valid slices of variable-length containers because it treats retained child values outside the visible offset range as part of the batch.
Keep the fast path only when the offset domain spans the entire child array; otherwise derive child visibility from the slice's offsets before validating required descendants.
| offsets: &OffsetBuffer<O>, | ||
| values_len: usize, | ||
| ) -> Option<BooleanBuffer> { | ||
| if hidden.is_none() && nulls.is_none_or(|n| n.null_count() == 0) { |
There was a problem hiding this comment.
offsets_hidden returns None whenever the parent has no null bitmap, but sliced List, LargeList, and Map arrays retain child values outside their visible offset range. The recursive check therefore counts an unreachable child null and rejects a logically valid batch. Only take this fast path when the visible offsets span the entire child array; otherwise mark values outside that domain hidden.
Executed regression
On exact head 619654a764ffef894a57abf7591246555a6fae6e, I added test_accepts_sliced_list_with_unreachable_item_null. A V2.1 dataset has one List<item: Int32 non-null> row; the staged input is a two-row list [[null, 20], [30, 40]] sliced to its second row, with no list validity buffer. The only visible values are therefore [30, 40].
CARGO_TARGET_DIR=/home/agent/tmp/gate-target-619-arrow cargo test -p lance test_accepts_sliced_list_with_unreachable_item_null -- --nocapture
Expected: staging succeeds. Observed: staging returns InvalidInput: The field item contained null values even though the field is marked non-null in the schema. Focused writer tests reproduced the same failure for sliced List, LargeList, and Map.
There was a problem hiding this comment.
Fixed on 0bd855e497ab791e8f4c34341e85a6622dd954d6: the raw fast path now requires offsets to cover the complete retained child, while sliced domains mark out-of-range values hidden. The public sliced-List write/commit/readback regression passes, as do focused List, LargeList, and Map validator cases.
619654a to
0bd855e
Compare
There was a problem hiding this comment.
The sliced-container correctness blocker is fixed: the fast path now requires full child coverage, and the public staging regression passes.
The remaining risk is resource usage. Validating a tiny zero-copy variable-length slice allocates visibility for the entire retained child buffer. Compacting sliced inputs mitigates it; constraining validation to the referenced child domain would remove the worst-case overhead.
| if spans_child && hidden.is_none() && nulls.is_none_or(|n| n.null_count() == 0) { | ||
| return None; | ||
| } | ||
| let mut builder = BooleanBufferBuilder::new(values_len); |
There was a problem hiding this comment.
values_len is the retained child-array length, so a tiny zero-copy slice allocates and initializes a bitmap for every original child value. The mask costs ceil(N / 8) bytes; at the i32 List offset limit it is about 256 MiB, and Boolean children can roughly double their buffer footprint. Correctness is preserved, but staging memory scales with unreachable data. Consider slicing the child to [first_offset, last_offset) before recursion and, when parent masking is needed, building the bitmap relative to that domain.
There was a problem hiding this comment.
Fixed on 3d22895: offset-mapped validation now slices the child to the referenced offset window and builds any visibility bitmap relative to that window, so a tiny zero-copy slice no longer allocates over the retained child. The focused writer tests (4/4) and the public sliced-List write/commit/readback regression pass.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
0bd855e to
3d22895
Compare
3d22895 to
30b93f8
Compare
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The rebase preserves the reviewed behavior. File-version selection now forwards the manifest's exact concrete version, while DataReplacement and conflict handling continue to preserve legacy rejection, newer overlays, and invalidation bookkeeping.
westonpace
left a comment
There was a problem hiding this comment.
Plenty of nits but I don't actually see anything that would need to block this PR from advancing.
We could probably update the description to include all the things the PR is achieving:
- Adds a fragment write_columns path (already documented)
- Improves nullability detection to allow for masked nulls
- Adds auto-write projection for map arrays
- Changes conflict detection to consider a column deletion to conflict with a data replacement of that column
- Adds support for data replacement operations that replace a subset of an existing data file
In the future, it will be easier to review if these are included as separate PRs or stacked PRs.
| // The 2.0 logical encoders reject a physically-null child slot even when | ||
| // a null ancestor masks it, so the raw null count is the correct gate | ||
| // here. The 2.1+ structural writer, whose repdef encoding stores such | ||
| // slots as the ancestor's null, checks only visible nulls instead. |
There was a problem hiding this comment.
What is this comment saying? It doesn't seem to be describing the method.
| //! Write-time nullability validation that counts only nulls a reader can | ||
| //! observe. A null beneath a null ancestor is not a value of the field: the | ||
| //! ancestor masks the slot, its contents are unconstrained, and the repdef | ||
| //! encoding records it as the ancestor's null. Rejecting it would turn away | ||
| //! valid Arrow -- including batches produced by scanning this same dataset. |
There was a problem hiding this comment.
This whole file seems unrelated to the goal of the PR.
There was a problem hiding this comment.
I'll split this; will address separately. This came out of gatekeeper's review of this patch, but it is equivalent to #7844. I don't think anything will break badly if it is separate.
| map_arr.nulls().cloned(), | ||
| *sorted, | ||
| )?)) | ||
| } |
There was a problem hiding this comment.
Grumpy nit: this seems unrelated to the goal of the PR
| return false; | ||
| } | ||
| let Some(nulls) = array.nulls() else { | ||
| return true; |
There was a problem hiding this comment.
What if the entire array were already hidden by this point? Wouldn't we want to return false in that case? Or does this method never get reached in that case?
| if array.null_count() == 0 { | ||
| return false; | ||
| } | ||
| let Some(nulls) = array.nulls() else { |
There was a problem hiding this comment.
Old rambling rant nit: it irritates me that this method is called nulls and not validity but that ship has long since sailed.
| /// children may arrive in any order, but a batch not describing exactly | ||
| /// that tree is rejected. |
There was a problem hiding this comment.
By "describing exactly that tree" you mean "having all the same children"?
| /// deletion vector is applied on the way in. Batches are pulled one at a | ||
| /// time, so the full column need not be held in memory. | ||
| /// | ||
| /// Staging does not bind the result to the version it was computed from. |
There was a problem hiding this comment.
What does this sentence mean?
| /// Concurrent replacements of the same field conflict at commit, but only | ||
| /// for a caller that commits with the version it actually read; a caller | ||
| /// that supplies a newer one publishes stale values unchallenged. |
There was a problem hiding this comment.
I'd maybe rephrase this as a constraint.
/// Callers should take care to set the read version correctly. If
/// this is not done then multiple replacements to the same field will
/// not be detected as a conflict.
| for field in &schema.fields { | ||
| // The per-field identity check cannot see the request naming an | ||
| // id twice, and the set-based batch comparison downstream would | ||
| // match one batch column against both copies. | ||
| if !requested.insert(field.id) { | ||
| return Err(Error::invalid_input(format!( | ||
| "column data for fragment {} names field id {} ('{}') more than once", | ||
| self.id(), | ||
| field.id, | ||
| field.name | ||
| ))); | ||
| } | ||
| // The scanner injects these itself; a stored copy collides with it | ||
| // at projection time. Same boundary the ordinary insert path draws. | ||
| if lance_core::is_system_column(&field.name) { | ||
| return Err(Error::invalid_input(format!( | ||
| "column data for fragment {} names reserved column '{}'", | ||
| self.id(), | ||
| field.name | ||
| ))); | ||
| } | ||
| let Some(existing) = dataset_schema | ||
| .fields | ||
| .iter() | ||
| .find(|existing| existing.id == field.id) | ||
| else { | ||
| // The commit path publishes data files, never schema, so a | ||
| // field the manifest does not define would commit as a file no | ||
| // live field answers for -- and a concurrent schema change | ||
| // could never be checked against it. | ||
| return Err(Error::invalid_input(format!( | ||
| "column data for fragment {} names field id {} ('{}') that the dataset schema \ | ||
| does not define; declare the column with add_columns before staging its data", | ||
| self.id(), | ||
| field.id, | ||
| field.name | ||
| ))); | ||
| }; | ||
| // `explain_difference` recurses, covering the whole subtree. | ||
| if let Some(difference) = field.explain_difference(existing, &compare_options) { | ||
| return Err(Error::invalid_input(format!( | ||
| "column data for fragment {} does not match dataset field id {}: {}", | ||
| self.id(), | ||
| field.id, | ||
| difference | ||
| ))); | ||
| } | ||
| writer_fields.push(existing.clone()); | ||
| } |
There was a problem hiding this comment.
Minor nit: These rules feel like something we should already have in the code base. For example, in the normal write path. It would be nice if we could reuse them in a helper function instead of defining them twice.
| } | ||
|
|
||
| #[track_caller] | ||
| fn data_replacement_field_removed_err( |
There was a problem hiding this comment.
Does this really need to be an error? Could the data-replacement of the column just silently become a no-op?
There was a problem hiding this comment.
I think this should be consistent with what happens if you append data and it conflicts with a column getting dropped. I think that is a conflict.
This adds FileFragment::write_column, which stages new data for one of a fragment's declared columns as a standalone data file and returns its DataReplacementGroup without committing it. add_columns can only append a new field; write_column stages a file that answers for a field the dataset already defines, so a caller can recompute a column instead of only adding one. Staging validates full field identity against the manifest -- the commit path publishes data files, never schema, so a file staged for an undeclared field could never commit validly -- and matches batches to the manifest tree by name at every level. On 2.1+ it accepts nulls hidden beneath a null ancestor, which the repdef encoding stores as the ancestor's null; 2.0's encoders cannot store them, so that writer still rejects them up front. Every staging failure -- a bad batch, a stream error, a write or finish error, a row-count mismatch -- discards the staged file and any blob sidecars through one exit path. It also extends DataReplacement to handle one more layout. Today DataReplacement swaps a file when the field sets match exactly, or appends a file when the fragment does not cover the fields at all, and rejects everything else. That rejection covers the layout a long-lived column actually reaches: compaction folds the column into a shared base file, no file's field set matches a single-column replacement any more, and nothing can replace the column again. Where the replaced fields all sit inside one wider file, DataReplacement now tombstones them in place and appends the new file to answer for them; a file left answering for no live schema field is dropped rather than carried as an unreachable reference. A concurrent projection that drops a replaced field now conflicts at commit rather than rebasing the replacement into a manifest where its file answers for no live field. Two pieces land in shared machinery because staging reuses it: batch projection (lance-arrow) learns to rebuild MapArray recursively, where it previously fell through to a clone and rejected any by-name reordering inside a map, and the 2.1+ structural writer's nullability check walks ancestor visibility instead of raw null counts. This supersedes lance-format#8207, which proposed a Dataset-level staging and commit protocol for the same goal. Review there argued for building on the fragment-level API instead.
30b93f8 to
e2544b8
Compare
…iles Staging a per-fragment column write required one data file to cover every replaced field. Compaction decides that layout, so a multi-column write was refused whenever it had scattered those columns -- a shape the caller does not choose -- and the error blamed schema and file version mismatch. The tombstone loop already walked every file, so only the guard was narrow. It now requires each replaced field to be covered, and covered only by files that can be tombstoned. V1 is excluded per field rather than per file: its reader derives page table offsets from the first field in the metadata, so a tombstone there leaves the siblings decoding from the wrong pages.
A mask is only ever recorded as `Some`, so a fully masked parent reaches the visibility scan rather than the bail-out, and reports no visible null. That was untested, and the two read the same from the call site.
Both write paths that reject a stored system column carried their own copy of why, and the predicate they share said only what the columns are. The rationale moves to the predicate and the call sites drop it.
Relaxing the writer to count only observable nulls changed validation for every 2.1 write, not just staged columns, and lance-format#7844 already carries that fix with production evidence behind it. Staging keeps the strict rule the writer has always applied, and the tests that asserted the relaxed shapes go with it.
`FileFragment::write_column` accepts a schema and record-batch stream that can contain multiple columns, so the singular name misrepresents the API contract. Rename it to `write_columns` and update its Rust callers and tests. The API was introduced by #8313 and has not appeared in a release tag, so this intentionally does not retain a deprecated alias.
This adds FileFragment::write_column, which stages new data for one
fragment's column as a standalone data file and returns its
DataReplacementGroup without committing it. add_columns only appends a new
field; write_column may stage a file that answers for a field the fragment
already has, so a caller can recompute a column instead of only adding one.
It also extends DataReplacement to handle one more layout. Today
DataReplacement swaps a file when the field sets match exactly, or appends a
file when the fragment does not cover the fields at all, and rejects
everything else. That rejection covers the layout a long-lived column
actually reaches: compaction folds the column into a shared base file, no
file's field set matches a single-column replacement any more, and nothing
can replace the column again. Where the replaced fields all sit inside one
wider file, DataReplacement now tombstones them in place and appends the new
file to answer for them.
Three supporting changes fall out of those two, each needed before a
recomputed column round-trips:
beneath a null ancestor is the ancestor's null rather than a value of the
field, so rejecting it turned away valid Arrow, including batches produced
by scanning this same dataset. 2.0 keeps the strict raw-null check its
logical encoders require.
declared more loosely than the target is judged on its data rather than on
its declaration.
that field. Rebased over the drop, the staged file would answer for a field
no live schema defines, and a retry cannot help because the field is gone.
This supersedes #8207, which proposed a Dataset-level staging and commit
protocol for the same goal. Review there argued for building on the
fragment-level API instead.
That review also proposed committing the collected fragments through
Operation::Merge. DataReplacement is used instead because a Merge carries
the whole fragment list and schema, so the conflict resolver has to assume
it touched everything: it conflicts with every concurrent operation, and
each retry recomputes the column from scratch, which gets worse the longer
the refresh runs. A DataReplacement declares exactly what changed -- this
file now answers for these fields in this fragment -- so the resolver
tolerates concurrent appends outright and conflicts only on genuine
(fragment, field) overlap, which is the granularity a per-fragment column
write needs.