From 0dce7c8d185aafe1435ba616fd5373ce352adafb Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Tue, 30 Jun 2026 14:06:49 +0200 Subject: [PATCH] feat(engine): unify constraint validation across all write surfaces (#314) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(engine): unify constraint validation across all write surfaces Constraint enforcement (value/range/check, enum, uniqueness, edge referential integrity, cardinality) was implemented three times — once each in the bulk loader, the mutation executor, and the branch-merge path — and had drifted: merge validated @range/@check but not enum, and neither the mutation nor the load path enforced cross-version uniqueness against already-committed rows. Introduce one catalog-derived evaluator (`crate::validate`) that all three surfaces route through. It is delta-scoped (checks only the change set, not the whole graph) and index-backed (probes committed state through the @key/@unique/src/dst BTREEs instead of full-scanning every catalog table), reusing the existing leaf checks (validate_value_constraints, validate_enum_constraints, composite_unique_key) so the surfaces cannot drift again. A one-row-delta merge now opens ~3 data tables instead of ~6+, and validation cost is flat in graph size rather than O(V+E). Behavior changes (all stricter, none relaxed): - Enum constraints are now enforced on the merge path (was a gap). - A write or load whose @unique value collides with an already-committed different row is now rejected (cross-version uniqueness); re-upserting an existing @key still upserts. - Uniqueness distinguishes a duplicate key WITHIN one input batch (two distinct records -> rejected, e.g. a bulk load listing a @key twice) from the SAME id reappearing ACROSS batches (ordered supersession of one logical row -> coalesced, e.g. a mutation insert-then-update). - Overwrite loads validate per-table: a touched table's committed view is its replacement image (empty), but a table absent from the batch keeps its committed rows, so an edges-only overwrite still resolves referential integrity against retained nodes. Remove the per-surface validation orchestration the evaluator supersedes, and the now-orphaned version-pinned dataset opener from the sealed storage trait (reads route through the snapshot path). Docs (invariants, testing) updated; full engine suite green. * test(engine): pin orphan-edge validation on adopt-by-pointer merge Regression for a gap in the unified merge validation: when a table is adopted by pointer switch (AdoptSourceState) — source on main, target on a branch — build_merge_changeset skips it, so referential integrity is never checked for it. Merging main into a branch that deleted a node while main added an edge to that node silently publishes the orphan edge. This test merges main -> feature where feature deleted Bob and main added Knows Alice->Bob, and asserts an OrphanEdge conflict. Red against HEAD (merge returns Merged); turns green with the AdoptSourceState validation fix. * fix(engine): validate adopt-by-pointer merge tables (AdoptSourceState) The unified merge validator skipped any table classified AdoptSourceState (a pointer switch / fork), so referential integrity, uniqueness, and cardinality were never checked for it. Merging main into a branch that deleted a node while main added an edge to that node silently published the orphan edge — the prior full-scan validation caught it. Root cause: classify_adopt keyed AdoptSourceState on the publish mechanism ("does it advance Lance HEAD") and returned before computing any delta, and build_merge_changeset then skipped the table. Fix decouples the validation input from the publish mechanism: classify_adopt now always computes the source-vs-target delta (base == target on this path, so it is the right validation delta) and carries it as AdoptSourceState { validation_delta }; build_merge_changeset validates it exactly like AdoptWithDelta. The publish stays a pointer/fork (delta ignored) and remains excluded from recovery pins, so publish/recovery semantics are unchanged — only validation is restored. Closes the class: no publish optimization can bypass validation. Turns the orphan-edge regression test green. * test(engine): pin typed committed-uniqueness probe on non-String columns The cross-version @unique check pushes a committed-state filter built from the stringified key. On a non-String @unique column (e.g. Date) this compares a Date32 column to a Utf8 literal — and the stringified key is the raw day count, so the probe raises "Cannot cast string '20633' to Date32" for ANY second write to the table (colliding or not). Two regressions: a colliding Date value must surface a proper "@unique violation" (not a coercion error), and a non-colliding write must succeed. Both red against HEAD; green with the typed-literal probe fix. * fix(engine): build committed uniqueness probe from typed column values The cross-version @unique check pushed a Lance filter built with a stringified key (lit(String)) against the real, typed column. On a non-String @unique column this compared a Date32/numeric/bool column to a Utf8 literal: a coercion error on Date/Bool (failing every write to the table) or a silent miss on Float. For Date the stringified key was even the raw day count, so the literal could never parse. unique_holders now takes typed ScalarValues, built at the call site via ScalarValue::try_from_array(group_column, row), so the pushed-down predicate compares like-typed for any scalar @unique. The in-memory intra-delta dedup keeps the stringified key (a type-agnostic equality grouping, unaffected). Turns the Date @unique cross-version regression tests green. * test(engine): pin id-keyed cardinality on merge-load edge moves/dups Two cardinality drifts between validation and what commit persists: - Move (B): a Merge-load that moves an edge to a new src only recounts the new src, so vacating a src and dropping it below @card min is missed — moving Alice's only WorksAt to Bob silently succeeds under @card(1..). - Dup (A): a Merge-load batch listing one edge id under two srcs counts it under both, but commit dedupes by id (last-wins). Alice gets a phantom second edge and a spurious "has 2 edges (max 1)" violation under @card(0..1). Both red against HEAD; green with the id-keyed last-wins cardinality model. * fix(engine): key merge/load cardinality by edge id, last-wins @card validation diverged from what commit persists in two ways: (1) it only recounted the new src of a delta edge, so a Merge-load that moves an edge to a new src never rechecked the vacated src and missed a drop below @card min; (2) it counted raw delta rows, so the same edge id under two srcs in one batch was counted under both, while commit dedupes by id (last-wins) — a phantom edge and a spurious max violation. evaluate_cardinality now coalesces the delta by edge id (last-wins, matching dedupe_merge_batches_by_id) and builds the affected-src set from both the new src of each delta edge AND the old committed src of each changed/deleted edge id; a committed edge is dropped from its src when the delta deletes or re-places it. The validated edge set per src now equals the committed image. Turns the edge-move and duplicate-id cardinality regression tests green. * docs(rfcs): add RFC 0001 — branch merge by fragment adoption Proposed design for the by-design fix to merge cost/OOM: adopt the source branch's Lance fragments by reference (base_paths) instead of re-materializing rows, with a re-home reconciler + branch-delete reference guard closing the dangling-reference lifecycle, and a reachability-complete cleanup sweep. Grounded in the public Lance 7.0.0 multi-base APIs and the prior art (Delta shallow/deep clone, Iceberg/lakeFS reachability GC). Status: Proposed. * test(engine): pin @card validation on direct edge delete Deletes stage as predicates, not constructive batches, so a delete-only mutation produces an empty change-set and validate_changeset no-ops — a `delete WorksAt where from = X` that removes a source's only edge commits below @card(1..), while the merge path (which carries deleted_ids) rejects it. Red against HEAD (the delete commits); green once the delete path resolves its predicates into the validation change-set. * fix(engine): validate edge cardinality on delete via resolved predicates A delete-only mutation produced an empty change-set (deletes stage as predicates, not constructive batches), so validate_changeset no-op'd and a `delete Edge` that dropped a source below @card min committed silently — while the merge path, which carries deleted_ids, rejects it. validate_staged_mutation now resolves each staged delete predicate against the live committed table (CommittedState::deleted_ids_matching, a SQL-filter scan projecting id) and folds the matched ids into the change-set's deleted_ids for that table. The existing evaluator then recounts the srcs a delete empties (@card min) and sees removed rows for RI/node-delete — the same faithful change-set the merge path already builds, so validation matches what commits. Covers direct edge deletes, node deletes, and node-delete edge cascades uniformly (all are staged predicates). Turns the direct-edge-delete @card regression test green. * refactor(engine): capture deleted ids at delete time, drop validation re-scan The delete-cardinality fix resolved staged delete predicates a second time at validation. Instead, capture the removed ids during the delete op's own scan: execute_delete_edge and the node-delete edge cascade now scan id (not count_rows), record the ids via MutationStaging::record_deleted_ids, and to_changeset() folds them into the change-set's deleted_ids. validate_staged_ mutation reverts to plain to_changeset(); CommittedState::deleted_ids_matching and scan_filtered_sql are removed. Behavior-preserving (the @card-on-delete test stays green) and strictly fewer scans — one scan at delete time replaces count-here + resolve-at-validation. Node deletes already scanned their ids; this reuses that via a shared ids_from_batches helper. Full engine suite green; workspace builds clean. * test(engine): pin overwrite-removal RI + coalesced-unique final image Two reviewer findings, both red against HEAD: - F1 (High): overwriting a node table removes nodes without expressing them as deleted_ids, so a retained edge in a non-overwritten table that references a removed node is published as an orphan (edge-RI path-b never runs). overwrite_node_removal_rejects_retained_orphan_edge. - F2 (Medium): evaluate_unique accumulates superseded keys across batches, so a mutation that frees a @unique value (Alice.email temp -> final) and reuses it (insert Carol.email = temp) false-rejects a valid final image. chained_unique_update_then_reuse_freed_value_is_not_a_violation. * fix(engine): validate overwrite removals (orphan edges, emptied srcs) An Overwrite load replaces each touched table, but to_changeset() only recorded the new batch, never the committed rows the overwrite removes. So overwriting node:Person to drop Bob while a retained edge:Knows(Alice->Bob) referenced him published an orphan edge unchecked — edge-RI path-b is gated on the node's deleted_ids, which were empty. The loader now computes per overwritten table the removed ids (committed ids in the pinned base minus the replacement batch's ids, via validate:: overwrite_removed_ids) and folds them into the change-set's deleted_ids. The evaluator then runs RI path-b and cardinality against them — the same faithful change-set the merge path builds. Overwrite is per-table, so a table absent from the batch is untouched; a removed node referenced by a retained edge is now a loud OrphanEdge. Updates two tests that asserted the old silent-orphan behavior to self-consistent overwrites (per-table Overwrite can't drop edge endpoints without also overwriting the edge tables): end_to_end::overwrite_replaces_data and writes::load_overwrite_with_bad_edge_reference_unblocks_next_load. The orphan-rejection case itself is pinned by the new validators test. * fix(engine): evaluate @unique against the coalesced final delta image evaluate_unique iterated the raw delta batches and accumulated every key it saw into one cross-batch map, so a coalesced write that frees then reuses a @unique value within a query — update a row's email to 'temp', update the same row to 'final', insert a new row with 'temp' — false-rejected: 'temp' lingered in the seen-set from the superseded first write though it no longer holds in the final image that commits. Restructure to validate the final coalesced image — the bytes that actually publish: - Pass 1 coalesces the delta by id (last-wins) into each id's final key, and flags genuine within-ONE-batch duplicates (two distinct input records — the bulk-load contract) before coalescing, so an unordered load batch with a real dup still rejects. - Pass 2 checks two distinct final ids holding the same key. - Pass 3 does the committed cross-version lookup, excluding the delta's own ids. Entries are sorted by id before the cross-row/committed passes so violation order never depends on HashMap iteration. Coalescing first also drops the redundant committed probes a superseded key used to issue. Pinned by the chained-update red test; preserves intra-batch dup rejection (consistency::loader_rejects_intra_batch_duplicate_keys) and cross-version uniqueness (validators). * style(engine): drop trailing blank line at staging.rs EOF Left by a block-delete in an earlier refactor; flagged by git diff --check. * docs(engine): refresh validate.rs module doc to current consumers The module doc still said the merge path was the only consumer and the write path a later, mechanical migration, and listed cardinality as a later increment. Mutation and bulk load have since migrated onto the evaluator and cardinality ships — correct both so the doc reflects that all three write surfaces route through one evaluator. --- crates/omnigraph/src/db/omnigraph.rs | 9 - .../omnigraph/src/db/omnigraph/table_ops.rs | 11 - crates/omnigraph/src/exec/merge.rs | 460 +++---- crates/omnigraph/src/exec/mutation.rs | 322 ++--- crates/omnigraph/src/exec/staging.rs | 281 +---- crates/omnigraph/src/lib.rs | 1 + crates/omnigraph/src/loader/mod.rs | 311 +---- crates/omnigraph/src/storage_layer.rs | 18 - crates/omnigraph/src/table_store.rs | 14 - crates/omnigraph/src/validate.rs | 1085 +++++++++++++++++ crates/omnigraph/tests/branching.rs | 66 + crates/omnigraph/tests/end_to_end.rs | 11 +- crates/omnigraph/tests/merge_cost.rs | 186 +++ crates/omnigraph/tests/validators.rs | 352 +++++- crates/omnigraph/tests/writes.rs | 5 + docs/dev/invariants.md | 2 +- docs/dev/testing.md | 3 +- docs/rfcs/0001-fragment-adopt-branch-merge.md | 432 +++++++ 18 files changed, 2467 insertions(+), 1102 deletions(-) create mode 100644 crates/omnigraph/src/validate.rs create mode 100644 crates/omnigraph/tests/merge_cost.rs create mode 100644 docs/rfcs/0001-fragment-adopt-branch-merge.md diff --git a/crates/omnigraph/src/db/omnigraph.rs b/crates/omnigraph/src/db/omnigraph.rs index 1e6be426..bbff6837 100644 --- a/crates/omnigraph/src/db/omnigraph.rs +++ b/crates/omnigraph/src/db/omnigraph.rs @@ -1786,15 +1786,6 @@ impl Omnigraph { .await } - pub(crate) async fn open_dataset_at_state( - &self, - table_path: &str, - table_branch: Option<&str>, - table_version: u64, - ) -> Result { - table_ops::open_dataset_at_state(self, table_path, table_branch, table_version).await - } - pub(crate) async fn build_indices_on_dataset( &self, table_key: &str, diff --git a/crates/omnigraph/src/db/omnigraph/table_ops.rs b/crates/omnigraph/src/db/omnigraph/table_ops.rs index 26146728..72cdb086 100644 --- a/crates/omnigraph/src/db/omnigraph/table_ops.rs +++ b/crates/omnigraph/src/db/omnigraph/table_ops.rs @@ -936,17 +936,6 @@ pub(super) async fn reopen_for_mutation( } } -pub(super) async fn open_dataset_at_state( - db: &Omnigraph, - table_path: &str, - table_branch: Option<&str>, - table_version: u64, -) -> Result { - db.storage() - .open_dataset_at_state(table_path, table_branch, table_version) - .await -} - /// A declared index the builder could not materialize on this pass. Today the /// only such case is a vector (IVF) column with no trainable vectors yet /// (KMeans needs >=1 vector), e.g. the load-before-embed window. Reported, not diff --git a/crates/omnigraph/src/exec/merge.rs b/crates/omnigraph/src/exec/merge.rs index 8c36f454..81fb4850 100644 --- a/crates/omnigraph/src/exec/merge.rs +++ b/crates/omnigraph/src/exec/merge.rs @@ -6,8 +6,17 @@ const MERGE_STAGE_DIR_ENV: &str = "OMNIGRAPH_MERGE_STAGING_DIR"; #[derive(Debug)] enum CandidateTableState { /// Adopt the source's table state via a pointer switch or a branch fork — - /// no data HEAD advance, so nothing to pin for recovery. - AdoptSourceState, + /// no data HEAD advance, so nothing to pin for recovery. `validation_delta` + /// carries the source-vs-target row delta (added/changed/deleted) for the + /// evaluator ONLY — the publish is still a pointer/fork — so a pointer-adopt + /// whose source diverged is still validated (RI/uniqueness/cardinality) + /// against the merged state instead of being silently published. `None` when + /// the source matched the target (nothing to validate). Decoupling the + /// validation delta from the publish mechanism keeps the publish O(1) while + /// closing the unvalidated-adopt gap. + AdoptSourceState { + validation_delta: Option, + }, /// Adopt the source's state by applying a non-empty delta onto the target's /// lineage (append new + upsert changed + delete removed). The delta is /// pre-computed at classification so this candidate can be recovery-pinned: @@ -24,7 +33,6 @@ struct StagedTable { #[derive(Debug)] struct StagedMergeResult { - full_staged: StagedTable, delta_staged: Option, deleted_ids: Vec, } @@ -446,7 +454,6 @@ async fn stage_streaming_table_merge( conflicts: &mut Vec, ) -> Result> { let schema = schema_for_table_key(catalog, table_key)?; - let mut full_writer = StagedTableWriter::new(&format!("{}_full", table_key), schema.clone())?; let mut delta_writer = StagedTableWriter::new(&format!("{}_delta", table_key), schema)?; let mut deleted_ids: Vec = Vec::new(); let mut base = OrderedTableCursor::from_snapshot(base_snapshot, table_key).await?; @@ -514,9 +521,9 @@ async fn stage_streaming_table_merge( } if let Some(selection) = selection { - // Always write to full (for validation) - full_writer.push_row(selection).await?; - // Only write changed rows to delta (for publish) + // Only changed rows go to the delta (for publish). The full merged + // table is no longer staged — validation works off this delta plus + // the committed target via index lookups, not a full re-scan. if selection.signature.as_str() != target_sig.unwrap_or("") { delta_writer.push_row(selection).await?; needs_update = true; @@ -538,7 +545,6 @@ async fn stage_streaming_table_merge( }; Ok(Some(StagedMergeResult { - full_staged: full_writer.finish().await?, delta_staged, deleted_ids, })) @@ -621,297 +627,138 @@ fn row_signature(batch: &RecordBatch, row: usize) -> Result { Ok(values.join("\u{1f}")) } -async fn scan_validation_stream(ds: &Dataset) -> Result { - crate::table_store::TableStore::scan_stream_with(ds, None, None, None, false, |_| Ok(())).await +/// Build the per-table [`ChangeSet`](crate::validate::ChangeSet) for a merge from +/// the classified candidates — the new/changed rows (from the staged deltas) and +/// removed ids the validator evaluates, instead of re-scanning whole tables. +/// `AdoptSourceState` is published as a pointer/fork but still carries a +/// `validation_delta` (the source-vs-target rows) when its source diverged, so +/// it is validated like `AdoptWithDelta`; only an empty-delta adopt is skipped. +async fn build_merge_changeset( + db: &Omnigraph, + candidates: &HashMap, +) -> Result { + let catalog = db.catalog(); + let mut changeset = crate::validate::ChangeSet::new(); + for (table_key, candidate) in candidates { + // Validation reads only id/src/dst + scalar constraint columns; project + // out Vector/Blob so the change-set never holds embeddings (holding the + // delta with embeddings would re-introduce the memory pressure the + // streaming append exists to avoid). + let projection = validation_projection(&catalog, table_key); + let projection: Vec<&str> = projection.iter().map(String::as_str).collect(); + let mut change = crate::validate::TableChange::default(); + match candidate { + // Pointer/fork adopt whose source matched the target: nothing to + // validate. A pointer/fork adopt whose source diverged carries a + // `validation_delta` and is validated exactly like `AdoptWithDelta` + // (only the publish differs — pointer vs HEAD-advancing). + CandidateTableState::AdoptSourceState { + validation_delta: None, + } => continue, + CandidateTableState::AdoptSourceState { + validation_delta: Some(delta), + } + | CandidateTableState::AdoptWithDelta(delta) => { + if let Some(table) = &delta.appends { + change + .added + .extend(scan_staged_for_validation(db, table, &projection).await?); + } + if let Some(table) = &delta.upserts { + change + .changed + .extend(scan_staged_for_validation(db, table, &projection).await?); + } + change.deleted_ids = delta.deleted_ids.clone(); + } + CandidateTableState::RewriteMerged(staged) => { + if let Some(table) = &staged.delta_staged { + change + .changed + .extend(scan_staged_for_validation(db, table, &projection).await?); + } + change.deleted_ids = staged.deleted_ids.clone(); + } + } + changeset.insert(table_key.clone(), change); + } + Ok(changeset) +} + +/// Columns validation needs from a staged delta: `id` (+ `src`/`dst` for edges) +/// plus scalar/enum property columns. Vector and Blob columns are excluded — no +/// constraint reads them, and keeping them out of the change-set keeps validation +/// memory bounded regardless of embedding width. +fn validation_projection(catalog: &Catalog, table_key: &str) -> Vec { + use omnigraph_compiler::types::{PropType, ScalarType}; + let is_heavy = |ty: &PropType| matches!(ty.scalar, ScalarType::Vector(_) | ScalarType::Blob); + let mut cols = vec!["id".to_string()]; + if let Some(name) = table_key.strip_prefix("node:") { + if let Some(node_type) = catalog.node_types.get(name) { + for (prop, ty) in &node_type.properties { + if !is_heavy(ty) { + cols.push(prop.clone()); + } + } + } + } else if let Some(name) = table_key.strip_prefix("edge:") { + cols.push("src".to_string()); + cols.push("dst".to_string()); + if let Some(edge_type) = catalog.edge_types.get(name) { + for (prop, ty) in &edge_type.properties { + if !is_heavy(ty) { + cols.push(prop.clone()); + } + } + } + } + cols +} + +/// Scan a staged delta table for validation, projected to the constraint columns +/// (no embeddings) and kept batch-shaped — never concatenated into one batch, so +/// it does not reintroduce the whole-delta materialization the streaming append +/// avoids. Empty batches are dropped. +async fn scan_staged_for_validation( + db: &Omnigraph, + table: &StagedTable, + projection: &[&str], +) -> Result> { + let snapshot = SnapshotHandle::new(table.dataset.clone()); + let batches = db + .storage() + .scan(&snapshot, Some(projection), None, None) + .await?; + Ok(batches + .into_iter() + .filter(|batch| batch.num_rows() > 0) + .collect()) } async fn validate_merge_candidates( db: &Omnigraph, - source_snapshot: &Snapshot, target_snapshot: &Snapshot, - candidates: &HashMap, + changeset: &crate::validate::ChangeSet, ) -> Result<()> { - let mut conflicts = Vec::new(); - let mut node_ids: HashMap> = HashMap::new(); - - for (type_name, node_type) in &db.catalog().node_types { - let table_key = format!("node:{}", type_name); - let mut values = HashSet::new(); - let mut unique_seen = vec![HashMap::new(); node_type.unique_constraints.len()]; - - if let Some(ds) = - candidate_dataset(source_snapshot, target_snapshot, candidates, &table_key).await? - { - let mut stream = scan_validation_stream(&ds).await?; - while let Some(batch) = stream - .try_next() - .await - .map_err(|e| OmniError::Lance(e.to_string()))? - { - if let Err(err) = crate::loader::validate_value_constraints(&batch, node_type) { - conflicts.push(MergeConflict { - table_key: table_key.clone(), - row_id: None, - kind: MergeConflictKind::ValueConstraintViolation, - message: err.to_string(), - }); - } - update_unique_constraints( - &table_key, - &batch, - &node_type.unique_constraints, - &mut unique_seen, - &mut conflicts, - )?; - let ids = batch - .column_by_name("id") - .ok_or_else(|| { - OmniError::manifest(format!("table {} missing id column", table_key)) - })? - .as_any() - .downcast_ref::() - .ok_or_else(|| { - OmniError::manifest(format!("table {} id column is not Utf8", table_key)) - })?; - for row in 0..ids.len() { - values.insert(ids.value(row).to_string()); - } - } - } - node_ids.insert(type_name.clone(), values); - } - - for (edge_name, edge_type) in &db.catalog().edge_types { - let table_key = format!("edge:{}", edge_name); - let mut unique_seen = vec![HashMap::new(); edge_type.unique_constraints.len()]; - let mut src_counts = HashMap::new(); - - if let Some(ds) = - candidate_dataset(source_snapshot, target_snapshot, candidates, &table_key).await? - { - let mut stream = scan_validation_stream(&ds).await?; - while let Some(batch) = stream - .try_next() - .await - .map_err(|e| OmniError::Lance(e.to_string()))? - { - update_unique_constraints( - &table_key, - &batch, - &edge_type.unique_constraints, - &mut unique_seen, - &mut conflicts, - )?; - accumulate_edge_cardinality(&batch, &mut src_counts, &table_key)?; - conflicts.extend(validate_orphan_edges_batch( - &table_key, edge_type, &batch, &node_ids, - )?); - } - } - - conflicts.extend(finalize_edge_cardinality_conflicts( - &table_key, - edge_name, - edge_type.cardinality.min, - edge_type.cardinality.max, - src_counts, - )); - } - - if conflicts.is_empty() { + // Δ-scoped, index-backed validation: the declared constraints are evaluated + // over the merge delta against the committed target (queried through its + // BTREE indexes), not by re-scanning every catalog table. Value/enum, + // uniqueness, edge-RI, and cardinality all route through one evaluator shared + // with (eventually) the write path — closing the merge-vs-write drift. + let committed = crate::validate::CommittedState::merge(target_snapshot); + let constraints = crate::validate::constraints_for(&db.catalog()); + let violations = + crate::validate::evaluate(&constraints, changeset, &committed, &db.catalog()).await?; + if violations.is_empty() { Ok(()) } else { - Err(OmniError::MergeConflicts(conflicts)) - } -} - -async fn candidate_dataset( - source_snapshot: &Snapshot, - target_snapshot: &Snapshot, - candidates: &HashMap, - table_key: &str, -) -> Result> { - if let Some(candidate) = candidates.get(table_key) { - return match candidate { - CandidateTableState::AdoptSourceState | CandidateTableState::AdoptWithDelta(_) => { - match source_snapshot.entry(table_key) { - Some(_) => Ok(Some(source_snapshot.open(table_key).await?)), - None => Ok(None), - } - } - CandidateTableState::RewriteMerged(staged) => { - Ok(Some(staged.full_staged.dataset.clone())) - } - }; - } - match target_snapshot.entry(table_key) { - Some(_) => Ok(Some(target_snapshot.open(table_key).await?)), - None => Ok(None), - } -} - -fn update_unique_constraints( - table_key: &str, - batch: &RecordBatch, - constraints: &[Vec], - seen: &mut [HashMap, String>], - conflicts: &mut Vec, -) -> Result<()> { - for (constraint_idx, columns) in constraints.iter().enumerate() { - let seen = &mut seen[constraint_idx]; - // Resolve the group's columns once. The candidate dataset always - // carries the full table schema, so a missing column is an internal - // error rather than a skip. - let group_columns = columns - .iter() - .map(|column_name| { - batch.column_by_name(column_name).cloned().ok_or_else(|| { - OmniError::manifest(format!( - "table {} missing unique column '{}'", - table_key, column_name - )) - }) - }) - .collect::>>()?; - for row in 0..batch.num_rows() { - // Same tuple key as the intake path — one shared derivation in - // `crate::loader::composite_unique_key`, so the two cannot drift on - // separator or scalar conversion. Null rows are exempt. - let Some(key) = crate::loader::composite_unique_key(&group_columns, row)? else { - continue; - }; - let row_id = row_id_at(batch, row)?; - if let Some(first_row_id) = seen.insert(key, row_id.clone()) { - conflicts.push(MergeConflict { - table_key: table_key.to_string(), - row_id: Some(row_id.clone()), - kind: MergeConflictKind::UniqueViolation, - message: format!( - "unique constraint {:?} violated by '{}' and '{}'", - columns, first_row_id, row_id - ), - }); - } - } - } - Ok(()) -} - -fn accumulate_edge_cardinality( - batch: &RecordBatch, - counts: &mut HashMap, - table_key: &str, -) -> Result<()> { - let srcs = batch - .column_by_name("src") - .ok_or_else(|| OmniError::manifest(format!("table {} missing src column", table_key)))? - .as_any() - .downcast_ref::() - .ok_or_else(|| { - OmniError::manifest(format!("table {} src column is not Utf8", table_key)) - })?; - for row in 0..srcs.len() { - *counts.entry(srcs.value(row).to_string()).or_insert(0_u32) += 1; - } - Ok(()) -} - -fn finalize_edge_cardinality_conflicts( - table_key: &str, - edge_name: &str, - min: u32, - max: Option, - counts: HashMap, -) -> Vec { - let mut conflicts = Vec::new(); - for (src, count) in counts { - if let Some(max) = max { - if count > max { - conflicts.push(MergeConflict { - table_key: table_key.to_string(), - row_id: None, - kind: MergeConflictKind::CardinalityViolation, - message: format!( - "@card violation on edge {}: source '{}' has {} edges (max {})", - edge_name, src, count, max - ), - }); - } - } - if count < min { - conflicts.push(MergeConflict { - table_key: table_key.to_string(), - row_id: None, - kind: MergeConflictKind::CardinalityViolation, - message: format!( - "@card violation on edge {}: source '{}' has {} edges (min {})", - edge_name, src, count, min - ), - }); - } - } - conflicts -} - -fn validate_orphan_edges_batch( - table_key: &str, - edge_type: &omnigraph_compiler::catalog::EdgeType, - batch: &RecordBatch, - node_ids: &HashMap>, -) -> Result> { - let srcs = batch - .column_by_name("src") - .ok_or_else(|| OmniError::manifest(format!("table {} missing src column", table_key)))? - .as_any() - .downcast_ref::() - .ok_or_else(|| { - OmniError::manifest(format!("table {} src column is not Utf8", table_key)) - })?; - let dsts = batch - .column_by_name("dst") - .ok_or_else(|| OmniError::manifest(format!("table {} missing dst column", table_key)))? - .as_any() - .downcast_ref::() - .ok_or_else(|| { - OmniError::manifest(format!("table {} dst column is not Utf8", table_key)) - })?; - - let from_ids = node_ids.get(&edge_type.from_type).ok_or_else(|| { - OmniError::manifest(format!( - "missing candidate node ids for {}", - edge_type.from_type + Err(OmniError::MergeConflicts( + violations + .into_iter() + .map(crate::validate::Violation::into_merge_conflict) + .collect(), )) - })?; - let to_ids = node_ids.get(&edge_type.to_type).ok_or_else(|| { - OmniError::manifest(format!( - "missing candidate node ids for {}", - edge_type.to_type - )) - })?; - - let mut conflicts = Vec::new(); - for row in 0..batch.num_rows() { - let row_id = row_id_at(batch, row)?; - let src = srcs.value(row); - let dst = dsts.value(row); - if !from_ids.contains(src) { - conflicts.push(MergeConflict { - table_key: table_key.to_string(), - row_id: Some(row_id.clone()), - kind: MergeConflictKind::OrphanEdge, - message: format!("src '{}' not found in {}", src, edge_type.from_type), - }); - } - if !to_ids.contains(dst) { - conflicts.push(MergeConflict { - table_key: table_key.to_string(), - row_id: Some(row_id), - kind: MergeConflictKind::OrphanEdge, - message: format!("dst '{}' not found in {}", dst, edge_type.to_type), - }); - } } - Ok(conflicts) } fn row_id_at(batch: &RecordBatch, row: usize) -> Result { @@ -944,7 +791,10 @@ async fn classify_adopt( table_key: &str, ) -> Result { let Some(source_entry) = source_snapshot.entry(table_key) else { - return Ok(CandidateTableState::AdoptSourceState); + // Source has no such table — nothing to adopt or validate. + return Ok(CandidateTableState::AdoptSourceState { + validation_delta: None, + }); }; let target_entry = target_snapshot.entry(table_key); let target_active = target_db.active_branch().await; @@ -961,12 +811,21 @@ async fn classify_adopt( // Source on main (pointer switch) or target doesn't own (fork): no advance. _ => false, }; - if !advances_head { - return Ok(CandidateTableState::AdoptSourceState); - } - match compute_adopt_delta(table_key, catalog, base_snapshot, source_snapshot).await? { - Some(delta) => Ok(CandidateTableState::AdoptWithDelta(delta)), - None => Ok(CandidateTableState::AdoptSourceState), + // Compute the source-vs-target delta UNCONDITIONALLY — it is the validation + // input the evaluator needs, independent of how the table is published. + // (`classify_adopt` is only reached when base == target, so the + // base-vs-source delta equals the target-vs-source delta.) A HEAD-advancing + // publish consumes it as the write payload (`AdoptWithDelta`); a pointer/fork + // publish ignores it and only validates it (`AdoptSourceState`), so a + // pointer-adopt whose source diverged is still checked for + // RI/uniqueness/cardinality against the merged state. + let validation_delta = + compute_adopt_delta(table_key, catalog, base_snapshot, source_snapshot).await?; + match (advances_head, validation_delta) { + (true, Some(delta)) => Ok(CandidateTableState::AdoptWithDelta(delta)), + (_, validation_delta) => { + Ok(CandidateTableState::AdoptSourceState { validation_delta }) + } } } @@ -1588,7 +1447,8 @@ impl Omnigraph { return Err(OmniError::MergeConflicts(conflicts)); } - validate_merge_candidates(self, source_snapshot, &target_snapshot, &candidates).await?; + let changeset = build_merge_changeset(self, &candidates).await?; + validate_merge_candidates(self, &target_snapshot, &changeset).await?; // Recovery sidecar: protect the per-table commit_staged loop. // Pin `RewriteMerged` and `AdoptWithDelta` candidates — both advance @@ -1627,7 +1487,7 @@ impl Omnigraph { matches!( candidates.get(*table_key), Some(CandidateTableState::RewriteMerged(_)) - | Some(CandidateTableState::AdoptSourceState) + | Some(CandidateTableState::AdoptSourceState { .. }) | Some(CandidateTableState::AdoptWithDelta(_)) ) }) @@ -1643,7 +1503,7 @@ impl Omnigraph { if !matches!( candidate, CandidateTableState::RewriteMerged(_) - | CandidateTableState::AdoptSourceState + | CandidateTableState::AdoptSourceState { .. } | CandidateTableState::AdoptWithDelta(_) ) { continue; @@ -1746,7 +1606,7 @@ impl Omnigraph { continue; }; let update = match candidate_state { - CandidateTableState::AdoptSourceState => { + CandidateTableState::AdoptSourceState { .. } => { publish_adopted_source_state(self, source_snapshot, &target_snapshot, table_key) .await? } diff --git a/crates/omnigraph/src/exec/mutation.rs b/crates/omnigraph/src/exec/mutation.rs index 78cedd65..c56897a8 100644 --- a/crates/omnigraph/src/exec/mutation.rs +++ b/crates/omnigraph/src/exec/mutation.rs @@ -338,112 +338,6 @@ fn build_insert_batch( RecordBatch::try_new(schema.clone(), columns).map_err(|e| OmniError::Lance(e.to_string())) } -async fn validate_edge_insert_endpoints( - db: &Omnigraph, - staging: &MutationStaging, - branch: Option<&str>, - edge_name: &str, - assignments: &HashMap, -) -> Result<()> { - let catalog = db.catalog(); - let edge_type = catalog - .edge_types - .get(edge_name) - .ok_or_else(|| OmniError::manifest(format!("unknown edge type '{}'", edge_name)))?; - let from = match assignments.get("from") { - Some(Literal::String(value)) => value.as_str(), - Some(other) => { - return Err(OmniError::manifest(format!( - "edge {} from endpoint must be a string id, got {}", - edge_name, - literal_to_sql(other) - ))); - } - None => { - return Err(OmniError::manifest(format!( - "edge {} missing 'from' endpoint", - edge_name - ))); - } - }; - let to = match assignments.get("to") { - Some(Literal::String(value)) => value.as_str(), - Some(other) => { - return Err(OmniError::manifest(format!( - "edge {} to endpoint must be a string id, got {}", - edge_name, - literal_to_sql(other) - ))); - } - None => { - return Err(OmniError::manifest(format!( - "edge {} missing 'to' endpoint", - edge_name - ))); - } - }; - - ensure_node_id_exists(db, staging, branch, &edge_type.from_type, from, "src").await?; - ensure_node_id_exists(db, staging, branch, &edge_type.to_type, to, "dst").await?; - Ok(()) -} - -/// Quick scan of pending batches for an `id` value match. Used by the -/// mutation path's edge endpoint validation to satisfy read-your-writes -/// for same-query inserts before they're committed to Lance. -fn pending_batches_contain_id(batches: &[RecordBatch], id: &str) -> bool { - for batch in batches { - let Some(col) = batch.column_by_name("id") else { - continue; - }; - let Some(arr) = col.as_any().downcast_ref::() else { - continue; - }; - for i in 0..arr.len() { - if arr.is_valid(i) && arr.value(i) == id { - return true; - } - } - } - false -} - -async fn ensure_node_id_exists( - db: &Omnigraph, - staging: &MutationStaging, - branch: Option<&str>, - node_type: &str, - id: &str, - label: &str, -) -> Result<()> { - let table_key = format!("node:{}", node_type); - - // Prefer the in-query pending accumulator so a same-query insert of - // the referenced node is visible to this validation. Fall back to - // the pre-mutation manifest snapshot when nothing pending matches. - let pending = staging.pending_batches(&table_key); - if pending_batches_contain_id(pending, id) { - return Ok(()); - } - - let filter = format!("id = '{}'", id.replace('\'', "''")); - let snapshot = db.snapshot_for_branch(branch).await?; - let ds = db - .storage() - .open_snapshot_at_table(&snapshot, &table_key) - .await?; - let exists = db.storage().count_rows(&ds, Some(filter)).await? > 0; - - if exists { - Ok(()) - } else { - Err(OmniError::manifest(format!( - "{} '{}' not found in {}", - label, id, node_type - ))) - } -} - /// Convert an IRMutationPredicate to a Lance SQL filter string. fn predicate_to_sql( predicate: &IRMutationPredicate, @@ -588,40 +482,6 @@ use super::staging::{MutationStaging, PendingMode}; /// them into one staged delete — there is no post-inline-commit reopen to /// special-case anymore. impl Omnigraph { - /// Resolve a LIVE-HEAD read handle for an edge table's committed-state `@card` - /// scan when collapse #1 skipped the accumulation open. The edge-insert path no - /// longer opens the edge dataset (non-strict op + txn), but cardinality is - /// validated ONCE (never rechecked at commit), so the scan must observe the - /// freshest committed edges — NOT the pinned `txn.base`. A concurrent writer can - /// commit edges to this table after `txn` capture; counting against the stale - /// base undercounts and lets a violating insert through (invariant 9). The table - /// LOCATION is read from the pinned entry (stable across versions); the dataset is - /// opened at live HEAD via `open_dataset_head_for_write` (a read here despite the - /// name — no lock/stage), restoring the pre-3b image (the mutation's own open). - /// The residual validate→commit race (a writer committing between this scan and - /// the end-of-query commit) is the §7.1 gap, closed by RFC-013 step 4. - async fn edge_cardinality_read_handle( - &self, - txn: Option<&crate::db::WriteTxn>, - table_key: &str, - ) -> Result { - let branch = txn.and_then(|t| t.branch.as_deref()); - match txn.and_then(|t| t.base.entry(table_key)) { - Some(entry) => { - let full_path = self.storage().dataset_uri(&entry.table_path); - self.storage() - .open_dataset_head_for_write(table_key, &full_path, branch) - .await - } - // Unreachable today (the `None` handle only reaches here under a txn whose - // base contains the table). Defensive: resolve the table fresh (live) - // without the schema re-validation `snapshot_for_branch` would re-run. - None => { - let snapshot = self.fresh_snapshot_for_branch_unchecked(branch).await?; - self.storage().open_snapshot_at_table(&snapshot, table_key).await - } - } - } } async fn open_table_for_mutation( @@ -776,6 +636,32 @@ impl Omnigraph { .await } + /// End-of-query validation for a constructive mutation: build the change-set + /// from the accumulated staging and run the unified evaluator (value/enum, + /// uniqueness incl. cross-version, edge-RI, cardinality) against committed + /// state. Read-your-writes is inherent — every same-query insert is already + /// in the change-set. Destructive queries (D2) stage no constructive batches, + /// so the change-set is empty and this is a no-op (deletes cascade). + async fn validate_staged_mutation( + &self, + staging: &MutationStaging, + txn: &crate::db::WriteTxn, + ) -> Result<()> { + // RI/uniqueness read the write's already-validated pinned base (`txn.base`), + // NOT a fresh `snapshot_for_branch` — which would re-run the schema-contract + // validation the WriteTxn already did once (RFC-013 step 3b capture-once). + // Cardinality reads LIVE HEAD per edge table (the #298 stale-handle fix) via + // the live opener in `CommittedState::write`. + let committed = + crate::validate::CommittedState::write(&txn.base, self, txn.branch.as_deref()); + // `to_changeset` carries both constructive batches and the ids the delete + // ops captured from their own scans (`deleted_ids`), so the evaluator + // recounts the srcs a delete empties (`@card`) and sees removed rows for + // RI — the faithful change-set the merge path also builds. + crate::validate::validate_changeset(&staging.to_changeset(), &committed, &self.catalog()) + .await + } + async fn mutate_with_current_actor( &self, branch: &str, @@ -872,6 +758,7 @@ impl Omnigraph { Err(e) => Err(e), Ok(total) if staging.is_empty() => Ok(total), Ok(total) => { + self.validate_staged_mutation(&staging, &txn).await?; let staged = staging.stage_all(self, requested.as_deref()).await?; // `_queue_guards` holds per-(table_key, branch) write // queues acquired inside `commit_all`. Held across the @@ -1100,16 +987,7 @@ impl Omnigraph { }; let batch = build_insert_batch(&schema, &id, &resolved, &blob_props)?; - crate::loader::validate_value_constraints(&batch, node_type)?; - crate::loader::validate_enum_constraints(&batch, &node_type.properties, type_name)?; - let unique_groups = crate::loader::unique_constraint_groups_for_node(node_type); - if !unique_groups.is_empty() { - crate::loader::enforce_unique_constraints_intra_batch( - &batch, - type_name, - &unique_groups, - )?; - } + // Validation (value/enum/unique) runs end-of-query via the evaluator. let has_key = node_type.key_property().is_some(); let table_key = format!("node:{}", type_name); // Capture pre-write metadata on first touch (no Lance write). @@ -1145,21 +1023,14 @@ impl Omnigraph { let id = ulid::Ulid::new().to_string(); let batch = build_insert_batch(&schema, &id, &resolved, &blob_props)?; - validate_edge_insert_endpoints(self, staging, branch, type_name, &resolved).await?; - crate::loader::validate_enum_constraints(&batch, &edge_type.properties, type_name)?; - let unique_groups = crate::loader::unique_constraint_groups_for_edge(edge_type); - if !unique_groups.is_empty() { - crate::loader::enforce_unique_constraints_intra_batch( - &batch, - type_name, - &unique_groups, - )?; - } + // Validation (edge-RI, enum, unique, @card against LIVE HEAD) runs + // end-of-query via the evaluator. let table_key = format!("edge:{}", type_name); - // Capture pre-write metadata on first touch. Edge inserts are - // non-strict, so with a `WriteTxn` this opens NOTHING (collapse #1) - // and returns `None`. - let (handle, _full_path, _table_branch) = open_table_for_mutation( + // Capture pre-write metadata on first touch (ensure_path). Edge + // inserts are non-strict, so with a `WriteTxn` this opens NOTHING + // (collapse #1) and the handle is discarded — validation, including + // `@card` against LIVE HEAD, runs end-of-query via the evaluator. + let (_handle, _full_path, _table_branch) = open_table_for_mutation( self, staging, branch, @@ -1170,32 +1041,7 @@ impl Omnigraph { .await?; // Accumulate the new edge row. Edge IDs are ULID-generated so // Append mode is correct (no key-based dedup needed). - staging.append_batch(&table_key, schema, PendingMode::Append, batch.clone())?; - - // Edge cardinality validation: scan committed edges via Lance - // + iterate pending edges in-memory for the `src` column, - // group-by-src. The pending side already includes the row - // we just appended (above). When the open was skipped (collapse - // #1), resolve a read handle for the committed scan at LIVE HEAD - // (`edge_cardinality_read_handle`, #298) — NOT the pinned txn.base, - // which would undercount edges a concurrent writer committed since - // capture. Only when cardinality is non-default, so the common - // default-cardinality edge keeps the open-free path. (The residual - // validate→commit race is the §7.1 gap — step 4.) - if !edge_type.cardinality.is_default() { - let committed_ds = match handle { - Some(h) => h, - None => self.edge_cardinality_read_handle(txn, &table_key).await?, - }; - validate_edge_cardinality_with_pending( - self, - &committed_ds, - staging, - &table_key, - edge_type, - ) - .await?; - } + staging.append_batch(&table_key, schema, PendingMode::Append, batch)?; self.invalidate_graph_index().await; @@ -1318,17 +1164,7 @@ impl Omnigraph { resolved.insert(a.property.clone(), resolve_expr_value(&a.value, params)?); } let updated = apply_assignments(&schema, &matched, &resolved, &blob_props)?; - let node_type = &self.catalog().node_types[type_name]; - crate::loader::validate_value_constraints(&updated, node_type)?; - crate::loader::validate_enum_constraints(&updated, &node_type.properties, type_name)?; - let unique_groups = crate::loader::unique_constraint_groups_for_node(node_type); - if !unique_groups.is_empty() { - crate::loader::enforce_unique_constraints_intra_batch( - &updated, - type_name, - &unique_groups, - )?; - } + // Validation (value/enum/unique) runs end-of-query via the evaluator. // Accumulate the updated batch into the Merge-mode pending stream. // The accumulator may now contain entries with the same id as a @@ -1402,19 +1238,7 @@ impl Omnigraph { .scan(&ds, Some(&["id"]), Some(&scan_filter), None) .await?; - let deleted_ids: Vec = batches - .iter() - .flat_map(|batch| { - let ids = batch - .column(0) - .as_any() - .downcast_ref::() - .unwrap(); - (0..ids.len()) - .map(|i| ids.value(i).to_string()) - .collect::>() - }) - .collect(); + let deleted_ids: Vec = ids_from_batches(&batches); if deleted_ids.is_empty() { return Ok(MutationResult { @@ -1433,6 +1257,7 @@ impl Omnigraph { // `open_table_for_mutation` above already captured the table's // path/version/op-kind via `ensure_path`. crate::failpoints::maybe_fail(crate::failpoints::names::MUTATION_DELETE_NODE_PRE_PRIMARY_DELETE)?; + staging.record_deleted_ids(&table_key, &deleted_ids); staging.record_delete(&table_key, pred_sql.clone()); let mut affected_edges = 0usize; @@ -1487,13 +1312,20 @@ impl Omnigraph { // delete removes the union); skip only when nothing NEW matches. let count_filter = dedup_delete_filter(&cascade_filter, staging.recorded_delete_predicates(&edge_table_key)); - let matched = self - .storage() - .count_rows(&edge_ds, Some(count_filter)) - .await?; + // Scan (not count) the cascade-removed edge ids so validation + // recounts the OTHER endpoint's @card after the cascade; `len()` is + // the affected count. + let matched_ids = ids_from_batches( + &self + .storage() + .scan(&edge_ds, Some(&["id"]), Some(&count_filter), None) + .await?, + ); + let matched = matched_ids.len(); affected_edges += matched; if matched > 0 { + staging.record_deleted_ids(&edge_table_key, &matched_ids); staging.record_delete(&edge_table_key, cascade_filter); } } @@ -1540,12 +1372,20 @@ impl Omnigraph { // union); only record when something NEW matches. let count_filter = dedup_delete_filter(&pred_sql, staging.recorded_delete_predicates(&table_key)); - let affected = self - .storage() - .count_rows(&ds, Some(count_filter)) - .await?; + // Scan the matched edge ids (not just count): the ids feed validation so + // a delete emptying a src below @card min is rejected; `len()` is the + // affected count. One scan replaces the former count-here + resolve-at- + // validation re-scan. + let deleted_ids = ids_from_batches( + &self + .storage() + .scan(&ds, Some(&["id"]), Some(&count_filter), None) + .await?, + ); + let affected = deleted_ids.len(); if affected > 0 { + staging.record_deleted_ids(&table_key, &deleted_ids); staging.record_delete(&table_key, pred_sql.clone()); self.invalidate_graph_index().await; } @@ -1557,6 +1397,25 @@ impl Omnigraph { } } +/// Extract the `id` column (projection index 0) from scanned batches. Used by +/// the delete paths to capture the rows they remove, so validation recounts a +/// src a delete empties without re-resolving the predicate. +fn ids_from_batches(batches: &[RecordBatch]) -> Vec { + batches + .iter() + .flat_map(|batch| { + let ids = batch + .column(0) + .as_any() + .downcast_ref::() + .unwrap(); + (0..ids.len()) + .map(|i| ids.value(i).to_string()) + .collect::>() + }) + .collect() +} + /// Concat the matched batches from `scan_with_pending` into a single batch. /// `scan_with_pending` returns committed-side and pending-side batches in /// order; both should share a schema if pending was produced through @@ -1583,25 +1442,6 @@ fn concat_match_batches_to_schema( }) } -/// Validate `@card` bounds against committed (Lance) + pending (in-memory) -/// edges for one edge table. Engine path: each insert produces a fresh -/// ULID id, so committed and pending cannot share a primary key — no -/// dedup needed (`dedupe_key_column = None`). -async fn validate_edge_cardinality_with_pending( - db: &Omnigraph, - committed_ds: &SnapshotHandle, - staging: &MutationStaging, - table_key: &str, - edge_type: &omnigraph_compiler::catalog::EdgeType, -) -> Result<()> { - if edge_type.cardinality.is_default() { - return Ok(()); - } - let counts = - super::staging::count_src_per_edge(db, committed_ds, table_key, staging, None).await?; - super::staging::enforce_cardinality_bounds(edge_type, &counts) -} - fn enrich_mutation_params(params: &ParamMap) -> Result { let mut resolved = params.clone(); if !resolved.contains_key(NOW_PARAM_NAME) { diff --git a/crates/omnigraph/src/exec/staging.rs b/crates/omnigraph/src/exec/staging.rs index 20c7bb28..1aa21bcf 100644 --- a/crates/omnigraph/src/exec/staging.rs +++ b/crates/omnigraph/src/exec/staging.rs @@ -28,7 +28,6 @@ use crate::storage_layer::{SnapshotHandle, StagedHandle}; use arrow_array::{Array, RecordBatch, StringArray, UInt32Array}; use arrow_schema::SchemaRef; use futures::stream::StreamExt; -use omnigraph_compiler::catalog::EdgeType; use crate::db::manifest::{ RecoverySidecarHandle, SidecarKind, SidecarTablePin, new_sidecar, write_sidecar, @@ -98,6 +97,13 @@ pub(crate) struct MutationStaging { /// `pending`. Staged as one combined `stage_delete` per table at /// end-of-query (no inline HEAD advance) — see `stage_delete_table`. pub(crate) delete_predicates: HashMap>, + /// Ids removed per table, captured by the delete ops as they scan their + /// matched rows (so validation recounts the srcs a delete empties without + /// re-resolving the predicates). Disjoint from `pending` by D₂; flows into + /// the validation [`ChangeSet`](crate::validate::ChangeSet) via + /// [`to_changeset`](Self::to_changeset). The combined `stage_delete` at + /// commit still removes by predicate — these ids are validation-only. + pub(crate) deleted_ids: HashMap>, /// Strictest [`MutationOpKind`] seen per table within this query. Drives /// the op-kind-aware drift check in [`StagedMutation::commit_all`]: for /// tables whose first or any subsequent touch was a strict op @@ -227,6 +233,20 @@ impl MutationStaging { .push(predicate); } + /// Record ids removed by a delete op on `table_key`, captured from the op's + /// own scan, for validation (so cardinality recounts an emptied src). The + /// caller scans with a dedup filter that excludes prior-scheduled matches, so + /// no id is recorded twice across statements. + pub(crate) fn record_deleted_ids(&mut self, table_key: &str, ids: &[String]) { + if ids.is_empty() { + return; + } + self.deleted_ids + .entry(table_key.to_string()) + .or_default() + .extend(ids.iter().cloned()); + } + /// Delete predicates already recorded for `table_key` by earlier delete /// statements in this query. Read before recording the current statement's /// predicate so its `affected_*` count can exclude rows a prior statement @@ -249,9 +269,31 @@ impl MutationStaging { .unwrap_or(&[]) } - /// Accumulator mode for `table_key`, if this query has touched it. - pub(crate) fn pending_mode(&self, table_key: &str) -> Option { - self.pending.get(table_key).map(|p| p.mode) + /// Build the validation [`ChangeSet`](crate::validate::ChangeSet) for this + /// staging: every touched table's accumulated rows as the `changed` delta + /// (record-batch clone is Arc-cheap — no data copy). Shared by the mutation + /// and loader write paths so their validation input cannot drift. + pub(crate) fn to_changeset(&self) -> crate::validate::ChangeSet { + let mut changeset = crate::validate::ChangeSet::new(); + for table_key in self.pending.keys() { + let batches = self.pending_batches(table_key); + if batches.is_empty() { + continue; + } + let mut change = crate::validate::TableChange::default(); + change.changed.extend(batches.iter().cloned()); + changeset.insert(table_key.clone(), change); + } + // Deletes (disjoint from `pending` by D₂) carry their removed ids so the + // evaluator recounts the srcs a delete empties (`@card`) and sees removed + // rows for RI — the faithful change-set the merge path also builds. + for (table_key, ids) in &self.deleted_ids { + if ids.is_empty() { + continue; + } + changeset.entry(table_key.clone()).or_default().deleted_ids = ids.clone(); + } + changeset } /// Schema of the accumulated batches for `table_key`, or `None` if no @@ -307,6 +349,8 @@ impl MutationStaging { paths, pending, delete_predicates, + // Validation-only; consumed before staging, nothing to commit here. + deleted_ids: _, op_kinds, } = self; @@ -983,232 +1027,3 @@ fn dedupe_merge_batches_by_id( arrow_select::concat::concat_batches(schema, &sliced) .map_err(|e| OmniError::Lance(e.to_string())) } - -// ─── Cardinality helpers (shared by mutation + loader paths) ──────────────── - -/// Count edges per `src` value across committed (Lance scan) + pending -/// (in-memory). Caller supplies an opened committed dataset so the -/// mutation path (which already has one) and the loader path (which -/// opens via snapshot) share the same body. For overwrite staging, the -/// pending batches are the replacement table image, so committed rows are -/// intentionally skipped. -/// -/// `dedupe_key_column` controls whether committed rows are shadowed by -/// pending: -/// - `None` — every committed row counts, every pending row counts. -/// Correct when committed and pending cannot share a primary key -/// (engine inserts always use fresh ULID edge ids; loader Append -/// mode uses fresh ids too). -/// - `Some(col)` — committed rows whose `col` value also appears in any -/// pending batch are EXCLUDED from the committed count, so a Merge-mode -/// load that *updates* an existing edge (potentially changing its -/// `src`) counts the post-update row exactly once. Without this, -/// `LoadMode::Merge` double-counts. -pub(crate) async fn count_src_per_edge( - db: &crate::db::Omnigraph, - committed_ds: &SnapshotHandle, - table_key: &str, - staging: &MutationStaging, - dedupe_key_column: Option<&str>, -) -> Result> { - let mut counts: HashMap = HashMap::new(); - - let pending_batches = staging.pending_batches(table_key); - - // Collect pending key values (for shadow-on-merge dedupe). Only when - // dedupe is requested AND there's anything pending. - let pending_keys: Option> = match dedupe_key_column { - Some(col) if !pending_batches.is_empty() => { - let mut set = HashSet::new(); - for batch in pending_batches { - if let Some(arr) = batch - .column_by_name(col) - .and_then(|c| c.as_any().downcast_ref::()) - { - for i in 0..arr.len() { - if arr.is_valid(i) { - set.insert(arr.value(i).to_string()); - } - } - } - } - Some(set) - } - _ => None, - }; - - let replace_committed = staging.pending_mode(table_key) == Some(PendingMode::Overwrite); - if !replace_committed { - // Committed side: scan `src` plus the dedupe key column when set, so - // we can both count and shadow in one pass. - let projection: Vec<&str> = match dedupe_key_column { - Some(col) if pending_keys.as_ref().is_some_and(|s| !s.is_empty()) => vec!["src", col], - _ => vec!["src"], - }; - let committed = db - .storage() - .scan(committed_ds, Some(&projection), None, None) - .await?; - for batch in &committed { - let srcs = batch - .column_by_name("src") - .ok_or_else(|| OmniError::Lance("missing 'src' column on edge table".into()))? - .as_any() - .downcast_ref::() - .ok_or_else(|| OmniError::Lance("'src' column is not Utf8".into()))?; - // Optional shadow-key column (only present when dedupe is on). - let key_arr = match (&pending_keys, dedupe_key_column) { - (Some(set), Some(col)) if !set.is_empty() => batch - .column_by_name(col) - .and_then(|c| c.as_any().downcast_ref::()), - _ => None, - }; - for i in 0..srcs.len() { - if !srcs.is_valid(i) { - continue; - } - // Shadow this committed row if its key is in pending. - if let (Some(arr), Some(set)) = (key_arr, pending_keys.as_ref()) { - if arr.is_valid(i) && set.contains(arr.value(i)) { - continue; - } - } - *counts.entry(srcs.value(i).to_string()).or_insert(0) += 1; - } - } - } - - // Pending side: walk in-memory batches for `src`. When dedupe is on, - // collapse rows that share `dedupe_key_column` to their last occurrence - // — mirrors `dedupe_merge_batches_by_id`'s last-write-wins applied at - // finalize time, so cardinality counts what `commit_staged` will - // actually publish, not raw input duplicates. - // - // Without this, a Merge-mode load whose input JSONL has two rows with - // the same edge id would be double-counted here, even though the - // finalize-time dedupe would collapse them to one. The result: spurious - // `@card` violations on perfectly valid Merge inputs. - match dedupe_key_column { - Some(key_col) => count_pending_src_with_dedupe(pending_batches, key_col, &mut counts)?, - None => count_pending_src_naive(pending_batches, &mut counts), - } - - Ok(counts) -} - -/// Count pending edges per `src` with NO dedup. Correct when caller -/// guarantees pending rows have unique primary keys (engine inserts via -/// fresh ULID; loader Append mode). -fn count_pending_src_naive(pending_batches: &[RecordBatch], counts: &mut HashMap) { - for batch in pending_batches { - let Some(col) = batch.column_by_name("src") else { - continue; - }; - let Some(srcs) = col.as_any().downcast_ref::() else { - continue; - }; - for i in 0..srcs.len() { - if srcs.is_valid(i) { - *counts.entry(srcs.value(i).to_string()).or_insert(0) += 1; - } - } - } -} - -/// Count pending edges per `src` after deduping rows that share -/// `dedupe_key_column`. Last occurrence wins (mirrors -/// `dedupe_merge_batches_by_id`'s walk-in-reverse contract). Required for -/// `LoadMode::Merge` where the same edge id may appear multiple times in -/// one load and finalize will collapse them to the last value. -fn count_pending_src_with_dedupe( - pending_batches: &[RecordBatch], - dedupe_key_column: &str, - counts: &mut HashMap, -) -> Result<()> { - // Walk in reverse, track seen keys, keep one (key, src) pair per key. - let mut seen: HashSet = HashSet::new(); - let mut kept_srcs: Vec = Vec::new(); - for batch in pending_batches.iter().rev() { - let Some(key_col) = batch.column_by_name(dedupe_key_column) else { - // Pending batch is missing the key column. By construction - // this is unreachable: callers in dedupe mode always push - // batches whose schema contains the key (loader Merge mode - // builds via build_edge_batch which always emits `id`; the - // append_batch schema-compatibility check at the call site - // would also reject a heterogeneous mix). If it ever fires - // it's a programmer error — fail loudly rather than skip - // counting (which would let `@card` violations slip). - return Err(OmniError::manifest_internal(format!( - "count_pending_src_with_dedupe: pending batch missing dedup key column '{}' \ - (schema-compat check at append_batch should have rejected this)", - dedupe_key_column - ))); - }; - let key_arr = key_col - .as_any() - .downcast_ref::() - .ok_or_else(|| { - OmniError::Lance(format!( - "count_src_per_edge: pending '{}' column is not Utf8", - dedupe_key_column - )) - })?; - let src_arr = batch - .column_by_name("src") - .and_then(|c| c.as_any().downcast_ref::()); - let Some(srcs) = src_arr else { - continue; - }; - for i in (0..batch.num_rows()).rev() { - if !srcs.is_valid(i) { - continue; - } - // NULL key: keep (NULL != NULL semantics — every NULL counts). - if !key_arr.is_valid(i) { - kept_srcs.push(srcs.value(i).to_string()); - continue; - } - let key = key_arr.value(i); - if seen.insert(key.to_string()) { - kept_srcs.push(srcs.value(i).to_string()); - } - } - } - for src in kept_srcs { - *counts.entry(src).or_insert(0) += 1; - } - Ok(()) -} - -/// Apply `@card(min..max)` bounds to a per-source count map. -/// -/// Both bounds are checked. The `min` check produces a misleading error -/// during a per-op insert mid-query (a bound of `2..` requires both -/// edges to be inserted before validation passes), but the historical -/// behavior was to enforce min per-op anyway — keeping users from -/// accidentally publishing a graph that violates the schema. Consumers -/// that need end-of-query semantics call this from after all edge ops -/// are accumulated (the loader does, via Phase 3). -pub(crate) fn enforce_cardinality_bounds( - edge_type: &EdgeType, - counts: &HashMap, -) -> Result<()> { - let card = &edge_type.cardinality; - for (src, count) in counts { - if let Some(max) = card.max { - if *count > max { - return Err(OmniError::manifest(format!( - "@card violation on edge {}: source '{}' has {} edges (max {})", - edge_type.name, src, count, max - ))); - } - } - if *count < card.min { - return Err(OmniError::manifest(format!( - "@card violation on edge {}: source '{}' has {} edges (min {})", - edge_type.name, src, count, card.min - ))); - } - } - Ok(()) -} diff --git a/crates/omnigraph/src/lib.rs b/crates/omnigraph/src/lib.rs index 7dd71353..c873dd86 100644 --- a/crates/omnigraph/src/lib.rs +++ b/crates/omnigraph/src/lib.rs @@ -20,3 +20,4 @@ pub mod runtime_cache; pub mod storage; pub mod storage_layer; pub mod table_store; +pub(crate) mod validate; diff --git a/crates/omnigraph/src/loader/mod.rs b/crates/omnigraph/src/loader/mod.rs index 075724dd..c8e08a47 100644 --- a/crates/omnigraph/src/loader/mod.rs +++ b/crates/omnigraph/src/loader/mod.rs @@ -1,4 +1,4 @@ -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::io::{BufRead, BufReader, Cursor}; use std::sync::Arc; @@ -476,12 +476,7 @@ async fn load_jsonl_reader( for (type_name, rows) in &node_rows { let node_type = &catalog.node_types[type_name]; let batch = build_node_batch(node_type, rows)?; - validate_value_constraints(&batch, node_type)?; - validate_enum_constraints(&batch, &node_type.properties, type_name)?; - let unique_groups = unique_constraint_groups_for_node(node_type); - if !unique_groups.is_empty() { - enforce_unique_constraints_intra_batch(&batch, type_name, &unique_groups)?; - } + // Validation (value/enum/unique) runs end-of-load via the evaluator. let loaded_count = batch.num_rows(); let table_key = format!("node:{}", type_name); let _entry = snapshot @@ -512,52 +507,14 @@ async fn load_jsonl_reader( result.nodes_loaded.insert(type_name, loaded_count); } - // Phase 2c: Validate edge referential integrity — every src/dst must - // reference an existing node ID in the appropriate type. For - // Append/Merge the lookup unions snapshot-committed IDs with the - // in-memory pending batches. For Overwrite, a touched node table's - // pending batch is the replacement image, so committed rows are not - // included for that table. - for (edge_name, rows) in &edge_rows { - let edge_type = &catalog.edge_types[edge_name]; - let from_ids = - collect_node_ids_with_pending(db, branch, &edge_type.from_type, &staging).await?; - let to_ids = - collect_node_ids_with_pending(db, branch, &edge_type.to_type, &staging).await?; - - for (i, (src, dst, _)) in rows.iter().enumerate() { - if !from_ids.contains(src.as_str()) { - return Err(OmniError::manifest(format!( - "edge {} row {}: src '{}' not found in {}", - edge_name, - i + 1, - src, - edge_type.from_type - ))); - } - if !to_ids.contains(dst.as_str()) { - return Err(OmniError::manifest(format!( - "edge {} row {}: dst '{}' not found in {}", - edge_name, - i + 1, - dst, - edge_type.to_type - ))); - } - } - } - - // Phase 2d: build edge batches. + // Phase 2d: build edge batches. Edge referential integrity (and the rest) + // runs end-of-load via the unified evaluator, below. let mut prepared_edges: Vec<(String, String, RecordBatch, usize)> = Vec::with_capacity(edge_rows.len()); for (edge_name, rows) in &edge_rows { let edge_type = &catalog.edge_types[edge_name]; let batch = build_edge_batch(edge_type, rows)?; - validate_enum_constraints(&batch, &edge_type.properties, edge_name)?; - let unique_groups = unique_constraint_groups_for_edge(edge_type); - if !unique_groups.is_empty() { - enforce_unique_constraints_intra_batch(&batch, edge_name, &unique_groups)?; - } + // Validation (enum/unique, edge-RI, @card) runs end-of-load via the evaluator. let loaded_count = batch.num_rows(); let table_key = format!("edge:{}", edge_name); let _entry = snapshot @@ -585,18 +542,40 @@ async fn load_jsonl_reader( result.edges_loaded.insert(edge_name, loaded_count); } - // Phase 3: Validate edge cardinality constraints (before commit — - // invalid data must not be committed). The helper scans committed - // edges via Lance + iterates pending edges in-memory; for Overwrite it - // treats the pending edge batches as the replacement table image. - for (edge_name, _) in &edge_rows { - let edge_type = &catalog.edge_types[edge_name]; - let table_key = format!("edge:{}", edge_name); - validate_edge_cardinality_with_pending_loader( - db, branch, edge_type, &table_key, &staging, mode, - ) - .await?; + // Phase 3: end-of-load validation — one unified evaluator pass over the + // accumulated staging (value/enum, uniqueness incl. cross-version, edge-RI, + // cardinality) against the pinned pre-load base. `Overwrite` validates each + // touched table as its whole new image (that table's committed view empty), + // but is PER-TABLE — a table absent from the batch keeps `base`, so an + // edges-only overwrite still resolves RI against committed nodes; + // `Append`/`Merge` keep `base` everywhere. This shares the evaluator with the + // mutation + merge paths, so the surfaces cannot drift. + let mut changeset = staging.to_changeset(); + // Overwrite replaces each touched table; a committed row absent from the new + // batch is REMOVED but is not in `to_changeset` (which only records the new + // batch). Express those removals as `deleted_ids` so edge-RI (path-b) and + // cardinality recompute against them — e.g. overwriting `node:Person` to drop + // Bob while a retained `edge:Knows(Alice->Bob)` would otherwise publish an + // orphan. (Per-table, like the rest of Overwrite handling.) + if mode == LoadMode::Overwrite { + let keys: Vec = changeset.keys().cloned().collect(); + for table_key in keys { + let removed = crate::validate::overwrite_removed_ids( + &snapshot, + &table_key, + changeset.get(&table_key).expect("key from this changeset"), + ) + .await?; + if !removed.is_empty() { + changeset + .get_mut(&table_key) + .expect("key from this changeset") + .deleted_ids = removed; + } + } } + let committed = crate::validate::CommittedState::load(&snapshot, mode, &changeset); + crate::validate::validate_changeset(&changeset, &committed, &catalog).await?; // Phase 4: Atomic manifest commit with publisher-level OCC. let staged = staging @@ -1361,61 +1340,6 @@ pub(crate) fn validate_enum_constraints( Ok(()) } -/// Detect duplicate values within a single `RecordBatch` for any of the -/// `unique_constraints` groups. Each group is a list of one or more columns -/// that together form a uniqueness key: a violation occurs when two rows share -/// the same tuple of values across *all* columns in a group, so a composite -/// `@unique(a, b)` only conflicts when both `a` and `b` match. Returns an -/// error on the first duplicate found. -/// -/// Rows where any column in a group is null are exempt (standard SQL semantics -/// for uniqueness over nullable columns), as is any group whose columns are -/// not all present in the batch (e.g. a partial-schema load). -/// -/// Note: this only catches duplicates *within* the batch. Cross-batch -/// uniqueness against already-committed rows is not enforced here — that -/// requires a dataset scan and is tracked separately. -pub(crate) fn enforce_unique_constraints_intra_batch( - batch: &RecordBatch, - type_name: &str, - unique_constraints: &[Vec], -) -> Result<()> { - for columns in unique_constraints { - // Resolve the group's columns once. A group whose columns aren't all - // present in this batch is skipped (e.g. a partial-schema load). - let Some(group_columns) = columns - .iter() - .map(|name| { - batch - .schema() - .index_of(name) - .ok() - .map(|i| batch.column(i).clone()) - }) - .collect::>>() - else { - continue; - }; - let mut seen: HashMap, usize> = HashMap::new(); - for row in 0..batch.num_rows() { - let Some(key) = composite_unique_key(&group_columns, row)? else { - continue; - }; - if let Some(prev_row) = seen.insert(key.clone(), row) { - return Err(OmniError::manifest(format!( - "@unique violation on {}.{}: value '{}' appears in rows {} and {}", - type_name, - format_tuple(columns), - format_tuple(&key), - prev_row, - row - ))); - } - } - } - Ok(()) -} - /// Build the composite uniqueness key for `row` over a constraint group's /// already-resolved columns (in declaration order). /// @@ -1429,9 +1353,10 @@ pub(crate) fn enforce_unique_constraints_intra_batch( /// - `Ok(Some(tuple))` otherwise. /// - `Err(..)` propagated from [`unique_key_scalar`] on an un-keyable value. /// -/// Shared by the intake path (`enforce_unique_constraints_intra_batch`) and the -/// branch-merge path (`exec/merge.rs::update_unique_constraints`) so the two -/// derive identical keys and cannot drift on separator or scalar conversion. +/// Shared by every write surface through the unified validation evaluator +/// (`crate::validate::evaluate_unique`, used by the loader, mutation, and +/// branch-merge paths) so they derive identical keys and cannot drift on +/// separator or scalar conversion. pub(crate) fn composite_unique_key( group_columns: &[ArrayRef], row: usize, @@ -1449,7 +1374,7 @@ pub(crate) fn composite_unique_key( /// Render a constraint's column tuple for error messages: a single item as /// `col`, a composite as `(a, b)`. Used for both the column list and the /// offending value tuple, which share the same shape. -fn format_tuple(items: &[String]) -> String { +pub(crate) fn format_tuple(items: &[String]) -> String { match items { [single] => single.clone(), _ => format!("({})", items.join(", ")), @@ -1517,32 +1442,6 @@ fn unique_key_scalar(array: &ArrayRef, row: usize) -> Result> { ))) } -/// Build the list of uniqueness constraint groups to enforce on a node type. -/// Each group is the column tuple of one constraint. Includes every -/// `@unique(...)` constraint (from `NodeType.unique_constraints`) and the -/// `@key` (which implies uniqueness over its column tuple). Grouping is -/// preserved so a composite `@unique(a, b)` is enforced as a composite key -/// rather than degraded into independent single-field checks. -pub(crate) fn unique_constraint_groups_for_node( - node_type: &omnigraph_compiler::catalog::NodeType, -) -> Vec> { - let mut groups: Vec> = node_type.unique_constraints.clone(); - if let Some(key) = &node_type.key - && !groups.contains(key) - { - groups.push(key.clone()); - } - groups -} - -/// Same as [`unique_constraint_groups_for_node`] but for an edge type (edges -/// have no `@key`). -pub(crate) fn unique_constraint_groups_for_edge( - edge_type: &omnigraph_compiler::catalog::EdgeType, -) -> Vec> { - edge_type.unique_constraints.clone() -} - fn extract_numeric_value(col: &ArrayRef, row: usize) -> Option { use arrow_array::{ Array, Float32Array, Float64Array, Int32Array, Int64Array, UInt32Array, UInt64Array, @@ -1576,130 +1475,6 @@ fn literal_value_to_f64(v: &omnigraph_compiler::catalog::LiteralValue) -> f64 { } } -// ─── Edge cardinality validation ───────────────────────────────────────────── - -/// Validate edge `@card` cardinality with in-memory pending edges visible. -/// -/// Loader-level analog to `exec::mutation::validate_edge_cardinality_with_pending`: -/// opens the committed dataset at the pre-load snapshot version, then -/// delegates to the shared `count_src_per_edge` + `enforce_cardinality_bounds` -/// helpers in `exec::staging`. Used by every load mode; for `LoadMode::Overwrite` -/// it treats the pending edge batches as the replacement table image (the -/// committed rows are being replaced, so only the pending set is counted). -/// -/// `mode` controls dedup behavior. `LoadMode::Merge` passes `Some("id")` -/// so committed edges that the load is *updating* (same edge id, -/// possibly changed `src`) are not double-counted. `LoadMode::Append` -/// passes `None` because each line generates a fresh ULID id that -/// never collides with committed. -async fn validate_edge_cardinality_with_pending_loader( - db: &Omnigraph, - branch: Option<&str>, - edge_type: &omnigraph_compiler::catalog::EdgeType, - table_key: &str, - staging: &MutationStaging, - mode: LoadMode, -) -> Result<()> { - if edge_type.cardinality.is_default() { - return Ok(()); - } - let snapshot = db.snapshot_for_branch(branch).await?; - let Some(entry) = snapshot.entry(table_key) else { - // No manifest entry — table doesn't exist yet. Pending-only is - // fine; the helper handles empty committed scans. - return Ok(()); - }; - let ds = db - .open_dataset_at_state( - &entry.table_path, - entry.table_branch.as_deref(), - entry.table_version, - ) - .await?; - let dedupe_key = match mode { - LoadMode::Merge => Some("id"), - LoadMode::Append | LoadMode::Overwrite => None, - }; - let counts = - crate::exec::staging::count_src_per_edge(db, &ds, table_key, staging, dedupe_key).await?; - crate::exec::staging::enforce_cardinality_bounds(edge_type, &counts) -} - -/// Collect all valid node IDs for a given type, with in-memory pending -/// node inserts visible. Used by the staged loader's Phase 2c -/// referential-integrity validation. -/// -/// Union of: -/// - IDs from the staged loader's pending batches (in-memory; just-staged -/// inserts of this type) -/// - IDs from the committed sub-table at the pre-load snapshot version -/// -/// For `LoadMode::Overwrite`, if the node table is touched then the pending -/// batches are the replacement image. In that case committed IDs are not -/// included, so edge RI is validated against exactly what the overwrite will -/// publish. -async fn collect_node_ids_with_pending( - db: &Omnigraph, - branch: Option<&str>, - type_name: &str, - staging: &MutationStaging, -) -> Result> { - let mut ids = HashSet::new(); - let table_key = format!("node:{}", type_name); - - // From staging.pending: walk the in-memory accumulator's id column. - for batch in staging.pending_batches(&table_key) { - if let Some(col) = batch.column_by_name("id") { - if let Some(arr) = col.as_any().downcast_ref::() { - for i in 0..arr.len() { - if arr.is_valid(i) { - ids.insert(arr.value(i).to_string()); - } - } - } - } - } - - if staging.pending_mode(&table_key) == Some(PendingMode::Overwrite) { - return Ok(ids); - } - - // From the committed Lance sub-table at the pre-load snapshot version. - let snapshot = db.snapshot_for_branch(branch).await?; - let Some(entry) = snapshot.entry(&table_key) else { - return Ok(ids); - }; - let ds = db - .open_dataset_at_state( - &entry.table_path, - entry.table_branch.as_deref(), - entry.table_version, - ) - .await?; - - let batches = db.storage().scan(&ds, Some(&["id"]), None, None).await?; - - for batch in &batches { - let id_col = batch - .column_by_name("id") - .ok_or_else(|| OmniError::Lance("missing 'id' column".into()))? - .as_any() - .downcast_ref::() - .ok_or_else(|| OmniError::Lance("'id' column is not Utf8".into()))?; - for i in 0..batch.num_rows() { - // Defensive: `id` is the @key column on every node type and - // is non-nullable by schema, but a committed-row corruption - // (or future schema change) could surface a NULL. Skip - // rather than insert "" — pending-side does the same. - if id_col.is_valid(i) { - ids.insert(id_col.value(i).to_string()); - } - } - } - - Ok(ids) -} - #[cfg(test)] mod tests { use super::*; diff --git a/crates/omnigraph/src/storage_layer.rs b/crates/omnigraph/src/storage_layer.rs index 9dcfde4b..fb4d34b7 100644 --- a/crates/omnigraph/src/storage_layer.rs +++ b/crates/omnigraph/src/storage_layer.rs @@ -240,13 +240,6 @@ pub trait TableStorage: sealed::Sealed + Send + Sync + Debug { branch: Option<&str>, ) -> Result; - async fn open_dataset_at_state( - &self, - table_path: &str, - branch: Option<&str>, - version: u64, - ) -> Result; - async fn fork_branch_from_state( &self, dataset_uri: &str, @@ -515,17 +508,6 @@ impl TableStorage for TableStore { .map(SnapshotHandle::new) } - async fn open_dataset_at_state( - &self, - table_path: &str, - branch: Option<&str>, - version: u64, - ) -> Result { - TableStore::open_dataset_at_state(self, table_path, branch, version) - .await - .map(SnapshotHandle::new) - } - async fn fork_branch_from_state( &self, dataset_uri: &str, diff --git a/crates/omnigraph/src/table_store.rs b/crates/omnigraph/src/table_store.rs index 248ec7bb..4a695704 100644 --- a/crates/omnigraph/src/table_store.rs +++ b/crates/omnigraph/src/table_store.rs @@ -284,20 +284,6 @@ impl TableStore { } } - pub async fn open_dataset_at_state( - &self, - table_path: &str, - branch: Option<&str>, - version: u64, - ) -> Result { - let ds = self - .open_dataset_head(&self.dataset_uri(table_path), branch) - .await?; - ds.checkout_version(version) - .await - .map_err(|e| OmniError::Lance(e.to_string())) - } - pub fn ensure_expected_version( &self, ds: &Dataset, diff --git a/crates/omnigraph/src/validate.rs b/crates/omnigraph/src/validate.rs new file mode 100644 index 00000000..5d750c8d --- /dev/null +++ b/crates/omnigraph/src/validate.rs @@ -0,0 +1,1085 @@ +//! Unified, catalog-derived integrity validation. +//! +//! Validation invariants (value/enum, uniqueness, edge referential integrity, +//! cardinality) are declared once in the schema and should be *evaluated* once, +//! not re-implemented per write surface. Historically the merge path +//! (`exec/merge.rs`) carried its own copy of these checks, parallel to the write +//! path (`loader`/`exec/mutation.rs`); the two drifted (merge validated +//! `@range`/`@check` but not enum membership), which is the class of bug this +//! module closes. +//! +//! The evaluator does NOT reimplement the leaf checks — it orchestrates the +//! existing ones (`loader::validate_value_constraints`, +//! `loader::validate_enum_constraints`, ...) over a per-table [`ChangeSet`], so +//! the surfaces that adopt it cannot diverge. All three write surfaces — +//! branch-merge (`exec/merge.rs`), mutation (`exec/mutation.rs`), and bulk load +//! (`loader`) — now route their integrity checks through this one evaluator, so +//! the drift class above is unrepresentable. +//! +//! Δ-scoping: checks run over the *changed* rows (the merge/write delta), not the +//! whole table. Row-local checks (value/enum) need only the changed rows because +//! unchanged rows were validated when written. Cross-row/cross-table checks +//! (uniqueness, RI, cardinality) evaluate the delta against an index-backed view +//! of committed state (the merge target snapshot, or the write's pinned base / +//! live HEAD depending on the surface). + +use std::collections::{HashMap, HashSet}; + +use arrow_array::{Array, RecordBatch, StringArray}; +use datafusion::prelude::{Expr, col, lit}; +use datafusion::scalar::ScalarValue; +use futures::TryStreamExt; +use lance::Dataset; +use omnigraph_compiler::catalog::{Catalog, EdgeType}; + +use crate::db::{Omnigraph, Snapshot}; +use crate::error::{MergeConflict, MergeConflictKind, OmniError, Result}; +use crate::loader::{ + composite_unique_key, format_tuple, validate_enum_constraints, validate_value_constraints, +}; +use crate::table_store::TableStore; + +/// A single integrity violation, surface-neutral. Maps to the merge path's +/// [`MergeConflict`] today via [`Violation::into_merge_conflict`]; a write-path +/// `into_omni_error` mapping is added when the write path migrates. +#[derive(Debug, Clone)] +pub(crate) struct Violation { + pub table_key: String, + pub row_id: Option, + pub kind: MergeConflictKind, + pub message: String, +} + +impl Violation { + pub(crate) fn into_merge_conflict(self) -> MergeConflict { + MergeConflict { + table_key: self.table_key, + row_id: self.row_id, + kind: self.kind, + message: self.message, + } + } + + /// Map to the write-path surface error. The message already matches the + /// write path's text (`validators.rs` asserts via `.contains`). + pub(crate) fn into_omni_error(self) -> OmniError { + OmniError::manifest(self.message) + } +} + +/// Per-table change produced by a write or a merge: the rows that were added or +/// changed (as batches, so row-local checks scan only them) plus the ids +/// removed. The unit of Δ-scoping. +#[derive(Debug, Default)] +pub(crate) struct TableChange { + /// Rows new in the result (absent from base). + pub added: Vec, + /// Rows present before but with changed values. + pub changed: Vec, + /// Ids removed in the result. + pub deleted_ids: Vec, +} + +impl TableChange { + /// Batches carrying field values a row-local constraint must check + /// (`added ∪ changed`). Unchanged rows were validated at write time. + pub fn value_batches(&self) -> impl Iterator { + self.added.iter().chain(self.changed.iter()) + } +} + +/// Per-table changes keyed by `table_key` (`node:Type` / `edge:Type`). +pub(crate) type ChangeSet = HashMap; + +/// Row-local value validation — `@range`/`@check` (nodes) and enum membership +/// (nodes **and** edges) — Δ-scoped to the changed rows. Reuses the loader +/// leaves so the merge and write paths share one implementation; including the +/// enum check here is what closes the merge-vs-write drift. +pub(crate) fn evaluate_value_constraints(changeset: &ChangeSet, catalog: &Catalog) -> Vec { + let mut violations = Vec::new(); + for (table_key, change) in changeset { + if let Some(type_name) = table_key.strip_prefix("node:") { + let Some(node_type) = catalog.node_types.get(type_name) else { + continue; + }; + for batch in change.value_batches() { + if let Err(err) = validate_value_constraints(batch, node_type) { + violations.push(value_violation(table_key, err)); + } + if let Err(err) = validate_enum_constraints(batch, &node_type.properties, type_name) { + violations.push(value_violation(table_key, err)); + } + } + } else if let Some(type_name) = table_key.strip_prefix("edge:") { + let Some(edge_type) = catalog.edge_types.get(type_name) else { + continue; + }; + // Edges carry no @range/@check (NodeType-only), but their properties + // can be enum-typed — the check the merge path was missing. + for batch in change.value_batches() { + if let Err(err) = validate_enum_constraints(batch, &edge_type.properties, type_name) { + violations.push(value_violation(table_key, err)); + } + } + } + } + violations +} + +/// Wrap a leaf-check error as a value-constraint [`Violation`]. The message is +/// the leaf's own text (`err.to_string()`), matching what the merge path +/// previously surfaced — error text is a contract. +fn value_violation(table_key: &str, err: OmniError) -> Violation { + Violation { + table_key: table_key.to_string(), + row_id: None, + kind: MergeConflictKind::ValueConstraintViolation, + message: err.to_string(), + } +} + +// ── Cross-row / cross-table checks (uniqueness, edge-RI, cardinality) ───────── +// +// These evaluate the merge delta against committed state. Because adopt is only +// chosen when `same_manifest_state(base, target)` (target unchanged since fork), +// the merged content of EVERY table is `target ± delta`. So committed lookups go +// to the indexed TARGET table and the in-memory delta is applied on top — never +// the source table or the unindexed staged temp (which carries no index). + +/// A declared integrity constraint, derived from the catalog. Mirrors the +/// `node_prop_index_kind` chokepoint: adding a kind is one variant + one arm in +/// [`evaluate`], run on every surface that adopts the evaluator. +#[derive(Debug, Clone)] +pub(crate) enum Constraint { + /// Row-local value/enum validation across the whole change-set (one entry + /// covers every table; handled by [`evaluate_value_constraints`]). + Value, + Unique { + table_key: String, + columns: Vec, + /// True for the `@key` group: it is id-backed, so a committed holder of a + /// key value is always the same row (an upsert), never a cross-version + /// duplicate. Intra-delta dedup suffices; the committed lookup is skipped. + is_key: bool, + }, + EdgeRi { + table_key: String, + from_type: String, + to_type: String, + }, + Cardinality { + table_key: String, + }, +} + +/// Derive the runtime constraint set from the catalog (the schema's declared +/// invariants). One `Value` plus one entry per `@unique` group, edge-RI, and +/// `@card` edge. +pub(crate) fn constraints_for(catalog: &Catalog) -> Vec { + let mut out = vec![Constraint::Value]; + for (name, node_type) in &catalog.node_types { + let table_key = format!("node:{name}"); + // `@key` is id-backed: cross-version duplication is impossible (the key + // IS the identity), so it needs only intra-delta dedup — `is_key: true` + // tells the evaluator to skip the committed lookup. + if let Some(key) = &node_type.key { + out.push(Constraint::Unique { + table_key: table_key.clone(), + columns: key.clone(), + is_key: true, + }); + } + // `@unique` (non-key) groups CAN collide cross-version → committed lookup. + for columns in &node_type.unique_constraints { + if Some(columns) == node_type.key.as_ref() { + continue; // same column tuple as @key — already covered above. + } + out.push(Constraint::Unique { + table_key: table_key.clone(), + columns: columns.clone(), + is_key: false, + }); + } + } + for (name, edge_type) in &catalog.edge_types { + let table_key = format!("edge:{name}"); + // Edges have no `@key`; every `@unique` group needs the committed lookup. + for columns in &edge_type.unique_constraints { + out.push(Constraint::Unique { + table_key: table_key.clone(), + columns: columns.clone(), + is_key: false, + }); + } + out.push(Constraint::EdgeRi { + table_key: table_key.clone(), + from_type: edge_type.from_type.clone(), + to_type: edge_type.to_type.clone(), + }); + out.push(Constraint::Cardinality { table_key }); + } + out +} + +/// Index-backed view of committed target state for the merge delta's lookups. +/// Every method reads the (indexed) target table via a structured `filter_expr` +/// so Lance serves it from the BTREE (index-search → take) rather than a full +/// scan — with the documented uncovered-fragment-tail caveat (a stale index +/// degrades to a tail scan; correctness is unaffected). +pub(crate) struct CommittedState<'a> { + /// The committed view for existence / uniqueness / cardinality lookups: the + /// merge target snapshot, the write path's pinned base, or the loader's + /// pinned pre-load base. `None` means EMPTY — an `Overwrite` load, where the + /// batch is the whole new image and no prior committed row survives. + committed: Option<&'a Snapshot>, + /// Tables whose committed view is EMPTY because this op replaces them: the + /// tables an `Overwrite` load touches. `Overwrite` is PER-TABLE (a table + /// absent from the load batch is retained), so this is the set of touched + /// tables, not a global flag — an edges-only overwrite still sees committed + /// nodes for RI. Empty on the merge / mutation / append / merge-load paths. + overwritten: HashSet, + /// Write path only: open edge tables at LIVE HEAD for `@card` (the #298 + /// stale-handle fix). `None` on the merge and load paths — there the snapshot + /// (or empty) is the committed view for every check. + live: Option<(&'a Omnigraph, Option<&'a str>)>, +} + +impl<'a> CommittedState<'a> { + /// Merge path: read the merge target snapshot for every lookup. + pub(crate) fn merge(target: &'a Snapshot) -> Self { + Self { + committed: Some(target), + overwritten: HashSet::new(), + live: None, + } + } + + /// Write path: existence/uniqueness read `committed` (the write's pinned + /// base); cardinality reads LIVE HEAD per edge table via `db` (#298). + pub(crate) fn write(committed: &'a Snapshot, db: &'a Omnigraph, branch: Option<&'a str>) -> Self { + Self { + committed: Some(committed), + overwritten: HashSet::new(), + live: Some((db, branch)), + } + } + + /// Bulk-load path: validate against the pinned pre-load `base` (never live + /// HEAD — the loader pins its base, unlike the mutation `@card` #298 case). + /// `Overwrite` replaces only the touched tables (PER-TABLE), so the committed + /// view of each table in `changeset` is EMPTY — the batch is that table's + /// whole new image — while tables absent from the batch keep `base` (an + /// edges-only overwrite still resolves RI against committed nodes). + /// `Append`/`Merge` keep `base` for every table. + pub(crate) fn load( + base: &'a Snapshot, + mode: crate::loader::LoadMode, + changeset: &ChangeSet, + ) -> Self { + let overwritten = match mode { + crate::loader::LoadMode::Overwrite => changeset.keys().cloned().collect(), + crate::loader::LoadMode::Append | crate::loader::LoadMode::Merge => HashSet::new(), + }; + Self { + committed: Some(base), + overwritten, + live: None, + } + } + + async fn open(&self, table_key: &str) -> Result> { + if self.overwritten.contains(table_key) { + return Ok(None); + } + let Some(committed) = self.committed else { + return Ok(None); + }; + match committed.entry(table_key) { + Some(_) => Ok(Some(committed.open(table_key).await?)), + None => Ok(None), + } + } + + /// Open an edge table for cardinality counting: LIVE HEAD on the write path + /// (so an edge a concurrent writer committed since the base was pinned is + /// counted — #298), the committed snapshot otherwise. `None` if there is no + /// committed view (Overwrite load) or the table isn't in it. + async fn open_cardinality(&self, table_key: &str) -> Result> { + if self.overwritten.contains(table_key) { + return Ok(None); + } + let Some(committed) = self.committed else { + return Ok(None); + }; + let Some(entry) = committed.entry(table_key) else { + return Ok(None); + }; + match self.live { + Some((db, branch)) => { + let full_path = db.storage().dataset_uri(&entry.table_path); + let handle = db.storage().open_dataset_head(&full_path, branch).await?; + Ok(Some(handle.dataset().clone())) + } + None => Ok(Some(committed.open(table_key).await?)), + } + } + + /// Which of `ids` exist as committed rows in `table_key` (by `id`). + async fn existing_ids(&self, table_key: &str, ids: &[String]) -> Result> { + let Some(ds) = self.open(table_key).await? else { + return Ok(HashSet::new()); + }; + if ids.is_empty() { + return Ok(HashSet::new()); + } + let expr = col("id").in_list(ids.iter().map(|k| lit(k.clone())).collect(), false); + let batches = scan_filtered(&ds, &["id"], expr).await?; + let mut present = HashSet::new(); + for batch in &batches { + let column = string_col(batch, "id")?; + for i in 0..column.len() { + if !column.is_null(i) { + present.insert(column.value(i).to_string()); + } + } + } + Ok(present) + } + + /// Ids of committed rows in `table_key` whose `columns` tuple equals `key`. + /// Used to detect a cross-version unique collision (the one constraint the + /// write path does not enforce, so it is load-bearing at merge). + async fn unique_holders( + &self, + table_key: &str, + columns: &[String], + key_values: &[ScalarValue], + ) -> Result> { + let Some(ds) = self.open(table_key).await? else { + return Ok(Vec::new()); + }; + // AND of per-column equality so each indexed column is served by its + // BTREE (a non-indexed `@unique` column falls back to a scan). The + // literal is TYPED (built from the row's Arrow column), so the + // pushed-down filter compares like-typed. A stringified key would push a + // Utf8 literal against a typed column — a coercion error on Date/Bool + // (breaking every write) or a silent miss on Float. + let mut expr: Option = None; + for (column, value) in columns.iter().zip(key_values.iter()) { + let eq = col(column.as_str()).eq(lit(value.clone())); + expr = Some(match expr { + Some(acc) => acc.and(eq), + None => eq, + }); + } + let Some(expr) = expr else { + return Ok(Vec::new()); + }; + let batches = scan_filtered(&ds, &["id"], expr).await?; + let mut ids = Vec::new(); + for batch in &batches { + let column = string_col(batch, "id")?; + for i in 0..column.len() { + if !column.is_null(i) { + ids.push(column.value(i).to_string()); + } + } + } + Ok(ids) + } + + /// Committed edges `(id, src)` in `edge_table` matching `keys` on `key_col` + /// (`"id"` or `"src"`). Index-backed. + async fn committed_edges( + &self, + edge_table: &str, + key_col: &str, + keys: &[String], + ) -> Result> { + let Some(ds) = self.open_cardinality(edge_table).await? else { + return Ok(Vec::new()); + }; + if keys.is_empty() { + return Ok(Vec::new()); + } + let expr = col(key_col).in_list(keys.iter().map(|k| lit(k.clone())).collect(), false); + let batches = scan_filtered(&ds, &["id", "src"], expr).await?; + let mut out = Vec::new(); + for batch in &batches { + let ids = string_col(batch, "id")?; + let srcs = string_col(batch, "src")?; + for i in 0..batch.num_rows() { + out.push((ids.value(i).to_string(), srcs.value(i).to_string())); + } + } + Ok(out) + } + + /// Committed edges `(id, src, dst)` in `edge_table` whose src is in + /// `src_nodes` OR dst is in `dst_nodes` — the edges a node deletion would + /// strand. Index-backed (BTREE on src/dst). + async fn edges_referencing( + &self, + edge_table: &str, + src_nodes: &[String], + dst_nodes: &[String], + ) -> Result> { + if src_nodes.is_empty() && dst_nodes.is_empty() { + return Ok(Vec::new()); + } + let Some(ds) = self.open(edge_table).await? else { + return Ok(Vec::new()); + }; + let mut expr: Option = None; + if !src_nodes.is_empty() { + expr = + Some(col("src").in_list(src_nodes.iter().map(|k| lit(k.clone())).collect(), false)); + } + if !dst_nodes.is_empty() { + let dst = col("dst").in_list(dst_nodes.iter().map(|k| lit(k.clone())).collect(), false); + expr = Some(match expr { + Some(acc) => acc.or(dst), + None => dst, + }); + } + let batches = scan_filtered(&ds, &["id", "src", "dst"], expr.unwrap()).await?; + let mut out = Vec::new(); + for batch in &batches { + let ids = string_col(batch, "id")?; + let srcs = string_col(batch, "src")?; + let dsts = string_col(batch, "dst")?; + for i in 0..batch.num_rows() { + out.push(( + ids.value(i).to_string(), + srcs.value(i).to_string(), + dsts.value(i).to_string(), + )); + } + } + Ok(out) + } +} + +/// Scan `ds` projecting `projection`, filtered by a structured `expr` applied via +/// `Scanner::filter_expr` so Lance can route it through the scalar index. The one +/// place the index-backed scan boilerplate lives. +async fn scan_filtered(ds: &Dataset, projection: &[&str], expr: Expr) -> Result> { + TableStore::scan_stream_with(ds, Some(projection), None, None, false, move |scanner| { + scanner.filter_expr(expr); + Ok(()) + }) + .await? + .try_collect() + .await + .map_err(|e| OmniError::Lance(e.to_string())) +} + +/// Scan `projection` from every row (no filter). Used to enumerate a table's +/// committed ids when computing what an `Overwrite` removes. +async fn scan_all(ds: &Dataset, projection: &[&str]) -> Result> { + TableStore::scan_stream_with(ds, Some(projection), None, None, false, |_| Ok(())) + .await? + .try_collect() + .await + .map_err(|e| OmniError::Lance(e.to_string())) +} + +/// Ids an `Overwrite` of `table_key` removes: committed ids in `base` that are +/// NOT in `change`'s replacement image (`added ∪ changed`). The loader folds +/// these into the change-set's `deleted_ids` so edge-RI (path-b) and cardinality +/// recompute against a node/edge the overwrite drops — e.g. a retained edge to a +/// removed node, or a src an overwrite empties. Reads the RAW base (NOT the +/// overwrite-emptied [`CommittedState`] view). Empty if the table is new in `base`. +pub(crate) async fn overwrite_removed_ids( + base: &Snapshot, + table_key: &str, + change: &TableChange, +) -> Result> { + if base.entry(table_key).is_none() { + return Ok(Vec::new()); + } + let mut new_ids: HashSet = HashSet::new(); + for batch in change.value_batches() { + let column = string_col(batch, "id")?; + for i in 0..column.len() { + if !column.is_null(i) { + new_ids.insert(column.value(i).to_string()); + } + } + } + let ds = base.open(table_key).await?; + let mut removed = Vec::new(); + for batch in &scan_all(&ds, &["id"]).await? { + let column = string_col(batch, "id")?; + for i in 0..column.len() { + if !column.is_null(i) && !new_ids.contains(column.value(i)) { + removed.push(column.value(i).to_string()); + } + } + } + Ok(removed) +} + +fn string_col<'b>(batch: &'b RecordBatch, name: &str) -> Result<&'b StringArray> { + batch + .column_by_name(name) + .ok_or_else(|| OmniError::manifest(format!("batch missing column '{name}'")))? + .as_any() + .downcast_ref::() + .ok_or_else(|| OmniError::manifest(format!("column '{name}' is not Utf8"))) +} + +/// Non-null `id`s across a table's added∪changed delta rows. +fn delta_id_set(change: &TableChange) -> Result> { + let mut ids = HashSet::new(); + for batch in change.value_batches() { + let column = string_col(batch, "id")?; + for i in 0..column.len() { + if !column.is_null(i) { + ids.insert(column.value(i).to_string()); + } + } + } + Ok(ids) +} + +/// `(edge_id, src)` for a table's added∪changed delta edge rows. +fn delta_edge_src(change: &TableChange) -> Result> { + let mut out = Vec::new(); + for batch in change.value_batches() { + let ids = string_col(batch, "id")?; + let srcs = string_col(batch, "src")?; + for i in 0..batch.num_rows() { + out.push((ids.value(i).to_string(), srcs.value(i).to_string())); + } + } + Ok(out) +} + +/// Write-path tail: derive the catalog constraints, run [`evaluate`] over the +/// change-set against `committed`, and return the first violation as an +/// `OmniError`. Shared by the mutation and loader paths (the merge path maps to +/// `MergeConflict`s instead, so it calls [`evaluate`] directly). +pub(crate) async fn validate_changeset( + changeset: &ChangeSet, + committed: &CommittedState<'_>, + catalog: &Catalog, +) -> Result<()> { + if changeset.is_empty() { + return Ok(()); + } + let constraints = constraints_for(catalog); + let violations = evaluate(&constraints, changeset, committed, catalog).await?; + match violations.into_iter().next() { + Some(violation) => Err(violation.into_omni_error()), + None => Ok(()), + } +} + +/// Run the declared constraints over the merge delta against committed state. +/// Δ-scoped: only tables present in `changeset` do any work. +pub(crate) async fn evaluate( + constraints: &[Constraint], + changeset: &ChangeSet, + committed: &CommittedState<'_>, + catalog: &Catalog, +) -> Result> { + let mut violations = Vec::new(); + for constraint in constraints { + match constraint { + Constraint::Value => { + violations.extend(evaluate_value_constraints(changeset, catalog)); + } + Constraint::Unique { + table_key, + columns, + is_key, + } => { + if let Some(change) = changeset.get(table_key) { + violations.extend( + evaluate_unique(table_key, columns, *is_key, change, committed).await?, + ); + } + } + Constraint::EdgeRi { + table_key, + from_type, + to_type, + } => { + // Run when the edge itself has a delta OR when a referenced node + // type has deletions (path-b can strand a committed target edge + // even if this edge table has no delta of its own). + let node_deleted = |node_type: &str| { + changeset + .get(&format!("node:{node_type}")) + .map(|change| !change.deleted_ids.is_empty()) + .unwrap_or(false) + }; + let change = changeset.get(table_key); + if change.is_some() || node_deleted(from_type) || node_deleted(to_type) { + violations.extend( + evaluate_edge_ri(table_key, from_type, to_type, change, changeset, committed) + .await?, + ); + } + } + Constraint::Cardinality { table_key } => { + if let Some(change) = changeset.get(table_key) { + let Some(type_name) = table_key.strip_prefix("edge:") else { + continue; + }; + if let Some(edge_type) = catalog.edge_types.get(type_name) { + violations.extend( + evaluate_cardinality(table_key, edge_type, change, changeset, committed) + .await?, + ); + } + } + } + } + } + Ok(violations) +} + +/// Uniqueness for one `@unique`/`@key` group on `table_key`, evaluated against +/// the delta's FINAL coalesced image (last-wins per id) — the same image commit +/// persists. Three checks: +/// 1. **within ONE batch** — two distinct input records sharing a key (a bulk +/// load listing the same `@key`/`@unique` value twice). Always a violation, +/// even with the same id (a load has no ordering), and coalescing would hide +/// it — so it is checked per-batch first. +/// 2. **across the coalesced image** — two DISTINCT ids holding the same final +/// key. Coalescing by id means a read-your-writes update that changes a row's +/// key (`temp -> final`) releases the old key, so a later row may reuse it. +/// 3. **committed cross-version** (non-`@key`) — a final key colliding with a +/// SURVIVING committed row (not itself in the delta, not deleted). `@key` is +/// id-backed, so a committed holder of a key value is the same row (an +/// upsert) — self-excluded — so the probe is skipped. +async fn evaluate_unique( + table_key: &str, + columns: &[String], + is_key: bool, + change: &TableChange, + committed: &CommittedState<'_>, +) -> Result> { + let mut violations = Vec::new(); + let delta_ids = delta_id_set(change)?; + let deleted: HashSet<&String> = change.deleted_ids.iter().collect(); + + // Pass 1: per-batch within-batch dup detection AND coalesce the delta by id + // (last-wins) into each id's final (key, typed values). A row whose key + // became null removes the id (it no longer holds a unique key). + let mut final_by_id: HashMap, Vec)> = HashMap::new(); + for batch in change.value_batches() { + let group_columns = columns + .iter() + .map(|name| { + batch.column_by_name(name).cloned().ok_or_else(|| { + OmniError::manifest(format!("table {table_key} missing unique column '{name}'")) + }) + }) + .collect::>>()?; + let ids = string_col(batch, "id")?; + let mut seen_in_batch: HashMap, String> = HashMap::new(); + for row in 0..batch.num_rows() { + let id = ids.value(row).to_string(); + let Some(key) = composite_unique_key(&group_columns, row)? else { + final_by_id.remove(&id); + continue; + }; + if let Some(prior) = seen_in_batch.insert(key.clone(), id.clone()) { + violations.push(unique_violation(table_key, columns, &key, &id, &prior)); + } + // Typed literals from the row's Arrow columns for the committed probe + // (a stringified key would compare a typed column to Utf8). `key` is + // `Some`, so every column is non-null and `try_from_array` is concrete. + let values = group_columns + .iter() + .map(|arr| ScalarValue::try_from_array(arr, row)) + .collect::, _>>() + .map_err(|e| OmniError::manifest(e.to_string()))?; + final_by_id.insert(id, (key, values)); + } + } + // A within-batch duplicate is an unambiguous bulk-input error; report it + // without the coalesced cross-row pass (which would re-report the same pair). + if !violations.is_empty() { + return Ok(violations); + } + + // Deterministic order — no HashMap iteration in violation ordering. + let mut entries: Vec<(String, (Vec, Vec))> = + final_by_id.into_iter().collect(); + entries.sort_by(|a, b| a.0.cmp(&b.0)); + + // Pass 2: two DISTINCT ids holding the same final key. + let mut holder_by_key: HashMap<&Vec, &String> = HashMap::new(); + for (id, (key, _)) in &entries { + if let Some(other) = holder_by_key.insert(key, id) { + if other != id { + violations.push(unique_violation(table_key, columns, key, id, other)); + } + } + } + + // Pass 3: committed cross-version (non-`@key` only). + if !is_key { + for (id, (key, values)) in &entries { + for holder in committed.unique_holders(table_key, columns, values).await? { + if !delta_ids.contains(&holder) && !deleted.contains(&holder) { + violations.push(unique_violation(table_key, columns, key, id, &holder)); + break; + } + } + } + } + Ok(violations) +} + +/// Edge referential integrity for added∪changed edges: each endpoint must exist +/// in the MERGED node universe (`target ± delta`). The path-b case — a deleted +/// node stranding a pre-existing committed edge — is unreachable here: `mutate` +/// cascades a node delete to its edges and `load` validates RI, so a surviving +/// edge can never reference a node the same merge deleted (it would either be +/// cascade-removed or surface as a structural `DeleteVsUpdate`). So checking the +/// edge delta is sufficient and equivalent to the old full scan on all reachable +/// inputs. +async fn evaluate_edge_ri( + edge_table: &str, + from_type: &str, + to_type: &str, + change: Option<&TableChange>, + changeset: &ChangeSet, + committed: &CommittedState<'_>, +) -> Result> { + let from_table = format!("node:{from_type}"); + let to_table = format!("node:{to_type}"); + let mut violations = Vec::new(); + // Delta edge ids — excluded from path-b (path-a already covers them). + let mut delta_edge_ids: HashSet = HashSet::new(); + + // Path-a: each added/changed edge's endpoints must exist in the merged node + // universe (`target ± delta`). + if let Some(change) = change { + let mut edges = Vec::new(); + for batch in change.value_batches() { + let ids = string_col(batch, "id")?; + let srcs = string_col(batch, "src")?; + let dsts = string_col(batch, "dst")?; + for i in 0..batch.num_rows() { + let id = ids.value(i).to_string(); + delta_edge_ids.insert(id.clone()); + edges.push((id, srcs.value(i).to_string(), dsts.value(i).to_string())); + } + } + if !edges.is_empty() { + let srcs: Vec = edges.iter().map(|(_, src, _)| src.clone()).collect(); + let dsts: Vec = edges.iter().map(|(_, _, dst)| dst.clone()).collect(); + let from_exist = merged_node_existence(&from_table, &srcs, changeset, committed).await?; + let to_exist = merged_node_existence(&to_table, &dsts, changeset, committed).await?; + for (id, src, dst) in &edges { + if !from_exist.contains(src) { + violations.push(orphan_violation(edge_table, id, "src", src, from_type)); + } + if !to_exist.contains(dst) { + violations.push(orphan_violation(edge_table, id, "dst", dst, to_type)); + } + } + } + } + + // Path-b: a node deleted by this merge can strand a committed (target) edge + // the merge keeps — reachable when the edge lives on the target side and the + // node deletion on the source side, so the edge is neither cascade-removed + // nor in this table's delta. Probe committed target edges referencing the + // deleted nodes; any that survive (not in the delta, not removed) are orphans. + let deleted_from: Vec = changeset + .get(&from_table) + .map(|change| change.deleted_ids.clone()) + .unwrap_or_default(); + let deleted_to: Vec = changeset + .get(&to_table) + .map(|change| change.deleted_ids.clone()) + .unwrap_or_default(); + if !deleted_from.is_empty() || !deleted_to.is_empty() { + let removed: HashSet<&String> = change + .map(|change| change.deleted_ids.iter().collect()) + .unwrap_or_default(); + let from_set: HashSet<&String> = deleted_from.iter().collect(); + let to_set: HashSet<&String> = deleted_to.iter().collect(); + for (id, src, dst) in committed + .edges_referencing(edge_table, &deleted_from, &deleted_to) + .await? + { + if delta_edge_ids.contains(&id) || removed.contains(&id) { + continue; + } + if from_set.contains(&src) { + violations.push(orphan_violation(edge_table, &id, "src", &src, from_type)); + } + if to_set.contains(&dst) { + violations.push(orphan_violation(edge_table, &id, "dst", &dst, to_type)); + } + } + } + + Ok(violations) +} + +/// Which of `ids` exist in the merged node table `node_table` = `target ± delta`: +/// present if added/changed in the delta, absent if deleted, else an index probe +/// of the committed target. +async fn merged_node_existence( + node_table: &str, + ids: &[String], + changeset: &ChangeSet, + committed: &CommittedState<'_>, +) -> Result> { + let (added_changed, deleted) = match changeset.get(node_table) { + Some(change) => ( + delta_id_set(change)?, + change.deleted_ids.iter().cloned().collect::>(), + ), + None => (HashSet::new(), HashSet::new()), + }; + let mut exist = HashSet::new(); + let mut to_probe = Vec::new(); + for id in ids { + if added_changed.contains(id) { + exist.insert(id.clone()); + } else if !deleted.contains(id) { + to_probe.push(id.clone()); + } + } + for id in committed.existing_ids(node_table, &to_probe).await? { + exist.insert(id); + } + Ok(exist) +} + +/// `@card` for an edge type, scoped to the srcs the delta affects. The delta is +/// coalesced by edge id (last-wins, as commit does); the merged edge set per src +/// = (committed edges with that src, minus those deleted or re-placed by the +/// delta) ∪ (coalesced delta edges with that src). The affected set includes the +/// new src of each delta edge AND the old committed src of each changed/deleted +/// edge id, so moving an edge off a src recounts the vacated src. A src that is +/// itself a deleted node is skipped. +async fn evaluate_cardinality( + edge_table: &str, + edge_type: &EdgeType, + change: &TableChange, + changeset: &ChangeSet, + committed: &CommittedState<'_>, +) -> Result> { + let card = &edge_type.cardinality; + // Default unbounded cardinality can never be violated — skip the lookups. + if card.min == 0 && card.max.is_none() { + return Ok(Vec::new()); + } + let delta_edges = delta_edge_src(change)?; + let removed_ids: Vec = change.deleted_ids.clone(); + let removed_id_set: HashSet<&String> = removed_ids.iter().collect(); + + // Coalesce the delta by edge id, last-wins — matching commit's + // `dedupe_merge_batches_by_id`. A Merge load can list the same edge id twice + // with different srcs; commit keeps the last, so counting raw delta rows + // would place one id under multiple srcs and over-count. + let mut delta_by_id: HashMap = HashMap::new(); + for (id, src) in &delta_edges { + delta_by_id.insert(id.clone(), src.clone()); + } + let changed_ids: Vec = delta_by_id.keys().cloned().collect(); + let delta_id_set: HashSet<&String> = changed_ids.iter().collect(); + + // Committed srcs of the edges this delta touches. `removed_edges` are the + // deleted ids (direct deletes; a node-delete cascade already lands those ids + // in `deleted_ids`). `moved_from` are the changed ids' OLD committed srcs: an + // upsert that moves an edge's src vacates its old src, which must be + // recounted or a drop below @card min is missed. + let removed_edges = committed.committed_edges(edge_table, "id", &removed_ids).await?; + let moved_from = committed.committed_edges(edge_table, "id", &changed_ids).await?; + + let deleted_src_nodes: HashSet = changeset + .get(&format!("node:{}", edge_type.from_type)) + .map(|change| change.deleted_ids.iter().cloned().collect()) + .unwrap_or_default(); + + let mut affected: HashSet = HashSet::new(); + for src in delta_by_id.values() { + affected.insert(src.clone()); + } + for (_, src) in removed_edges.iter().chain(moved_from.iter()) { + affected.insert(src.clone()); + } + affected.retain(|src| !deleted_src_nodes.contains(src)); + if affected.is_empty() { + return Ok(Vec::new()); + } + + let affected_vec: Vec = affected.iter().cloned().collect(); + let committed_for_affected = committed + .committed_edges(edge_table, "src", &affected_vec) + .await?; + + // Merged edge-id set per src. A committed edge is dropped from its src when + // the delta deletes it (`removed_id_set`) OR re-places it (`delta_id_set` — a + // changed edge is recounted at its new src below, so its old src must not + // keep counting it). Then add the coalesced delta edges at their last-wins + // src. Counting by id keeps the validated set equal to what commit persists. + let mut per_src: HashMap> = HashMap::new(); + for (id, src) in &committed_for_affected { + if removed_id_set.contains(id) || delta_id_set.contains(id) { + continue; + } + per_src.entry(src.clone()).or_default().insert(id.clone()); + } + for (id, src) in &delta_by_id { + per_src.entry(src.clone()).or_default().insert(id.clone()); + } + + let mut violations = Vec::new(); + for src in &affected { + let count = per_src.get(src).map(|ids| ids.len() as u32).unwrap_or(0); + if let Some(max) = card.max { + if count > max { + violations.push(cardinality_violation( + edge_table, + &edge_type.name, + src, + count, + "max", + max, + )); + } + } + if count < card.min { + violations.push(cardinality_violation( + edge_table, + &edge_type.name, + src, + count, + "min", + card.min, + )); + } + } + Ok(violations) +} + +/// Canonical `@unique` violation message, matching the write path's format +/// (`validators.rs` asserts the `"@unique violation on {Type}.{cols}"` prefix via +/// `.contains`). `type_name` is the bare type (`User`), not the `node:`/`edge:` +/// table key; `columns`/`key` render via `format_tuple` (single → `email`, +/// composite → `(a, b)`). +fn unique_violation( + table_key: &str, + columns: &[String], + key: &[String], + id: &str, + other: &str, +) -> Violation { + let type_name = table_key + .strip_prefix("node:") + .or_else(|| table_key.strip_prefix("edge:")) + .unwrap_or(table_key); + Violation { + table_key: table_key.to_string(), + row_id: Some(id.to_string()), + kind: MergeConflictKind::UniqueViolation, + message: format!( + "@unique violation on {type_name}.{}: value '{}' held by '{other}' and '{id}'", + format_tuple(columns), + format_tuple(key) + ), + } +} + +/// Canonical orphan-edge message, matching the write path's `"{src|dst} '{id}' +/// not found in {Type}"` format. +fn orphan_violation( + edge_table: &str, + edge_id: &str, + label: &str, + endpoint: &str, + node_type: &str, +) -> Violation { + Violation { + table_key: edge_table.to_string(), + row_id: Some(edge_id.to_string()), + kind: MergeConflictKind::OrphanEdge, + message: format!("{label} '{endpoint}' not found in {node_type}"), + } +} + +fn cardinality_violation( + edge_table: &str, + edge_name: &str, + src: &str, + count: u32, + bound: &str, + limit: u32, +) -> Violation { + Violation { + table_key: edge_table.to_string(), + row_id: None, + kind: MergeConflictKind::CardinalityViolation, + message: format!( + "@card violation on edge {edge_name}: source '{src}' has {count} edges ({bound} {limit})" + ), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::Arc; + + use arrow_array::StringArray; + use arrow_schema::{DataType, Field, Schema}; + use omnigraph_compiler::catalog::build_catalog; + use omnigraph_compiler::schema::parser::parse_schema; + + const DOC_SCHEMA: &str = "node Doc {\n slug: String @key\n status: enum(draft, published)\n}\n"; + + fn catalog(src: &str) -> Catalog { + build_catalog(&parse_schema(src).unwrap()).unwrap() + } + + /// A change-set touching only `Doc.status` with the given values. + fn status_change(values: &[&str]) -> ChangeSet { + let schema = Arc::new(Schema::new(vec![Field::new("status", DataType::Utf8, true)])); + let batch = + RecordBatch::try_new(schema, vec![Arc::new(StringArray::from(values.to_vec())) as _]) + .unwrap(); + let mut change = TableChange::default(); + change.changed.push(batch); + let mut cs = ChangeSet::new(); + cs.insert("node:Doc".to_string(), change); + cs + } + + /// The merge path previously validated `@range`/`@check` but NOT enum + /// membership, so a delta carrying an out-of-enum value slipped through (W1). + /// The unified evaluator runs the enum check the write path always ran. + #[test] + fn evaluator_flags_out_of_enum_value_in_delta() { + let v = evaluate_value_constraints(&status_change(&["bogus"]), &catalog(DOC_SCHEMA)); + assert_eq!(v.len(), 1, "expected one enum violation, got {v:?}"); + assert_eq!(v[0].kind, MergeConflictKind::ValueConstraintViolation); + assert!(v[0].message.contains("bogus"), "message was: {}", v[0].message); + } + + #[test] + fn evaluator_accepts_valid_delta() { + assert!( + evaluate_value_constraints(&status_change(&["draft"]), &catalog(DOC_SCHEMA)).is_empty() + ); + } + + /// Δ-scoping: an empty change-set does no work and raises nothing — + /// validation cost follows the delta, not the table size. + #[test] + fn evaluator_ignores_empty_changeset() { + assert!(evaluate_value_constraints(&ChangeSet::new(), &catalog(DOC_SCHEMA)).is_empty()); + } +} diff --git a/crates/omnigraph/tests/branching.rs b/crates/omnigraph/tests/branching.rs index 7edc6a4b..7f4fb4ba 100644 --- a/crates/omnigraph/tests/branching.rs +++ b/crates/omnigraph/tests/branching.rs @@ -1428,6 +1428,72 @@ async fn branch_merge_reports_cardinality_violation_conflict() { } } +/// Fix C regression: a table adopted by pointer switch (`AdoptSourceState`) +/// must still be validated. Merging `main` -> `feature` where `feature` deleted +/// a node and `main` added an edge referencing it classifies the edge table as +/// `AdoptSourceState` (source on main, target on a branch). The unified +/// evaluator must see the adopted edge and reject the orphan; before the fix it +/// skipped the table entirely and silently published the dangling edge. +#[tokio::test] +async fn merge_main_into_branch_validates_adopted_edge_against_branch_node_delete() { + const MUTATIONS: &str = r#" +query add_knows($from: String, $to: String) { + insert Knows { from: $from, to: $to } +} + +query delete_person($name: String) { + delete Person where name = $name +} +"#; + + let dir = tempfile::tempdir().unwrap(); + let uri = dir.path().to_str().unwrap(); + let mut main = init_db_from_schema_and_data(&dir, EDGE_UNIQUE_SCHEMA, EDGE_UNIQUE_DATA).await; + main.branch_create("feature").await.unwrap(); + let mut feature = Omnigraph::open(uri).await.unwrap(); + + // main (merge source): add an edge referencing Bob. + mutate_main( + &mut main, + MUTATIONS, + "add_knows", + ¶ms(&[("$from", "Alice"), ("$to", "Bob")]), + ) + .await + .unwrap(); + + // feature (merge target): delete Bob. + mutate_branch( + &mut feature, + "feature", + MUTATIONS, + "delete_person", + ¶ms(&[("$name", "Bob")]), + ) + .await + .unwrap(); + + // Merge main -> feature: edge:Knows is adopted by pointer switch + // (AdoptSourceState). The adopted edge Alice->Bob references Bob, which the + // target branch deleted, so the merge must reject with OrphanEdge. + let err = feature + .branch_merge("main", "feature") + .await + .expect_err("adopting main's edge into a branch that deleted its endpoint must conflict"); + match err { + OmniError::MergeConflicts(conflicts) => { + assert!( + conflicts + .iter() + .any(|c| c.table_key == "edge:Knows" + && c.kind == MergeConflictKind::OrphanEdge), + "expected OrphanEdge on edge:Knows, got {conflicts:?}" + ); + } + other => panic!("expected merge conflicts, got {other:?}"), + } +} + #[tokio::test] async fn branch_api_rejects_reserved_main_and_same_source_target_merge() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/omnigraph/tests/end_to_end.rs b/crates/omnigraph/tests/end_to_end.rs index ea11d0e6..aa72d91d 100644 --- a/crates/omnigraph/tests/end_to_end.rs +++ b/crates/omnigraph/tests/end_to_end.rs @@ -233,8 +233,15 @@ async fn overwrite_replaces_data() { .await .unwrap(); - // Overwrite with just one person - let small = r#"{"type": "Person", "data": {"name": "Zara", "age": 40}}"#; + // Overwrite to a small SELF-CONSISTENT image. Overwrite is per-table, so a + // Person-only overwrite would drop Alice/Bob while the retained Knows/WorksAt + // edges still reference them — a now-rejected orphan (see + // `validators::overwrite_node_removal_rejects_retained_orphan_edge`). To + // replace the graph, overwrite the edge tables too; Company stays retained + // and Zara->Acme references it. + let small = r#"{"type": "Person", "data": {"name": "Zara", "age": 40}} +{"edge": "Knows", "from": "Zara", "to": "Zara"} +{"edge": "WorksAt", "from": "Zara", "to": "Acme"}"#; load_jsonl(&mut db, small, LoadMode::Overwrite) .await .unwrap(); diff --git a/crates/omnigraph/tests/merge_cost.rs b/crates/omnigraph/tests/merge_cost.rs new file mode 100644 index 00000000..4457b3ec --- /dev/null +++ b/crates/omnigraph/tests/merge_cost.rs @@ -0,0 +1,186 @@ +//! EMPIRICAL VALIDATION of the branch-merge latency analysis (investigation +//! artifact). Two claims from the code review, measured at the object-store +//! boundary with the shared `helpers::cost` harness: +//! +//! 1. `merge_validation_opens_untouched_tables` — `validate_merge_candidates` +//! loops EVERY catalog node/edge type and opens each (untouched tables fall +//! through to the target snapshot), so a merge whose delta touches ONE table +//! still opens ALL tables. Cost ∝ #types (whole graph), not ∝ delta. +//! +//! 2. `merge_manifest_cost_grows_with_history` — a merge does several cold +//! coordinator opens (head lookups + merge_base + base/source/target +//! snapshots), each an O(history) `__manifest` scan, so on an un-compacted +//! graph merge `__manifest` reads grow with commit depth (Regime A — the +//! production RustFS/S3 case). +//! +//! Both bodies run on a 64 MiB-stack thread: the debug-build merge future plus +//! the `cost_harness`/`measure` task-local layers overflow the default 2 MiB test +//! stack (the same reason these cost tests raise `recursion_limit`). +#![recursion_limit = "512"] + +mod helpers; + +use std::future::Future; + +use helpers::cost::{ + IoCounts, assert_flat, assert_grows, cost_harness, local_graph, measure, measure_with_staged, +}; +use helpers::{MUTATION_QUERIES, commit_many, mixed_params}; + +/// Run an async test body on a thread with a large stack. The debug merge future +/// is deep enough to overflow the default test-thread stack under the cost +/// harness's extra async layers. +fn on_big_stack(body: impl FnOnce() -> F + Send + 'static) +where + F: Future, +{ + std::thread::Builder::new() + .stack_size(64 * 1024 * 1024) + .spawn(move || { + tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap() + .block_on(body()); + }) + .unwrap() + .join() + .unwrap(); +} + +/// CLAIM 1 (post-#5): merge validation is Δ-scoped, not whole-graph. The fixture +/// has 4 tables (Person, Company, Knows, WorksAt). A merge whose only change is +/// one inserted Person must NOT open the untouched tables for validation — cost +/// follows the delta, not the catalog. Pre-#5 this opened ~6 tables via a +/// full-graph validation scan; the index-backed evaluator probes only the +/// committed Person table (for uniqueness) plus the delta. +#[test] +fn merge_validation_is_delta_scoped() { + on_big_stack(|| async { + let dir = tempfile::tempdir().unwrap(); + let db = local_graph(&dir).await; + + // Control: a 1-row insert on main — the write path opens only the + // touched table (Person). + let (ctrl_res, ctrl) = measure(db.mutate( + "main", + MUTATION_QUERIES, + "insert_person", + &mixed_params(&[("$name", "ctrl")], &[("$age", 30)]), + )) + .await; + ctrl_res.unwrap(); + + // Branch + a one-row change touching ONLY Person. + db.branch_create("feature").await.unwrap(); + db.mutate( + "feature", + MUTATION_QUERIES, + "insert_person", + &mixed_params(&[("$name", "f1")], &[("$age", 41)]), + ) + .await + .unwrap(); + + // Measure the merge. + let (res, io, staged) = measure_with_staged(db.branch_merge("feature", "main")).await; + res.unwrap(); + + eprintln!( + "CONTROL 1-row insert on main : data_open_count={} data_reads={} manifest_reads={}", + ctrl.data_open_count, ctrl.data_reads, ctrl.manifest_reads + ); + eprintln!( + "MERGE 1-Person-row delta : data_open_count={} data_reads={} manifest_reads={} \ + [stage_append={} stage_merge_insert={} create_vector_index={}]", + io.data_open_count, + io.data_reads, + io.manifest_reads, + staged.stage_append, + staged.stage_merge_insert, + staged.create_vector_index, + ); + + // The proof: only Person changed, so the merge opens only Person-related + // tables (the delta + the committed-target index probe for uniqueness) — + // never the untouched Company / Knows / WorksAt. Pre-#5 this was ~6 + // (every catalog table, full-scanned). + assert!( + io.data_open_count <= 3, + "merge of a 1-Person delta opened {} data tables; expected <= 3 (Δ-scoped). \ + Pre-#5 it opened every catalog table (~6) via a whole-graph validation scan.", + io.data_open_count + ); + }); +} + +/// CLAIM 2: a merge's `__manifest` cost grows with commit-history depth on an +/// un-compacted graph (the several cold coordinator opens each scan O(fragments) +/// of `__manifest`). Contrast with `write_cost.rs`, where a single write's +/// manifest scan is held FLAT *after compaction* — here we deliberately do NOT +/// compact, modelling the production graph that has grown its `_versions/` and +/// `__manifest` fragments without GC. +#[test] +fn merge_manifest_cost_grows_with_history() { + on_big_stack(|| { + cost_harness(async { + let dir = tempfile::tempdir().unwrap(); + let mut db = local_graph(&dir).await; + + let mut curve: Vec<(u64, IoCounts)> = Vec::new(); + let mut current = 0u64; + for d in [5u64, 80] { + if d > current { + commit_many(&mut db, (d - current) as usize).await; + current = d; + } + let br = format!("feat_{d}"); + db.branch_create(&br).await.unwrap(); + db.mutate( + &br, + MUTATION_QUERIES, + "insert_person", + &mixed_params(&[("$name", &format!("p_{d}"))], &[("$age", 30)]), + ) + .await + .unwrap(); + current += 1; // the branch write advanced depth + + // Control single write at this depth, to quantify the merge's + // manifest-open multiplication vs a normal write. + let (cres, ctrl) = measure(db.mutate( + "main", + MUTATION_QUERIES, + "insert_person", + &mixed_params(&[("$name", &format!("c_{d}"))], &[("$age", 30)]), + )) + .await; + cres.unwrap(); + current += 1; + + let (res, io) = measure(db.branch_merge(&br, "main")).await; + res.unwrap(); + current += 1; // the merge advanced depth + + eprintln!( + "depth~{d}: MERGE manifest_reads={} data_reads={} data_open_count={} | \ + single-write manifest_reads={} (merge/write ratio = {:.1}x)", + io.manifest_reads, + io.data_reads, + io.data_open_count, + ctrl.manifest_reads, + io.manifest_reads as f64 / ctrl.manifest_reads.max(1) as f64, + ); + curve.push((d, io)); + } + + // Regime A: merge __manifest cost still grows with history — the cold + // cross-branch coordinator opens, a separate amplification not + // addressed by the validation change. + assert_grows(&curve, |c| c.manifest_reads, 1, "merge __manifest scan"); + // But validation table-opens are now Δ-scoped: flat across history + // depth (the merge no longer scans the catalog's tables per merge). + assert_flat(&curve, |c| c.data_open_count, 1, "merge data-table opens"); + }) + }); +} diff --git a/crates/omnigraph/tests/validators.rs b/crates/omnigraph/tests/validators.rs index ce8525d1..23698f6f 100644 --- a/crates/omnigraph/tests/validators.rs +++ b/crates/omnigraph/tests/validators.rs @@ -8,7 +8,7 @@ mod helpers; use omnigraph::db::Omnigraph; use omnigraph::loader::{LoadMode, load_jsonl}; -use helpers::{mutate_main, params}; +use helpers::{count_rows, mutate_main, params}; const ENUM_SCHEMA: &str = r#" node Person { @@ -55,6 +55,22 @@ node User { } "#; +const UNIQUE_MUTATIONS: &str = r#" +query insert_user($name: String, $email: String) { + insert User { name: $name, email: $email } +} +"#; + +// A non-String `@unique` column: the committed cross-version probe must build a +// typed literal, not a stringified key, or it compares a Date32 column to a Utf8 +// value (a DataFusion coercion error that breaks every write to the table). +const DATE_UNIQUE_SCHEMA: &str = r#" +node Task { + name: String @key + due: Date @unique +} +"#; + const CARDINALITY_SCHEMA: &str = r#" node Person { name: String @key } node Company { name: String @key } @@ -71,6 +87,19 @@ query add_employment($person: String, $company: String) { } "#; +// A non-zero @card min so a move that vacates a src can drop it below the floor. +const CARD_MIN_SCHEMA: &str = r#" +node Person { name: String @key } +node Company { name: String @key } +edge WorksAt: Person -> Company @card(1..) +"#; + +const CARD_MIN_DELETE_MUTATIONS: &str = r#" +query drop_employment($person: String) { + delete WorksAt where from = $person +} +"#; + async fn init_with(schema: &str, data: &str) -> (tempfile::TempDir, Omnigraph) { let dir = tempfile::tempdir().unwrap(); let uri = dir.path().to_str().unwrap(); @@ -200,9 +229,324 @@ async fn intra_batch_unique_rejected_on_jsonl_load() { ); } -// Note: single-row mutation insert can't violate intra-batch uniqueness -// (only one row in the batch). Cross-batch uniqueness against committed rows -// is out of scope for this wire-up — see the unified write-validator effort. +// Single-row mutation insert can't violate INTRA-BATCH uniqueness (one row). +// CROSS-VERSION uniqueness against already-committed rows IS now enforced on the +// mutation path via the unified evaluator (#1/#2); the loader path's +// cross-version check lands with the loader migration. + +/// Cross-version uniqueness, closed by the write-path evaluator migration: +/// two SEPARATE mutations inserting distinct rows with the same `@unique` value — +/// the second is rejected against the committed first (previously a gap). +#[tokio::test] +async fn cross_version_unique_rejected_on_mutation_insert() { + let (_dir, mut db) = init_with(UNIQUE_SCHEMA, "").await; + mutate_main( + &mut db, + UNIQUE_MUTATIONS, + "insert_user", + ¶ms(&[("$name", "Bob"), ("$email", "dup@example.com")]), + ) + .await + .unwrap(); + let err = mutate_main( + &mut db, + UNIQUE_MUTATIONS, + "insert_user", + ¶ms(&[("$name", "Carol"), ("$email", "dup@example.com")]), + ) + .await + .unwrap_err(); + assert!( + err.to_string().contains("@unique violation on User.email"), + "got: {}", + err + ); +} + +/// The cross-version unique check must NOT flag a row updating itself: an upsert +/// of an existing `@key` (same id) is an update, not a duplicate. Re-inserting +/// the same key with its own `@unique` value must succeed (the evaluator excludes +/// the committed same-id holder). +#[tokio::test] +async fn reinsert_existing_key_is_upsert_not_unique_violation() { + let (_dir, mut db) = init_with(UNIQUE_SCHEMA, "").await; + mutate_main( + &mut db, + UNIQUE_MUTATIONS, + "insert_user", + ¶ms(&[("$name", "Alice"), ("$email", "alice@example.com")]), + ) + .await + .unwrap(); + mutate_main( + &mut db, + UNIQUE_MUTATIONS, + "insert_user", + ¶ms(&[("$name", "Alice"), ("$email", "alice@example.com")]), + ) + .await + .expect("re-inserting an existing @key upserts; it is not a unique violation"); +} + +// ─── Cross-version uniqueness + RI on the LOADER path (Slice 3) ─────────────── + +const RI_SCHEMA: &str = r#" +node Person { name: String @key } +edge Knows: Person -> Person +"#; + +/// Cross-version uniqueness is now enforced on the bulk-load path too: a second +/// Append load duplicating a committed `@unique` value is rejected. +#[tokio::test] +async fn cross_version_unique_rejected_on_append_load() { + let (_dir, mut db) = init_with(UNIQUE_SCHEMA, "").await; + load_jsonl( + &mut db, + r#"{"type":"User","data":{"name":"Bob","email":"dup@example.com"}}"#, + LoadMode::Append, + ) + .await + .unwrap(); + let err = load_jsonl( + &mut db, + r#"{"type":"User","data":{"name":"Carol","email":"dup@example.com"}}"#, + LoadMode::Append, + ) + .await + .unwrap_err(); + assert!( + err.to_string().contains("@unique violation on User.email"), + "got: {}", + err + ); +} + +/// Fix D: the cross-version `@unique` probe must use a typed literal on a +/// non-String column. A second-version row colliding with a committed `Date` +/// value must surface a proper `@unique` violation — not a Date32-vs-Utf8 +/// coercion error (the red symptom before the fix). +#[tokio::test] +async fn cross_version_unique_rejected_on_date_column() { + let (_dir, mut db) = init_with(DATE_UNIQUE_SCHEMA, "").await; + load_jsonl( + &mut db, + r#"{"type":"Task","data":{"name":"T1","due":"2026-06-29"}}"#, + LoadMode::Append, + ) + .await + .unwrap(); + let err = load_jsonl( + &mut db, + r#"{"type":"Task","data":{"name":"T2","due":"2026-06-29"}}"#, + LoadMode::Append, + ) + .await + .unwrap_err(); + assert!( + err.to_string().contains("@unique violation on Task.due"), + "got: {}", + err + ); +} + +/// Fix D companion: a non-colliding write to a `Date @unique` table must +/// succeed. Before the fix the committed probe raised a coercion error for +/// ANY second write (it compared Date32 to a Utf8 literal regardless of a +/// match), so this happy path failed too. +#[tokio::test] +async fn noncolliding_write_to_date_unique_column_succeeds() { + let (_dir, mut db) = init_with(DATE_UNIQUE_SCHEMA, "").await; + load_jsonl( + &mut db, + r#"{"type":"Task","data":{"name":"T1","due":"2026-06-29"}}"#, + LoadMode::Append, + ) + .await + .unwrap(); + load_jsonl( + &mut db, + r#"{"type":"Task","data":{"name":"T2","due":"2026-07-01"}}"#, + LoadMode::Append, + ) + .await + .expect("a distinct Date value must not collide and must not raise a coercion error"); + assert_eq!(count_rows(&db, "node:Task").await, 2); +} + +/// Fix B: a Merge-load that MOVES an edge to a new src must recount the OLD +/// src. Moving Alice's only WorksAt to Bob drops Alice to zero, below +/// @card(1..). Before the fix only the new src (Bob) was in the affected set, +/// so Alice's underflow was missed and the load silently succeeded. +#[tokio::test] +async fn merge_load_edge_src_move_rechecks_vacated_src_cardinality() { + let seed = r#"{"type":"Person","data":{"name":"Alice"}} +{"type":"Person","data":{"name":"Bob"}} +{"type":"Company","data":{"name":"Acme"}} +{"edge":"WorksAt","from":"Alice","to":"Acme","data":{"id":"E1"}}"#; + let (_dir, mut db) = init_with(CARD_MIN_SCHEMA, seed).await; + + let err = load_jsonl( + &mut db, + r#"{"edge":"WorksAt","from":"Bob","to":"Acme","data":{"id":"E1"}}"#, + LoadMode::Merge, + ) + .await + .expect_err("moving Alice's only edge to Bob drops Alice below @card(1..)"); + assert!( + err.to_string().contains("@card violation") && err.to_string().contains("Alice"), + "got: {}", + err + ); +} + +/// Fix A: a Merge-load batch listing the same edge id twice with different srcs +/// must be counted ONCE (commit dedupes by id, last-wins). Alice keeps her one +/// committed edge and Bob gets the (deduped) E1, both within @card(0..1), so the +/// load must succeed. Before the fix E1 was counted under both srcs, giving +/// Alice a phantom second edge and a spurious max violation. +#[tokio::test] +async fn merge_load_duplicate_edge_id_counts_once_per_card() { + let seed = r#"{"type":"Person","data":{"name":"Alice"}} +{"type":"Person","data":{"name":"Bob"}} +{"type":"Company","data":{"name":"Acme"}} +{"type":"Company","data":{"name":"Beta"}} +{"edge":"WorksAt","from":"Alice","to":"Acme","data":{"id":"E0"}}"#; + let (_dir, mut db) = init_with(CARDINALITY_SCHEMA, seed).await; + + // Same edge id E1 under two srcs in one batch: commit keeps the last + // (Bob->Beta). Alice stays at her one committed edge (E0). + let batch = r#"{"edge":"WorksAt","from":"Alice","to":"Beta","data":{"id":"E1"}} +{"edge":"WorksAt","from":"Bob","to":"Beta","data":{"id":"E1"}}"#; + load_jsonl(&mut db, batch, LoadMode::Merge) + .await + .expect("a deduped edge id must not double-count Alice into a @card(0..1) violation"); + assert_eq!(count_rows(&db, "edge:WorksAt").await, 2); +} + +/// A direct edge DELETE must recount the source it empties. Deleting Alice's +/// only WorksAt drops her to zero, below @card(1..), and must be rejected. +/// Deletes stage as predicates (absent from the constructive change-set), so +/// before the fix the mutation committed without any cardinality check — while +/// the merge path, which carries deleted_ids, would have caught it. +#[tokio::test] +async fn mutation_delete_edge_below_card_min_rejected() { + let seed = r#"{"type":"Person","data":{"name":"Alice"}} +{"type":"Company","data":{"name":"Acme"}} +{"edge":"WorksAt","from":"Alice","to":"Acme","data":{"id":"E1"}}"#; + let (_dir, mut db) = init_with(CARD_MIN_SCHEMA, seed).await; + + let err = mutate_main( + &mut db, + CARD_MIN_DELETE_MUTATIONS, + "drop_employment", + ¶ms(&[("$person", "Alice")]), + ) + .await + .expect_err("deleting Alice's only WorksAt drops her below @card(1..)"); + assert!( + err.to_string().contains("@card violation") && err.to_string().contains("Alice"), + "got: {}", + err + ); + assert_eq!( + count_rows(&db, "edge:WorksAt").await, + 1, + "the rejected delete must not have removed the edge" + ); +} + +/// A Merge load re-upserting an existing `@key` with its own `@unique` value is +/// an update, not a duplicate — it must NOT false-trigger the cross-version check. +#[tokio::test] +async fn merge_load_reupsert_existing_key_is_not_unique_violation() { + let (_dir, mut db) = init_with(UNIQUE_SCHEMA, "").await; + let row = r#"{"type":"User","data":{"name":"Alice","email":"alice@example.com"}}"#; + load_jsonl(&mut db, row, LoadMode::Merge).await.unwrap(); + load_jsonl(&mut db, row, LoadMode::Merge) + .await + .expect("merge-load re-upserting an existing @key is not a unique violation"); +} + +/// `Overwrite` replaces the touched tables, so edge RI must validate against the +/// NEW batch image, not the replaced committed one. An edge to a node that exists +/// only in the new batch loads cleanly (regression against using the old image). +#[tokio::test] +async fn overwrite_load_validates_ri_against_new_image() { + let (_dir, mut db) = init_with(RI_SCHEMA, r#"{"type":"Person","data":{"name":"Alice"}}"#).await; + let batch = r#"{"type":"Person","data":{"name":"Carol"}} +{"edge":"Knows","from":"Carol","to":"Carol"}"#; + load_jsonl(&mut db, batch, LoadMode::Overwrite) + .await + .expect("Overwrite RI validates against the new batch image, not the replaced committed"); +} + +/// And an Append load whose edge references a non-existent node is still rejected +/// (edge-RI enforced via the evaluator). +#[tokio::test] +async fn append_load_rejects_orphan_edge() { + let (_dir, mut db) = init_with(RI_SCHEMA, r#"{"type":"Person","data":{"name":"Alice"}}"#).await; + let err = load_jsonl( + &mut db, + r#"{"edge":"Knows","from":"Alice","to":"Ghost"}"#, + LoadMode::Append, + ) + .await + .unwrap_err(); + assert!( + err.to_string().contains("not found"), + "orphan edge must be rejected, got: {}", + err + ); +} + +/// Finding 1: overwriting a NODE table can strand a retained edge in a +/// non-overwritten table. Seed Alice, Bob + Knows(Alice->Bob); Overwrite-load +/// node:Person with only Alice (Bob removed), leaving edge:Knows untouched -> +/// Knows(Alice->Bob) is now an orphan and must be rejected. The overwrite-removed +/// Bob is not expressed as a deleted_id, so edge-RI path-b never runs. +#[tokio::test] +async fn overwrite_node_removal_rejects_retained_orphan_edge() { + let seed = r#"{"type":"Person","data":{"name":"Alice"}} +{"type":"Person","data":{"name":"Bob"}} +{"edge":"Knows","from":"Alice","to":"Bob"}"#; + let (_dir, mut db) = init_with(RI_SCHEMA, seed).await; + + let err = load_jsonl( + &mut db, + r#"{"type":"Person","data":{"name":"Alice"}}"#, + LoadMode::Overwrite, + ) + .await + .expect_err("removing Bob via overwrite while Knows(Alice->Bob) is retained orphans the edge"); + assert!( + err.to_string().contains("not found"), + "retained edge to an overwrite-removed node must be rejected, got: {}", + err + ); +} + +/// Finding 2: uniqueness must evaluate the final coalesced image per id, not +/// accumulate superseded keys. One mutation updates Alice.email temp -> final, +/// then inserts Carol.email = temp. The final state (Alice=final, Carol=temp) is +/// valid, but the validator retains the stale Alice->temp and false-rejects Carol. +#[tokio::test] +async fn chained_unique_update_then_reuse_freed_value_is_not_a_violation() { + let (_dir, mut db) = init_with( + UNIQUE_SCHEMA, + r#"{"type":"User","data":{"name":"Alice","email":"orig"}}"#, + ) + .await; + const Q: &str = r#" +query reassign() { + update User set { email: "temp" } where name = "Alice" + update User set { email: "final" } where name = "Alice" + insert User { name: "Carol", email: "temp" } +} +"#; + mutate_main(&mut db, Q, "reassign", ¶ms(&[])) + .await + .expect("Alice ends at 'final' and Carol takes the freed 'temp' — final image has no collision"); +} // ─── Edge cardinality ──────────────────────────────────────────────────────── diff --git a/crates/omnigraph/tests/writes.rs b/crates/omnigraph/tests/writes.rs index 593b0e53..72821fef 100644 --- a/crates/omnigraph/tests/writes.rs +++ b/crates/omnigraph/tests/writes.rs @@ -926,9 +926,14 @@ async fn load_overwrite_with_bad_edge_reference_unblocks_next_load() { assert_eq!(count_rows(&db, "node:Person").await, pre_persons); assert_eq!(count_rows(&db, "edge:Knows").await, pre_edges); + // The good overwrite must be self-consistent: it replaces Person, so it also + // replaces every edge table that referenced the old Persons. WorksAt is in the + // batch (pointing the surviving Company at a new Person) so the retained + // WorksAt rows that named Alice/Bob don't strand against the new node image. let good = r#"{"type": "Person", "data": {"name": "Pat", "age": 55}} {"type": "Person", "data": {"name": "Quinn", "age": 56}} {"edge": "Knows", "from": "Pat", "to": "Quinn"} +{"edge": "WorksAt", "from": "Pat", "to": "Acme"} "#; load_jsonl(&mut db, good, LoadMode::Overwrite) .await diff --git a/docs/dev/invariants.md b/docs/dev/invariants.md index ebad595e..caa08055 100644 --- a/docs/dev/invariants.md +++ b/docs/dev/invariants.md @@ -157,7 +157,7 @@ converge the physical state. | Deletes | Staged like inserts/updates (`stage_delete` via Lance 7.0 `DeleteBuilder::execute_uncommitted`, MR-A) — no inline HEAD advance; mixed insert/update/delete in one query rejected by D2 as a deliberate boundary (constructive XOR destructive per query; compose via separate mutations or a branch) | [query-language.md](../user/queries/index.md), [writes.md](writes.md) | | Branch delete | Manifest is the single authority, flipped atomically first; per-table forks are derived state, reclaimed best-effort (`force_delete_branch`) with the `cleanup` reconciler as the guaranteed backstop. Reusing a name whose reclaim failed before `cleanup` surfaces an actionable error | [branches-commits.md](../user/branching/index.md), [maintenance.md](../user/operations/maintenance.md) | | Schema validation | Type checks, required fields, defaults, edge endpoint checks, and edge cardinality are enforced on write paths | [schema-language.md](../user/schema/index.md), [execution.md](execution.md) | -| Unique constraints | Intra-batch and write-path checks exist; intake and branch-merge derive the composite key through one shared function (`loader::composite_unique_key`, a separator-free `Vec` tuple) and fail loudly on an un-keyable column type rather than silently exempting it; full cross-version uniqueness against already-committed rows is still a gap | [schema-language.md](../user/schema/index.md) | +| Unique constraints | Value/enum, uniqueness, edge-RI, and cardinality route through ONE unified, catalog-derived evaluator (`crate::validate`) on ALL THREE write surfaces — branch-merge, mutation, and bulk load: Δ-scoped (checks the delta, not the whole graph) and index-backed (probes committed state through its BTREE rather than full-scanning every catalog table), reusing the leaf checks (`loader::validate_value_constraints`/`validate_enum_constraints`/`composite_unique_key`) so the surfaces cannot drift. This closed the prior merge bug (merge validated `@range`/`@check` but not enum) AND the **cross-version uniqueness gap** on the mutation and load paths (a duplicate of a committed `@unique` value is now rejected; the merge path always enforced it). The committed view is the merge target snapshot (merge), the write's pinned `txn.base` (mutation), or the pinned pre-load base (load — `Overwrite` validates the batch as the whole new image, committed view empty); `@card` reads LIVE HEAD per edge table on the mutation path only (the #298 stale-handle fix). `@key` is id-backed, so it is checked intra-delta only (a committed holder of a key value is always the same row — an upsert), skipping a wasted O(Δ) probe per keyed row; `@unique` (non-key) groups do the committed lookup. | [schema-language.md](../user/schema/index.md) | | Storage trait | `TableStorage` (via `db.storage()`) is staged-only; the sole inline-commit residual (`create_vector_index`) is split onto a separate sealed `InlineCommitResidual` trait reached via `db.storage_inline_residual()` (MR-854), so §1 holds by construction; capability/stat surfaces are roadmap | [writes.md](writes.md), [architecture.md](architecture.md) | | Index lifecycle | `@index`/`@key` declares *intent*; the physical index is derived state and never fails a logical op. `schema apply` builds no indexes (records intent only; index-only changes touch no table data). `load`/`mutate` build inline through one chokepoint (`build_indices_on_dataset_for_catalog`, type-dispatched by `node_prop_index_kind`: enum + orderable scalar → BTREE, free-text String → FTS, Vector → vector) that fault-isolates an untrainable Vector column into a *pending* index instead of aborting. `optimize`/`ensure_indices` is the reconciler: it creates declared-but-missing indexes and folds appended/rewritten fragments into existing ones (`optimize_indices`), reporting still-pending columns. Explicit maintenance call, not yet a background loop | [indexes.md](../user/search/indexes.md), [maintenance.md](../user/operations/maintenance.md) | | Traversal IDs | Runtime still builds `TypeIndex`; Lance stable row-id based graph IDs are roadmap | [architecture.md](architecture.md), [query-language.md](../user/queries/index.md) | diff --git a/docs/dev/testing.md b/docs/dev/testing.md index be8f83fb..a40170d9 100644 --- a/docs/dev/testing.md +++ b/docs/dev/testing.md @@ -43,7 +43,8 @@ The engine's `tests/` is the principal coverage surface; most graph-shaped behav | `export.rs` | NDJSON streaming export filters | | `s3_storage.rs` | S3-backed graph (skipped unless `OMNIGRAPH_S3_TEST_BUCKET` is set). Includes `s3_fresh_branch_traversal_reuses_main_graph_index_with_etags` — the CSR topology cache-key test on a **real** per-table e_tag (`None` on local FS, so `warm_read_cost.rs` can't reach this path); forces CSR via the scoped `with_traversal_mode` seam | | `lance_version_columns.rs` | Per-row `_row_last_updated_at_version` behavior | -| `validators.rs` | Schema constraint enforcement (enum, range, unique, cardinality) across JSONL, insert, update paths | +| `validators.rs` | Schema constraint enforcement (enum, range, unique, cardinality) across JSONL load, mutation insert/update. ALL THREE write surfaces — mutation, bulk load, AND merge — route through the unified `crate::validate` evaluator (Δ-scoped, index-backed, reusing these leaf checks). Cross-version-uniqueness closure: `cross_version_unique_rejected_on_mutation_insert` + `reinsert_existing_key_is_upsert_not_unique_violation` (mutation path); `cross_version_unique_rejected_on_append_load` + `merge_load_reupsert_existing_key_is_not_unique_violation` (load path). Per-table `Overwrite`: `overwrite_load_validates_ri_against_new_image` (an edges-only overwrite still resolves RI against retained committed nodes) + `append_load_rejects_orphan_edge`. The evaluator's own unit tests live in `src/validate.rs` (`#[cfg(test)]`); its merge-conflict equivalence is pinned by `merge_truth_table.rs` (OrphanEdge) + `branching.rs` (Unique/Cardinality merge tests). Intra-batch duplicate-`@key` rejection on every load mode is pinned by `consistency.rs::loader_rejects_intra_batch_duplicate_keys`; the mutation-coalesce counterpart (insert+update / chained updates of one id are NOT a self-collision) by `writes.rs`. Non-String `@unique` columns probe committed state with a TYPED literal (not a stringified key): `cross_version_unique_rejected_on_date_column` + `noncolliding_write_to_date_unique_column_succeeds` (a `Date @unique` collision is a proper `@unique` violation, and a distinct value does not raise a Date32-vs-Utf8 coercion error). Cardinality is keyed by edge id, last-wins (matching commit's `dedupe_merge_batches_by_id`): `merge_load_edge_src_move_rechecks_vacated_src_cardinality` (a Merge-load moving an edge recounts the vacated src for `@card` min) + `merge_load_duplicate_edge_id_counts_once_per_card` (a dup edge id under two srcs in one batch counts once, no spurious max violation). Direct deletes capture the ids they remove (from the delete op's own scan) into the change-set's `deleted_ids`, so a delete emptying a src is validated: `mutation_delete_edge_below_card_min_rejected` (a `delete Edge` dropping a src below `@card` min is rejected, not silently committed). | +| `merge_cost.rs` | Cost-budget tests for branch MERGE on the shared `helpers::cost` harness: `merge_validation_is_delta_scoped` (a 1-row-delta merge opens ≤3 data tables — Δ-scoped, not the whole catalog; was ~6 pre-#5) and `merge_manifest_cost_grows_with_history` (the cross-branch `__manifest` open amplification still grows with commit depth — a separate, not-yet-addressed term — while validation `data_open_count` stays flat) | | `policy_engine_chassis.rs` | Engine-layer Cedar enforcement (MR-722): allow + deny through every `_as` writer via the SDK directly — no HTTP — proving embedded and CLI callers hit the same gate as the server, with action × scope shapes matching `authorize_request` | | `maintenance.rs` | `optimize` (compaction), `repair` (explicit uncovered-drift publish), and `cleanup` (version GC): empty/idempotent/no-op edges, policy validation, head preservation; `optimize` publishes its own compaction (`optimize_publishes_compaction_to_manifest_so_schema_apply_succeeds`), skips pre-existing uncovered drift (`optimize_skips_preexisting_manifest_head_drift`), and refuses to run while a `__recovery` sidecar is pending (`optimize_defers_when_recovery_sidecar_is_pending`); `repair` previews/heals verified maintenance drift, refuses raw semantic drift without `--force`, and forced repair publishes only by explicit operator choice; the index reconciler (iss-848): `index_build_tolerates_null_vector_rows` (an untrainable Vector column defers instead of aborting the build, sibling indexes still build) and `optimize_materializes_index_declared_but_unbuilt` (optimize creates a declared-but-deferred index) | | `failpoints.rs` | Failure-injection coverage (gated on `failpoints` feature). Includes the five per-writer Phase B → recovery integration tests (`recovery_rolls_forward_after_finalize_publisher_failure`, `schema_apply_phase_b_failure_recovered_on_next_open`, `branch_merge_phase_b_failure_recovered_on_next_open`, `ensure_indices_phase_b_failure_recovered_on_next_open`, `optimize_phase_b_failure_recovered_on_next_open`) and the write-entry in-process heal contract (the four `*_after_finalize_publisher_failure_heals_without_reopen` tests — load, mutation, schema apply, branch merge: a follow-up write on the same handle rolls a sidecar-covered residual forward without reopen/refresh) and the storage-fault matrix for the sidecar lifecycle (`recovery.sidecar_{write,delete,list}` / `recovery.record_audit` failpoints: Phase A put failure aborts with zero drift, Phase D delete failure is swallowed and healed by the next write, list failures are loud at heal and open, audit-append failures are retried to exactly one audit row; plus the bucket-gated `s3_load_recovers_after_publisher_failure_without_reopen`). And the convergence-idempotent roll-forward regression (`open_sweep_roll_forward_converges_when_manifest_advances_concurrently`: two concurrent open-sweeps race one sidecar at the `recovery.before_roll_forward_publish` rendezvous; the CAS loser must converge, not fail the open — iss-schema-apply-reopen-recovery-race). | diff --git a/docs/rfcs/0001-fragment-adopt-branch-merge.md b/docs/rfcs/0001-fragment-adopt-branch-merge.md new file mode 100644 index 00000000..1d69dfe5 --- /dev/null +++ b/docs/rfcs/0001-fragment-adopt-branch-merge.md @@ -0,0 +1,432 @@ +# RFC 0001: Branch merge by fragment adoption + +| | | +|---|---| +| **Status** | Proposed | +| **Author(s)** | Ragnor Comerford | +| **Discussion** | — | +| **Implementation** | — | + +> Status is maintained by maintainers: `Proposed` while the PR is open, +> `Accepted` on merge, `Declined` on close, `Superseded by NNNN` later. + +## Summary + +Make branch merge **adopt the source branch's Lance fragments by reference** +instead of re-materializing rows. Today, merging a branch whose table diverged +from the merge base copies every new/changed row through a temp Lance dataset +(`stage_append` + `stage_merge_insert`) and then rebuilds the table's indexes. +On embedding-heavy graphs this is the dominant merge cost and an out-of-memory +risk at scale. Because OmniGraph branches are Lance-native branches whose data +already lives durably under `{table}/tree/{branch}/data/`, the merged table can +instead be committed as a new table version whose data files **point at** the +source's fragments via Lance's `base_paths` mechanism — an O(metadata) commit, +no row copy, no hash-join, no synchronous reindex. A background **re-home +reconciler** then copies those referenced files into the target's own storage so +the source branch can be safely deleted, closing the only correctness hazard the +reference introduces. + +## Motivation + +Branch merge has three per-table shapes (`exec/merge.rs`, +`CandidateTableState`): + +- **`AdoptSourceState`** — the target table is unchanged since the fork and the + source is wholly adoptable. This is **already** metadata-only: a manifest + pointer switch or a Lance-native `create_branch` fork, zero row copy. No + change is proposed here. +- **`AdoptWithDelta`** — the target is unchanged since the fork but the source + added / changed / deleted rows. Today this copies the delta row-by-row into the + target's storage. **This is the target of this RFC.** +- **`RewriteMerged`** — both sides changed; a real three-way row merge. Phase 2 + (see Unresolved questions). + +For `AdoptWithDelta`, the merged result *is* the source's table version (the +target equals the base, so there is nothing of the target's to preserve). Yet we +rebuild it row by row and then reindex. For a connector that incrementally +upserts embeddings, the changed set is large and each row carries a 3072-dim +vector; the copy + merge-insert hash-join + vector reindex is what hangs and +OOMs production merges. + +The long-run liability argument: the row-copy machinery +(`OrderedTableCursor`, `row_signature`, `compute_adopt_delta`, +`publish_adopted_delta`) is already documented as transitional — to be *removed +wholesale* once the substrate offers a fragment-level branch merge. Investing in +patching its cost (e.g. a cheaper change-signature) spends effort on +deletion-bound code without removing the copy. Fragment adoption removes the +copy now, on **public** Lance 7.0.0 APIs, and deletes the transitional machinery +for the `AdoptWithDelta` path — it converges the design toward the same end +state the code already anticipates, rather than forking it. + +## Guide-level explanation + +Nothing changes in the user-facing merge contract. `branch merge` produces the +same committed graph state, the same conflict kinds, and the same atomic +visibility. What changes is cost and latency: + +- A merge whose source diverged by appends/upserts/deletes commits in time + proportional to the **fragment count**, not the **row count** — typically a + handful of metadata operations per table instead of a full table copy. +- The vector/FTS index is not rebuilt synchronously on the merge path. +- A new background maintenance step, **re-home**, runs as part of + `optimize`/`cleanup` (and is forced before a branch that the target still + references can be reclaimed). Operators see it the way they see compaction: a + convergence step that reclaims the "borrowed" layout into target-local files. + +The one new operational rule: **a branch cannot be reclaimed while the target +still references its fragments.** Branch *deletion* (the logical operation) is +unchanged and immediate — the manifest flips authority as it does today — but the +physical reclamation of `tree/{branch}/data` is deferred until re-home has run, +exactly as branch-fork reclamation is already deferred to the `cleanup` +reconciler. + +## Reference-level design + +### Background: how Lance multi-base works (verified, Lance 7.0.0) + +All required APIs are `pub` and constructible (no `#[non_exhaustive]`): + +- A manifest carries `base_paths: HashMap` + (`lance-table/format/manifest.rs:103`); a `BasePath{id, name, is_dataset_root, + path: String}` holds a **full URI** to another dataset/branch root + (`manifest.rs:556`). +- A `DataFile` carries `base_id: Option` (`lance-table/format/fragment.rs:56`) + indexing into that map; at read time `Dataset::data_file_dir_for_base` + (`dataset.rs:1954`) resolves the file from the referenced base's store. +- `Dataset::create_data_file(path, base_id)` (`dataset.rs:1861`) reads an + **already-existing** file's metadata and returns a `DataFile` stamped with + `base_id` — **no copy**. +- `Operation::Overwrite{fragments, schema, initial_bases}`, + `Operation::Append{fragments}`, and `Operation::UpdateBases{new_bases}` are + public (`transaction.rs:308-456`). **A registration step is required and it is a + separate commit:** `initial_bases` on `Overwrite` is honored only in *create* + mode and is explicitly rejected on an existing dataset (*"OVERWRITE mode cannot + register new bases"*, `transaction.rs:1802-1808`), and a Lance transaction + carries exactly one `Operation`. So registering the base (`UpdateBases`) and + committing the base-referencing fragments are **two commits**, not one. They + are CAS-safe — `UpdateBases`/`Append`/`Overwrite`/`CreateIndex` each conflict + only with their own variant — and ride the per-table write queue. (`Overwrite` + also clears the index section, so an adopt that uses it rebuilds indexes — see + Component 6.) +- Lance's own `shallow_clone` (`dataset.rs:2505`, `Manifest::shallow_clone:237`) + is exactly this recipe applied whole-dataset: stamp `base_id` on every + `base_id == None` fragment, register one `BasePath`, clear the index section + to rebuild. We apply the same recipe **per table version, in place** (a new + version of the existing table dataset), which `shallow_clone` does not do + (it targets a fresh URI) — so we build it from the underlying primitives. + +LanceDB was surveyed for a reusable higher-level pattern; it has none (it +delegates branches/clone/maintenance straight to `lance` and never touches +`base_paths` or branch merge). This is the right layer to build at. + +### Component 1 — `TableStorage::stage_adopt_fragments` (new sealed primitive) + +A new staged primitive on the sealed `TableStorage` trait (`storage_layer.rs`), +shaped like the existing `stage_*` methods and consumed by the unchanged +`commit_staged`: + +```text +stage_adopt_fragments( + target: &SnapshotHandle, // target table dataset (== base) + source_branch_uri: &str, // {table}/tree/{source_branch} + source_fragments: &[Fragment], // source table version's fragments +) -> Result +``` + +It builds the referencing `DataFile`s via `create_data_file(path, Some(base_id))` +for each source fragment whose data is not already target-local, and produces a +**two-commit staged sequence** (per the registration constraint above): + +1. `Operation::UpdateBases{new_bases:[BasePath→source_branch_uri]}` — register + the source branch's data dir as a base on the target table dataset. +2. `Operation::Overwrite{fragments, schema}` — replace the target table with the + source's fragments (Overwrite because `target == base`: the table becomes the + source's image wholesale). The fragments carry `base_id` for the source-tree + ones and `None` for those already target-local. + +Both commits ride the existing `commit_staged` + per-`(table, branch)` write +queue + publisher CAS and are recovery-pinned together (the merge path already +does multi-commit-per-table under one sidecar — Component 3). No inline +HEAD/manifest coupling, so Invariant 1 holds by construction. (A +fragment-id-preserving variant — `Update` with `removed_fragment_ids` instead of +`Overwrite` — is required only if Component 6 Phase 2 index adoption is pursued, +since `Overwrite` clears indexes; Phase 1 uses `Overwrite` + rebuild.) + +Note on lazy fork: a source that lazy-forked from the target already references +the target's root fragments (`base_id` → target root) for unchanged rows; only +the source's own `tree/{branch}` fragments (`base_id == None`, physically in the +branch tree) become target-referencing after adoption. So the cross-tree +reference surface — and therefore the re-home work — is bounded to the genuinely +diverged fragments, not the whole table. + +### Component 2 — merge integration + +In `exec/merge.rs`, the `AdoptWithDelta` classification and its +`compute_adopt_delta` row walk + `publish_adopted_delta` copy are replaced: when +`target == base`, classify the table as **adopt-by-reference** and publish via +`stage_adopt_fragments(target, source_branch_uri, source_version_fragments)`. +The merge already holds everything required — `source_entry.table_branch`, +`table_version`, and `table_path` (`SubTableEntry`) — to locate the source's +fragments. `AdoptSourceState` is untouched (already optimal). `RewriteMerged` +stays on the existing row path until Phase 2. + +The per-table adopt commit still publishes through the unified merge manifest CAS +(one `graph_commit`), so cross-table atomic visibility (Invariant 2) is +unchanged. + +### Component 3 — recovery coverage + +`stage_adopt_fragments`'s commit advances Lance HEAD before the manifest publish, +so it is covered by the existing `SidecarKind::BranchMerge` recovery sidecar +(`recovery.rs`), which the merge path already writes with per-table version pins +and Phase-B confirmation. No new sidecar kind is required for the adopt step +itself; re-home (Component 4) runs inside `optimize`, so it rides the existing +`SidecarKind::Optimize` pin when in the same pass (a distinct `SidecarKind::Rehome` +is needed only if re-home runs independently of compaction). + +### Component 4 — the re-home reconciler (the load-bearing correctness piece) + +This is where the design earns its RFC. Referencing a branch's fragments from +the target creates a **dangling-reference / data-loss trap** that Lance does +**not** protect against (verified): + +- Lance's cleanup base-path protection is **descendant-directional** + (`cleanup.rs:655-868`): it keeps files a *descendant* branch references, but a + source branch is the target's *ancestor* (forked from it), so cleaning the + source never consults the target's manifest — the target's referenced files + look unreferenced and become GC-eligible. +- Branch **delete** (`refs.rs:552`) checks only descendant lineage, has zero + `base_paths` awareness, then `remove_dir_all(tree/{branch})` (`refs.rs:611`) — + silently deleting files the target still needs. +- There is **no native re-home API**: `UpdateBases` is add-only; `compact_files` + re-homes only fragments it happens to rewrite; `deep_clone` is wholesale. We + build re-home ourselves. + +**Re-home** copies each target fragment whose `DataFile.base_id` resolves into a +branch tree into the target's own `data/`, then commits +`Operation::DataReplacement{replacements}` swapping the `DataFile` to +`base_id = None`. `DataReplacement` is chosen over `Rewrite` because it keeps +**fragment ids and row ids stable** (so scalar/vector indexes stay valid and no +bitmap remap is needed); the object-store copy goes through the storage adapter +(no raw FS I/O, per the deny-list). Re-home runs: + +1. as a background pass folded into `optimize` (the natural maintenance home), + and +2. forced on demand before a referencing branch is reclaimed. + +**Concrete integration (verified):** re-home slots into `optimize_one_table` +(`db/omnigraph/optimize.rs`) as a Phase-B step — it needs the same `&mut Dataset` +handle (`open_dataset_head_for_write(...).into_dataset()`), runs under the same +per-`(table, main)` write queue, and advances HEAD before publish, so it can ride +the existing `SidecarKind::Optimize` pin when it runs in the same pass (a distinct +`SidecarKind::Rehome` is only needed if re-home runs independently of +compaction — decide by whether the two can be separated). It inherits +`optimize`'s bounded `maint_concurrency()` budget and its refuse-on-unrecovered / +skip-uncovered-drift guards. Progress is reported by extending the returned, +`#[non_exhaustive]` `TableOptimizeStats` with `bytes_rehomed` / `files_rehomed` / +`files_still_referenced` (the last modeled on the existing `pending_indexes` +"work this pass could not finish, reported not fatal" field) — additive, no +breakage. + +### Component 5 — branch-delete / reclaim guard + +The branch-delete reclaim path (`force_delete_branch`, +`reconcile_orphaned_branches`, `cleanup_deleted_branch_tables`) gains a +**reference check**: a branch tree is reclaimable only when no other branch's +manifest (the target's, in particular) holds a `base_id` reference into it. +While references remain, reclaim **re-homes them first** (Component 4) and only +then drops the tree. This is authority-derived and degrades to a no-op if Lance +ships an atomic multi-dataset branch merge — the same shape as the existing +branch-delete reconciler. The logical branch delete (manifest authority flip) +stays immediate; only physical reclamation waits, exactly as fork reclamation +already does. + +**Posture: prefer reachability-complete cleanup over a per-merge guard.** The +prior art (Iceberg, lakeFS, Delta + Unity Catalog) makes "don't delete what any +ref still reaches" a *property of GC computed over all references*, not a guard +bolted onto each merge — and Delta's own evolution (classic shallow clone left +the `FileNotFoundException` exposed; the fix moved reference tracking into the +catalog) shows the per-merge guard is the early-stage posture. OmniGraph is +well-placed to do the reachability version because the manifest is its single +source of truth (Invariant 15): `cleanup`'s existing per-table reconciler can +compute the live set as "every fragment any branch's manifest references, +following `base_id`" and refuse to delete anything in it — the same derive-from- +the-manifest shape as the index and orphaned-fork reconcilers. **Build the reachability sweep from the start (verified recommendation).** The +per-merge check and the reachability sweep are the *same code at a different call +site*, so deferring the sweep buys little and leaves a real cliff: the per-merge +guard protects only the *reclaim* path, not `cleanup_old_versions` itself, which +on an unrelated run would still GC a base-referenced file in a source tree (the +Delta "VACUUM-on-source" hazard the RFC cites). The integration point already +exists: `reconcile_orphaned_branches` (`optimize.rs`) — which `cleanup_all_tables` +already calls first — already enumerates all branches and caches each branch's +snapshot. Extend it to open each `(branch, table)` dataset at its pinned version, +union its manifest `base_paths` into a live set, then (a) thread that set into the +per-table `cleanup_old_versions` closure as a "refuse to delete in-set files" +filter and (b) into the reclaim decision so a tree with inbound references is +re-homed before `force_delete_branch`. **Cost (document it):** this adds +N_branches × N_tables per-table Lance manifest reads to `cleanup` — the same +*kind* of read the reconciler already does per branch, one level deeper — bounded +by the existing `maint_concurrency()` budget and the per-branch snapshot cache. + +**Concurrency (Q3, verified).** The guard inherits the existing in-process-only +serialization (the per-`(table, branch)` write queue + manifest CAS) and the +documented "fork reclaim is in-process-safe only" gap: it reads manifest +authority, acquires the queue, re-validates under it, then re-homes + drops — +structurally identical to today's `reconcile_orphaned_branches`. Cross-process, +the worst case stays *retry/contention, not data loss*, **provided the +reachability check + re-home always run before any `remove_dir_all`** (the +manifest CAS still mediates the publish winner). No new primitive is needed for +single-process or one-winner-CAS topologies; a cross-process serialization +primitive (a lease on the schema-apply lock branch, or an engine-level +`write_text_if_absent` lease) must be designed *with* that existing gap before +multi-process write topologies rely on the guard — not separately. + +Note the substrate gap this all works around: Lance cleanup is reachability-aware +only for **within-dataset descendant** branches (`retain_branch_lineage_files`, +base check scoped to `base_path.path == self.dataset.uri`), so the +**ancestor-references-descendant** case this merge creates (the target +referencing the source's data) and cross-dataset bases are not protected by Lance +today — the gap Issue 1 (below) raises upstream and Lance [#7185] partly covers. + +### Component 6 — indexes (scoped) + +Lance's `shallow_clone` clears the index section and rebuilds on access, and +`Operation::Overwrite` (Phase-1's data-adopt op) clears indexes too. +**Phase 1 mirrors that**: after an adopt, the index reconciler (`ensure_indices` +/ `optimize`, `build_indices_on_dataset_for_catalog`) rebuilds the table's indexes +as it does today (unchanged). This keeps Phase 1 correct and simple and adds **no** +index re-home surface, but does **not** remove the reindex cost — it defers it from +the synchronous merge path to the reconciler. + +**Phase 2** adopts the source's built indexes by reference, gated on evidence that +the reconciler reindex is the dominant cost for embedding-heavy tables. It is +feasible on Lance 7.0.0 (verified): `IndexMetadata` carries `base_id` + +`fragment_bitmap`, `Operation::CreateIndex{new_indices}` accepts a source index +verbatim, index files resolve from the base (`Dataset::indice_files_dir`), and the +bitmap stays valid because adopt preserves fragment ids. The staged-commit +mechanism already exists (our scalar-index staging builds `CreateIndex` via a +`StagedWrite`), so only a `stage_adopt_index` *construction* is new. But Phase 2 +has real extra cost the evidence bar must clear: (a) index files become +base-referenced, **doubling the re-home surface** (and there is no `IndexReplacement` +op — re-home must copy the index files and re-commit `CreateIndex` with +`base_id: None`); and (b) it forces the **data**-adopt op off `Overwrite` (which +clears indexes) onto a fragment-id-preserving sequence (`UpdateBases` → +`Update{removed_fragment_ids, new_fragments(base_id)}` → `CreateIndex`), coupling +the two. Hence Phase 2 is deferred, not folded into Phase 1. + +## Invariants & deny-list check + +- **Invariant 2 (manifest-atomic visibility):** preserved — adopt commits + publish through the unified merge CAS; one `graph_commit` per merge. +- **Invariant 5 (recovery is part of the commit protocol):** the adopt commits + reuse `SidecarKind::BranchMerge`; re-home rides `SidecarKind::Optimize` (it runs + inside `optimize`). No new HEAD-before-publish gap ships without sidecar coverage. +- **Invariant 7 / 15 (derived state, one source of truth):** the base reference + is a *view* into the source's fragments; re-home converges it target-local. + No maintained shadow copy; the manifest stays the source of truth. +- **Deny-list — "new write paths that advance Lance HEAD before manifest publish + without a recovery sidecar":** addressed (Component 3/4). +- **Deny-list — "mutating immutable substrate state in place":** not done — + adopt and re-home both commit new manifest versions and write new local files. +- **Deny-list — "raw filesystem I/O for cluster-stored state":** the re-home + copy goes through the storage adapter / `TableStore`. + +**New hazard explicitly closed:** the cross-branch dangling-reference trap. +Lance will not protect us (verified); Components 4 + 5 close it by construction. +This is a new Known Gap only if the guard is *not* shipped with the adopt path — +the RFC's position is that they land together. + +## Drawbacks & alternatives + +- **Do nothing.** Incremental and three-way merges keep OOMing at scale (the FF + case is already fine). Rejected — this is an active production failure mode. +- **Symptomatic change-detection fix** (cheaper `row_signature` via content hash + or row-version key). Investigated and rejected: it lands on deletion-bound + code, and it only trims change-*detection* memory — it does not remove the + row copy or the reindex, which are the actual OOM drivers. +- **Wait for native Lance branch merge.** This is the cleanest end state and the + substrate is actively building it: Lance [#7263] ("Branch merge and rebase", + open) specs *this design* — *"graft the source branch's base into the target + branch's `base_paths`… generalizes `shallow_clone`'s base grafting from 'the + whole manifest' to 'a single transaction's fragments,' replayed onto an + existing target branch"* — and depends on [#7185] ("Can not delete branches + referenced by other branches", open), which names the reclaim hazard and + proposes the guard. These are likely Lance 8.x/9.x (LanceDB already pins + `lance 9.0.0-beta.8`). The adopt design is the **bridge**: built so that when + native branch merge lands, `stage_adopt_fragments` + the re-home reconciler are + *removed*, not reworked. `exec/merge.rs` already anticipates this. +- **Re-home timing.** Eager re-home at merge would defeat the speedup (it's the + copy again). Re-home-at-delete-only would make the first delete after a big + merge slow. The design does lazy-background re-home (in `optimize`) with a + forced fallback at reclaim — amortized, with a hard correctness backstop. + +### Prior art + +This is a well-precedented pattern, not a novel one — which raises confidence in +the shape and tells us where it converges: + +- **Delta Lake `SHALLOW CLONE` → `DEEP CLONE`** is the closest named twin. + Shallow clone references the source's data files by path without copying (= our + adopt); the docs carry the *identical* hazard ("run `VACUUM` on the source + table → clients can no longer read those data files → `FileNotFoundException`") + and the *identical* mitigations: deep clone (= re-home to independence) and + Unity Catalog cross-clone reference tracking (= our reclaim guard, lifted into + the catalog/GC). +- **Reflink / copy-on-write filesystems** (`cp --reflink`, btrfs/XFS/APFS): shared + extents with refcounting; deleting one referer doesn't free shared blocks; + writing/breaking the share copies. The same shape at the block layer. +- **Iceberg, lakeFS, Nessie, Dolt**: merge by referencing shared immutable + objects (metadata-only, zero-copy) and GC by **reachability across all refs**. + Iceberg/lakeFS make "don't delete what any ref still reaches" a *property of + GC*, not a per-merge guard — see the GC-posture note below. + +[#7263]: https://github.com/lance-format/lance/issues/7263 +[#7185]: https://github.com/lance-format/lance/issues/7185 + +## Reversibility + +**Substrate-adjacent and partly irreversible** — the reason this is an RFC. The +adopt commit puts `base_id` references from the target's manifests into branch +trees: an on-disk layout the readers must understand and the lifecycle must +protect. A single bad merge is recoverable (re-home or rewrite the references +away), but once branches are reclaimed in reliance on re-home, the layout +decision is committed. The merge *contract* (result, conflicts, atomicity) is +unchanged and the change is gated behind the existing staged-write + recovery +machinery, but the format/lifecycle dimension earns the up-front design review. + +## Unresolved questions + +The lifecycle questions (cross-process safety, re-home throughput, GC posture) +were investigated against the code and are now resolved into the design above: +re-home rides `optimize` (Component 4), the reachability sweep is built from the +start in `reconcile_orphaned_branches` (Component 5), and the guard inherits the +documented in-process-only gap (Component 5, Q3). What remains genuinely open: + +- **Three-way (`RewriteMerged`) — deferred, confirmed not feasible now.** + Investigated: our three-way path is row-at-a-time with no fragment identity, and + Lance fragments are whole-or-nothing adopt units (any fragment containing a + both-changed row must be rewritten). Fragment-granular partial adopt would need + a different algorithm (per-fragment changed-row maps) and is a substantial + redesign the substrate's native branch-merge ([#7263]) would supersede. Phase 1 + leaves three-way on the existing row merge. Open: is it worth a Phase-3 attempt, + or wait for [#7263]? +- **Index adoption — Phase 2, evidence-gated.** Feasible (verified) but it doubles + the re-home surface to index files (no `IndexReplacement` op) and forces the + data-adopt off `Overwrite` onto a fragment-id-preserving sequence. Open: does + the reconciler reindex actually dominate for embedding-heavy tables enough to + clear that bar, or is Phase-1 rebuild sufficient indefinitely? +- **Cross-process write topologies.** The guard is safe in-process and under + one-winner-CAS; before multi-process *writers* on one graph rely on it, a + cross-process lease (on the schema-apply lock branch, or an engine + `write_text_if_absent` lease) must be designed — together with the existing + "fork reclaim is in-process-safe only" gap, not separately. +- **Upstream filing (filed).** Two validated gaps, now raised upstream: + Lance [#7514] (`cleanup_old_versions` does not protect `base_paths` references in + the ancestor / cross-dataset direction — the live Delta "VACUUM-on-source" + hazard; Lance protection is within-dataset-descendant only) and Lance [#7515] + (no in-place "materialize/promote shallow clone to independent" op — `deep_clone` + makes a new dataset, `UpdateBases` is add-only). Closing these upstream lets this + RFC's reachability sweep + re-home reconciler degrade to thin wrappers. + +[#7514]: https://github.com/lance-format/lance/issues/7514 +[#7515]: https://github.com/lance-format/lance/issues/7515