Field-level updates: stage only assigned columns in update mutations#342
Field-level updates: stage only assigned columns in update mutations#342ragnorc wants to merge 5 commits into
Conversation
Red (flip green with the partial-staging change): - scalar_indexes::update_of_unindexed_property_preserves_other_index_coverage (whole-row update merges mark every field modified, so Lance prunes the touched fragments from every index's fragment_bitmap) - writes::single_update_stages_partial_matched_only_source (a sole update should stage only key+assigned columns as a matched-only merge) Green pins (behavior that must not change): - writes::mixed_insert_update_same_table_stages_full_row_upsert - writes::chained_updates_same_table_stage_full_rows - writes::empty_match_update_stages_no_merge - validators::partial_update_completes_composite_unique_group Probe infra: MergeWriteProbes gains per-call MergeShape (source columns + unmatched-row disposition), recorded in stage_merge_insert.
A node table whose only op in a query is a single `update` now stages a PARTIAL merge source — (id + assigned + constraint-completion columns) — as a matched-only merge (WhenNotMatched::DoNothing). Lance's partial-schema path patches the provided columns in place on the same fragment, so: - the update never reads unassigned columns (the scan projects id + completion-minus-assigned; assignments are literal values) - write amplification drops from row-width to assigned-width - every index over unassigned columns keeps its fragment coverage (Lance prunes only fields_modified) Completion columns: every member of a @unique group intersecting the assigned set rides along, so composite-unique validation sees the whole tuple; the unique evaluator skips groups with no column present in a batch (untouched by the write) and stays loud on partially-present groups. Fallbacks (whole-row, behavior unchanged): mixed insert+update on one table, multiple updates on one table, and any non-eligible shape — partial and full batches cannot share one uniform-schema merge source, and a present column's null cell means "set NULL", so widening is unsound. Known residual (pinned as a tripwire in scalar_indexes): the merge source must carry the join key, and Lance's column patcher counts every source column as modified — so the id BTREE alone still loses patched fragments (parity with the whole-row path) until upstream excludes ON columns from column patches; every other index is preserved.
…ion/write docs lance_surface_guards gains partial_schema_merge_patches_in_place_and_prunes_only_modified_fields: a partial-source matched-only merge patches columns in place (same fragment set), leaves missing columns untouched, and prunes only indexes covering a source column — with the join-key index pruning pinned as the current residual (goes red when upstream excludes ON columns from column patches; tighten scalar_indexes.rs then). docs/user/mutations: updates write only assigned columns (cost follows the assignment, not row width); @embed staleness note. docs/dev/writes: partial-schema staging shape, completion-column rule, fallback matrix, evaluator group-skip, the id-BTREE residual.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…is (id + assigned) Review finding: @Unique-Group completion columns were included in the STAGED merge source, so Lance patched them (every source column counts as modified) and pruned their indexes even though their values were unchanged — e.g. updating only `room` of @unique(room, hour) degraded the index on `hour`. Completion columns are validation inputs, never merge inputs: the pending (validation) batch keeps (id + assigned + completion) so the evaluator sees the whole unique tuple, and the staged source is projected down to (id + assigned) before stage_merge_insert. PendingTable.partial_update becomes partial_stage_cols: Option<Vec<String>> carrying the projection. Regression test (red first with 'hour' Degraded): scalar_indexes::completion_column_index_survives_partial_update.
aaltshuler
left a comment
aaltshuler
left a comment
There was a problem hiding this comment.
Reviewed the diff plus the enclosing code at 985182d8, with every substrate claim re-verified against the pinned Lance 9.0.0-beta.15 checkout. The core mechanism is sound and well-defended: the surface guard exercises the exact production path (BTREE on the join key → legacy Merger, which at our pin locates keys by name, so there is no column-order hazard), change-feed visibility and read-your-writes were traced clean, and InsertAll→DoNothing is safe under the eligibility rule. Two correctness findings survive verification (one reproduced end-to-end at both commits), plus robustness/cleanup items — all inline below.
One finding with no inline anchor: docs/dev/testing.md is not updated in this PR (maintenance rule 1 — "update both the source code and the doc in the same change"). Four test files gained significant coverage (staging-shape tests in writes.rs, two index-coverage cells in scalar_indexes.rs, the composite-unique cell in validators.rs, the partial-merge surface guard) and the always-on test map describes none of it — the drift class 5c3125e7 recently existed to fix.
Verified and deliberately not flagged, to save you the re-derivation: the "partial staging rides the legacy indexed Merger" hazard is real as a path fact but harmless — at 9.0.0-beta.15 extract_selections locates join keys by name (the positional check was itself the lancedb#3515 bug, since fixed), and the id-first ordering is not load-bearing; the evaluate_unique all-absent skip cannot hide a real violation for blob-bearing groups, because unique_key_scalar makes a non-null blob under @unique un-committable on every write surface.
| let assigned: HashSet<&str> = assignments.iter().map(|a| a.property.as_str()).collect(); | ||
| let mut completion: HashSet<&str> = HashSet::new(); | ||
| for group in &node_type.unique_constraints { | ||
| if group.iter().any(|col| assigned.contains(col.as_str())) { |
There was a problem hiding this comment.
Correctness — overlapping @unique groups make a legal sole update hard-fail (verified). This loop pulls in only groups intersecting the assigned set — no transitive closure — while evaluate_unique (validate.rs) hard-errors on a partially present group.
Repro: @unique(room, hour) + @unique(hour, day), query update T set { room: $r } where … → completion = {room, hour}; batch = (id, room, hour); group (hour, day) is partially present (hour yes, day no) → the whole mutation fails with missing unique column 'day' though the update touches neither column. Nothing rejects overlapping groups (the parser only checks each column exists), and multiple @unique(...) bodies are grammatical.
Secondary trigger: a composite @key sharing a column with a @unique group — constraints_for registers the @key group as a Unique constraint too, but this fn iterates only node_type.unique_constraints, never node_type.key.
Fix shape: fixed-point-close the completion set over transitively overlapping groups (including the key group), or pass the assigned set into the evaluator so it skips any group the write doesn't intersect.
| output_fields.push(field.clone()); | ||
| } | ||
| if name == "id" || is_assigned { | ||
| stage_cols.push(name.to_string()); |
There was a problem hiding this comment.
Correctness — the partial plan is Blob-blind; one mode reproduced end-to-end.
(a) Reproduced at both commits: update Document set { content: $c } with param c = null on a content: Blob? property. apply_assignments omits a blob column whose resolved value isn't Literal::String, but stage_cols here unconditionally lists every assigned column → staging fails with manifest_internal("partial-update stage column 'content' missing from accumulated batch…"). At the merge-base the same query succeeds (blob left untouched) — and it still succeeds post-PR when another op rides the query (whole-row fallback), so the outcome now depends on unrelated statements in the same query.
(b) Traced: scan_fields doesn't exclude blobs either, so @unique(name, data) with data: Blob plus a sole update assigning name puts the blob into the scan projection — which the projection comment further down says Lance's scanner asserts on.
Fix: exclude non-String-assigned blob columns from stage_cols (mirroring apply_assignments' omission rule, or reject null blob assignment as a typed user error on both paths) and filter blobs from the scan schema. Cheap insurance for (b): the body-level @unique parser arm doesn't reject Blob/Vector the way the @key/@index arms do (schema/parser.rs ~944) — one guard closes that class at compile time.
| /// [`MutationStaging::append_partial_update_batch`], whose eligibility | ||
| /// rule (the table's ONLY op in this query is a single `update`) | ||
| /// guarantees no full-schema batch ever lands on the same table. | ||
| pub(crate) partial_stage_cols: Option<Vec<String>>, |
There was a problem hiding this comment.
Robustness — partial-ness is an Option side-field rather than part of the mode the code dispatches on. stage_pending_table matches table.mode three times (stage-kind, combine/dedupe, stage dispatch); only the last consults partial_stage_cols. A future consumer that matches PendingMode::Merge — a finalize-coalescing change (exactly what the accumulator's own docs invite), a retry/rebase path, a staged-scan union — treats a partial batch as full-row with no compiler help, and silently widening one produces the null-overwrite this PR's comments correctly identify as unsound. A PendingMode variant carrying the partial columns (e.g. Merge { partial: Option<Vec<String>> }) forces every match site to disposition it.
| } | ||
| let merged_rows = batch.num_rows() as u64; | ||
| let inserts_unmatched = matches!(when_not_matched, WhenNotMatched::InsertAll); | ||
| let source_columns: Vec<String> = batch |
There was a problem hiding this comment.
Efficiency — per-merge Vec<String> of all source column names allocated in production for a test-only probe. Every stage_merge_insert — every mutation query and every load — now pays a per-column String clone before the merge, dropped unused when no probes are installed. This breaks the instrumentation module's no-op-in-production pattern (cf. record_open, which passes a cheap borrow and classifies inside the try_with closure). Fix: capture batch.schema() (one Arc bump — already done below for the reader) and materialize the name vec inside record_stage_merge_insert's try_with.
| /// one table commits at most one version per query (invariant 4). Pins the | ||
| /// RFC-022 fallback so the partial path can never split a table's commit. | ||
| #[tokio::test] | ||
| async fn mixed_insert_update_same_table_stages_full_row_upsert() { |
There was a problem hiding this comment.
Test duplication (testing.md extend-vs-new rule). This test and chained_updates_same_table_stage_full_rows duplicate areas existing tests already own: mixed_insert_and_update_on_same_person_coalesces_to_one_merge (line 674) already pins insert+update-on-one-table coalescing — and its STAGED_QUERIES::insert_then_update_same_person fixture is the exact query shape MIXED_INSERT_UPDATE re-mints — while chained_updates_with_overlapping_predicate_respects_intermediate_value (line 1080) owns the chained-update case via update_then_filter_by_old_value. Wrapping those tests' mutate calls in with_merge_write_probes and adding the shape assertions there pins the fallback in one place; two copies of fallback behavior will drift.
| // means "set NULL", so widening a partial batch would null-overwrite), | ||
| // a second same-table op's read-your-writes scan must see full rows, | ||
| // and one table commits at most one version per query (invariant 4). | ||
| let mut update_op_census: std::collections::HashMap<&str, (usize, usize)> = |
There was a problem hiding this comment.
Simplification — the census is over-built and runs for every mutation query. At the Update arm, ops == 1 already implies updates == 1 (the census counts the current op itself), so the second counter and the is_update flag are dead weight; the HashMap is allocated even for insert-only/delete-only queries (the common bulk shapes); and on the partial path non_blob_cols below is still computed then discarded. An inline count at the Update arm — ir.ops.iter().filter(|op| touches(op, type_name)).count() == 1 — replaces the whole pre-pass (op lists are tiny), and non_blob_cols can move into the projection's else arm.
2 participants
What & why
An `update` that is the only statement touching its type now stages a partial merge source — the row key, the assigned columns, and any columns completing a touched `@unique` group — as a matched-only merge, which Lance patches in place on the same fragment. Unassigned columns (including large embedding vectors) are neither read nor rewritten, and indexes over them keep their fragment coverage, closing the class where every update eroded all of a table's index coverage until the next `optimize`. Semantics are unchanged; queries mixing an update with other same-table statements fall back to whole-row staging. One pinned residual: the join-key (`id`) index still loses patched fragments because the merge source must carry the key — parity with the previous behavior, tripwired to tighten once upstream excludes ON columns from column patches.
Backing issue / RFC
Maintainer change (internal process applies); design and validation notes live in the commit messages.
Checklist
Notes for reviewers
The eligibility rule is deliberately narrow (partial staging only when the update is the table's sole op in the query): partial and full batches cannot share one uniform-schema merge source, and a present column's null cell means "set NULL", so widening a partial batch would silently null-overwrite — the fallback matrix is pinned by tests. The surface guard pins the Lance contract this rests on (in-place column patch, missing columns untouched, field-scoped index pruning) and both tripwires go red if upstream behavior shifts.
Note
Medium Risk
Changes the mutation commit path for a common write shape (single updates) and relies on Lance partial-merge semantics; fallbacks and tests limit blast radius, but incorrect eligibility or projection could corrupt rows or indexes.
Overview
Eligible sole
updatestatements on a node type now stage a partial Lance merge: onlyidplus assigned columns are written, withWhenNotMatched::DoNothing(matched-only). The engine scans a narrower column set, keeps@uniquecompletion columns in the validation change-set but projects them out before merge so unassigned columns and their indexes are not touched.Eligibility is strict: one op on that table and it must be a single
update. Insert+update, chained updates, or any second op on the same table still use whole-row merge staging (InsertAll), with internal guards so partial and full batches never mix.Validation skips
@uniquegroups with no column present in a partial batch. Merge-write probes record merge source columns and unmatched-row policy for tests. New integration tests cover staging shape, fallbacks, composite-unique completion, scalar index coverage, and a raw-Lance partial-merge guard. User and dev docs describe the behavior and the knownidBTREE index residual.Reviewed by Cursor Bugbot for commit 985182d. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
This PR adds field-level staging for eligible update mutations. The main changes are:
idand assigned columns.Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "Merge branch 'main' into field-level-upd..." | Re-trigger Greptile
Context used: