feat(mem_wal): support delete against non-nullable base columns - #8352
Open
hamersaw wants to merge 3 commits into
Open
feat(mem_wal): support delete against non-nullable base columns#8352hamersaw wants to merge 3 commits into
hamersaw wants to merge 3 commits into
Conversation
A tombstone carries the primary key and null in every other column, so `delete` previously required every non-PK column to be nullable in the base table — pushing a storage-engine detail into the user's schema. Split the shard's schema in two. The *logical* schema is the base table's, exactly as the caller declared it; the *storage* schema widens every non-PK top-level field to nullable and is what the memtable, WAL entries, and SSTables physically carry. This mirrors the logical/physical split `SchemaAdapter` already applies to JSON and view types. Widening is top-level only: Arrow validates nullability just at the top level of a `RecordBatch`, so a null `FixedSizeList` or `Struct` needs no change below the top and a vector column's item field gains no validity layer. Primary keys are never widened, so `build_tombstone_batch` still rejects a null, mistyped, or missing key with no extra check. The contract is enforced at two boundaries: * Ingress — `put` validates caller input against the logical schema before the WAL append. This is now the only gate: append and `merge_insert` both compare schemas with `NullabilityComparison::Ignore`, and the encoder derives validity from the array rather than the field, so a null that got past here would reach the base table silently. Validating pre-append also keeps a rejected batch from wedging replay. WAL-only mode is covered too; it previously validated nothing. * Egress — the scan path narrows back to the logical schema once tombstone rows have been filtered out. `project_to_canonical` now actually emits its `target_schema` (DataFusion derives `ProjectionExec` nullability from its expressions, so it could not before), via a new `SchemaRelabelExec`. The same node widens arms so they agree before `UnionExec`/`CoalesceFirstExec`, which require exact schema equality. Narrowing doubles as the assertion that no tombstone escaped its filter. `ensure_tombstone_column` now always re-labels rather than returning a batch that already has the column unchanged, so an entry written under an older storage schema replays into the current one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cut the comments added by the logical/storage schema split roughly in half — keeping the why, dropping the restatement — and settle on one vocabulary for the pair. `logical schema` is the base table's, as the caller declared it; `storage schema` is the widened one the memtable, WAL, and SSTables carry. Renames follow: `relax_non_pk_nullability(logical_schema, ..)`, and the `target_schema` parameters of `ensure_tombstone_column` / `build_tombstone_batch` (both always receive the storage schema) plus their test locals. User-facing error text keeps "base table schema", which callers recognize. Two comments were stale rather than merely wordy: WAL replay no longer passes a `_tombstone`-carrying batch through unchanged, and `schema_with_tombstone` now produces the intermediate that gets widened, not the memtable schema itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
hamersaw
marked this pull request as ready for review
August 7, 2026 15:52
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
`item` is not used after the `FixedSizeList` is built, so the clone trips `clippy::redundant_clone` and fails the workspace clippy gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The change fixes the mismatch between the logical schema contract and tombstone storage at the right boundary. It keeps caller validation and scan output strict while widening only the physical non-key fields needed by delete markers, and it normalizes replay and mixed-source reads without changing a stable format.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A tombstone carries the primary key and null in every other column, so
ShardWriter::deleterequired every non-PK column to be nullable in the base table. That pushes a storage-engine detail into the user's schema for no reason the user can see.Approach
Split the shard's schema in two:
This mirrors the logical/physical split
SchemaAdapteralready applies to JSON and view types indataset/utils.rs, and rides the boundary that already exists —_tombstoneis a physical column the SSTable schema carries and the base table does not.Widening is top-level only. Arrow validates nullability just at the top level of a
RecordBatch, so a nullFixedSizeList/Structneeds no change below the top; a vector column's item field is untouched and gains no validity layer. Primary keys are never widened (Schema::unenforced_primary_keyrequires them non-nullable), sobuild_tombstone_batchstill rejects a null, mistyped, or missing key — the delete path needed no new validation.Where the contract is enforced
Ingress —
putvalidates against the logical schema before the WAL append.This is now the only gate. Both append (
write/insert.rs) andmerge_insertcompare schemas withNullabilityComparison::Ignore, and the encoder derives validity from the array rather than the field, so a null that got pastputwould land in a non-nullable base column silently. Validating pre-append also matters for a second reason: a batch that is appended and only then rejected fails identically on every replay, leaving the shard unable to reopen — the same hazard that putsvalidate_index_configsahead ofclaim_epoch.WAL-only mode is covered as well; it previously validated nothing at all.
Egress — the scan narrows back to the logical schema after tombstones are filtered.
project_to_canonicaldocumented that it emits itstarget_schemabut did not: DataFusion derivesProjectionExecnullability from its expressions, not from the requested schema. A newSchemaRelabelExecmakes that real. The same node widens arms so they agree beforeUnionExec/CoalesceFirstExec, both of which require exact schema equality —CoalesceFirstExec::newasserts it and would otherwise panic on the base-arm/WAL-arm nullability difference.The narrowing doubles as a runtime assertion: if a tombstone ever escaped its filter,
RecordBatch::try_newrejects the null instead of handing the caller a row of nulls.Ordering falls out of this —
carry_schemain the point-lookup path is built on the widened schema, because tombstones are still in flight untilfilter_tombstones_after_coalesce.vector_searchandfts_searchneeded no changes; they already route every arm throughproject_to_canonical.ensure_tombstone_columnnow always re-labels instead of passing through a batch that already has the column, so an entry written under an older storage schema replays into the current one.Tests
13 new tests; 575
mem_wallib tests pass.test_delete_against_non_nullable_base_column_round_trip— the headline: delete against a base table with a non-nullable non-PK column, survivors keep their values, and the scan reports the base table's own nullability.test_put_rejects_null_in_non_nullable_base_column/..._wal_only_...— the ingress gate in both modes.test_build_tombstone_batch_nulls_non_nullable_base_column/..._rejects_null_primary_key— widening works, PKs still strict.relax_*— top-level-only widening, nested fields untouched,_tombstonestays non-nullable, idempotence, metadata preserved (the PK marker rides on field metadata).schema_relabel::tests— widening, narrowing, narrowing rejects a surviving null, empty batches.Not yet run
Draft because these are outstanding, not because the change is incomplete:
cargo test -p lance --lib(~2700 tests) — only themem_walsubset has been run.cargo clippy --all --tests --benches -- -D warnings.merge_insert. The ingress gate is written as if it is load-bearing for base-table integrity, which is the safe assumption and what the code reading indicates, but it is unverified.cargo fmt --allhas been run.🤖 Generated with Claude Code