* MR-854: convert engine call sites to &dyn TableStorage; demote legacy methods
Phase 1b: every db.table_store.X(...) call site converts to
db.storage().X(...), reaching the storage layer through the sealed
TableStorage trait (returns &dyn TableStorage). Opaque SnapshotHandle
and StagedHandle replace bare lance::Dataset and Transaction in the
threaded values.
Phase 9: the inherent inline-commit methods on TableStore
(append_batch, merge_insert_batch{,es}, overwrite_batch,
create_btree_index, create_inverted_index) demote from pub to
pub(crate). Their only remaining direct users are table_store.rs
itself and the bulk loader's LoadMode::{Append, Overwrite, Merge}
concurrent fast-paths in loader::write_batch_to_dataset (no
two-phase shape in Lance 4.0.0 — closes after lance#6658 and #6666).
Docs:
- invariants.md \u00a7VI.23: drop "at the writer-trait surface"
qualifier; staged primitives are now the only engine surface.
- runs.md: residual matrix shrinks to delete_where and
create_vector_index (the two upstream-blocked residuals).
- forbidden_apis.rs: replace transitional language with the
current allow-list shape (table_store.rs + loader concurrent
fast-path only).
Files touched:
- changes/mod.rs, db/omnigraph.rs (+export/optimize/schema_apply/
table_ops.rs), exec/{merge,mod,mutation,staging}.rs,
loader/mod.rs, storage_layer.rs, table_store.rs,
tests/forbidden_apis.rs, docs/{invariants,runs}.md.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
* MR-854: replace test-only inline-commit append callers with local Lance helpers
After demoting TableStore::append_batch from pub to pub(crate), the
integration tests in tests/recovery.rs and tests/staged_writes.rs
that previously called store.append_batch(...) directly to simulate
HEAD-ahead-of-manifest drift can no longer access the inherent
method. Replace those calls with small in-test helpers that do a raw
Dataset::append (the same body the inherent method runs).
- tests/helpers/mod.rs gains lance_append_inline (shared helper).
- tests/staged_writes.rs gets a file-local lance_append_inline_local
(staged_writes.rs does not import helpers::).
- tests/recovery.rs drops the unused TableStore import in the one
function whose store binding became unused after the conversion.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
* MR-854: retrigger CI for flaky Test Workspace job
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
* MR-854: convert remaining table_store call sites in export.rs / read_blob
Two leftover `self.table_store.X` / `db.table_store.X` call sites were
missed in the initial sweep — flagged by Devin Review on PR #86. Both
now go through the trait surface:
- `entity_from_snapshot` (db/omnigraph/export.rs): switch from
`db.table_store.open_snapshot_table` + `db.table_store.scan` to
`db.storage().open_snapshot_at_table` + `db.storage().scan`.
- `read_blob` (db/omnigraph.rs): replace
`snapshot.open(table_key)` + `self.table_store.first_row_id_for_filter`
with `self.storage().open_snapshot_at_table` +
`self.storage().first_row_id_for_filter`. The follow-up
`take_blobs` call still needs an `Arc<Dataset>` (it's a Lance blob
accessor not surfaced through the trait), so we hand off via
`SnapshotHandle::into_arc()` with a comment.
After this commit, no engine code outside `table_store.rs` reaches the
inherent `TableStore` API — the docs/runs.md and docs/invariants.md
claim is now uniformly true.
Co-Authored-By: Ragnor Comerford <ragnor.comerford@gmail.com>
* MR-854: post-rebase doc fixes (Lance 6.0.1, MR-A framing, into_dataset note)
Reviewer feedback on the rebased PR:
* docs/dev/writes.md residuals matrix: drop demoted methods from the trait-surface table (now `pub(crate)`); keep only the two genuine trait-surface residuals (`delete_where`, `create_vector_index`); reframe under MR-A (Lance v7.x bump) per docs/dev/lance.md.
* tests/forbidden_apis.rs: update transitional allow-list header to (a) drop the truncate_table mislabel (truncate_table is a Lance Dataset method, not a TableStore method — overwrite_batch's internal call), (b) reframe trait-surface residuals under MR-A / Lance #6666.
* crates/omnigraph/src/storage_layer.rs::SnapshotHandle::{into_arc, into_dataset}: add single-ref invariant doc — both consume Arc via try_unwrap-or-clone; sibling SnapshotHandle clones across an await point force a deep Dataset clone.
* Replace lance-4.0.0 version refs with lance-6.0.1 in active source/test/dev-doc comments (storage_layer.rs, table_store.rs, table_ops.rs, schema_apply.rs, merge.rs, recovery.rs, staged_writes.rs, consistency.rs, docs/dev/execution.md, docs/user/query-language.md). Historical refs in docs/releases/v0.4.1.md and the canonical "Lance 4.0.0 → 6.0.1 migration" line in docs/dev/lance.md left intact.
No engine code changes.
* MR-854: update docs/dev/invariants.md Storage trait row + gap entry
Reviewer feedback: the docs reorg landed; the invariant row now lives in
docs/dev/invariants.md with stable headings (no more numbered §VI.23).
Update two pieces to reflect MR-854 completion:
* Status table 'Storage trait' row: was 'full call-site migration ... incomplete';
now 'engine call sites all route through db.storage() (MR-854); inline-commit
inherent methods are pub(crate)-demoted; capability/stat surfaces are roadmap'.
* 'Known Gaps' 'Storage abstraction' entry: was 'older inherent TableStore call
sites and inline residuals remain'; now names the closed scope (MR-854 — call
sites migrated, methods demoted, loader fast-paths) and the remaining
trait-surface residuals under MR-A (Lance v7.x bump) and Lance #6666.
Cross-links to docs/dev/lance.md and docs/dev/writes.md so the framing stays
co-located with the canonical Lance surface tracking.
* MR-854: remove dead inline-commit methods from the storage surface
The loader concurrent fast-path (write_batch_to_dataset) is only reached
for LoadMode::Overwrite — Append/Merge route through MutationStaging — so
its Append/Merge arms were unreachable. Collapse it to overwrite-only and
drop the now-unused mode params, which removes the only callers of:
- TableStorage::append_batch + TableStorage::merge_insert_batches (trait)
- TableStore::merge_insert_batch + merge_insert_batches (inherent)
create_btree_index / create_inverted_index had zero callers anywhere
(scalar index builds use the stage_* primitives). Remove both from the
trait and the inherent impl.
Inherent append_batch stays pub(crate): overwrite_batch and recovery
tests use it. Migrate the one trait-append_batch test caller
(seed_person_row) to stage_append + commit_staged. The merge_insert
FirstSeen-workaround rationale moves from the deleted merge_insert_batch
into stage_merge_insert (now the sole merge path). No behavior change.
Also corrects the inaccurate loader residual comment (the prior text
blamed Lance #6658/#6666, which are the delete and vector-index issues,
for keeping overwrite inline; a stage_overwrite primitive already exists
and schema_apply uses it).
* MR-854: seal db.storage() to staged-only; move residuals to InlineCommitResidual
Split the three remaining inline-commit writes (overwrite_batch,
delete_where, create_vector_index) off the TableStorage trait onto a new
sealed InlineCommitResidual trait, reachable only via the explicit
Omnigraph::storage_inline_residual() accessor. db.storage() now exposes
only staged primitives + reads, so engine code cannot couple a write
with a Lance HEAD advance through the default surface — MR-793 acceptance
§1 ("no public method commits as a side effect of writing") now holds by
construction, not by review + naming.
Call sites moved to storage_inline_residual(): loader overwrite
fast-path, the three mutation delete_where paths, the branch-merge
delete, and the vector-index build. Impl bodies are unchanged (same
delegation to the pub(crate) inherent methods); this is a pure surface
reshape with no behavior change.
The residual trait holds two genuinely upstream-blocked methods
(delete_where -> Lance #6658/v7.x, create_vector_index -> Lance #6666)
plus overwrite_batch, kept for the loader's cross-table bulk-overwrite
concurrency until its staged migration lands (tracked follow-up).
* MR-854 docs: describe the staged-only seal; fix stale Lance index URLs
- writes.md / invariants.md / AGENTS.md: the inline-commit residuals now
live on InlineCommitResidual behind db.storage_inline_residual(), so
acceptance §1 holds by construction rather than 'option (b)' per-method
enumeration. Drop the inaccurate 'until Lance exposes
Operation::Overwrite { fragments }' claim (that op exists; stage_overwrite
already builds it) and reframe overwrite_batch as a removable legacy
residual gated on the loader's bulk-overwrite concurrency.
- forbidden_apis.rs: rewrite the allow-list doc for the split surface.
- lance.md: the index spec pages moved from /format/table/index/ to
/format/index/ in Lance 6.x (the old paths 404). Fix all 13 URLs.
* MR-854: fix stale lance-4.0.0 comment refs flagged in review
Addresses greptile (exec/merge.rs) and aaltshuler's stale-version blocker:
update lance-4.0.0 -> 6.0.1 in the comment/doc refs within this PR's
footprint (exec/merge.rs, exec/mutation.rs, docs/dev/writes.md). Also
corrects exec/merge.rs to cite lance#6666 (not #6658) for
build_index_metadata_from_segments — that is the vector-index segment-commit
API; #6658 is the two-phase delete. (Pre-existing 4.0.0 refs in untouched
files like architecture.md/storage.md are main's incomplete migration
cleanup, left out of scope.)
* fix(storage): stage loader overwrites
* fix(storage): stage empty schema rewrites
---------
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: Ragnor Comerford <ragnor.comerford@gmail.com>
Co-authored-by: Ragnor Comerford <hello@ragnor.co>
15 KiB
Architectural Invariants
Type: standing review checklist Status: living document Audience: anyone proposing, reviewing, or implementing an OmniGraph change
This file is intentionally short. It records the rules that should be in working memory for every non-trivial change. Detailed mechanics live in the area docs linked below.
Use it this way:
- Review the change against Hard Invariants and the Deny-list.
- If code and docs disagree, either fix the code or add/update a Known Gap.
- Keep implementation ledgers, roadmap detail, and historical MR notes in the per-area docs. This file is the filter, not the encyclopedia.
Hard Invariants
-
Respect the substrate. Lance owns columnar storage, per-dataset versioning, fragments, branches, compaction, cleanup, and index primitives. DataFusion should own relational execution where it fits. Do not add custom WALs, transaction managers, buffer pools, page formats, or local clones of substrate behavior. Read lance.md before guessing.
-
Graph visibility is manifest-atomic. Lance commits are per dataset. OmniGraph's graph-level atomicity comes from publishing one manifest update for the whole graph, guarded by expected table versions and sidecar recovery. No write path may make a subset of touched node/edge tables visible as a graph commit.
-
A query reads one snapshot. Query execution captures a manifest snapshot for its lifetime. Do not re-read branch head mid-query to discover newer table versions.
-
Mutations publish at one boundary. A
mutate_asorloadoperation accumulates constructive writes, commits each touched table at the end, then publishes one manifest update. Do not commit per statement. Delete-only queries are the documented inline residual; the parse-time D2 rule prevents mixing deletes with insert/update until Lance exposes two-phase delete. Read writes.md and execution.md. -
Recovery is part of the commit protocol. Writers that can advance Lance HEAD before manifest publish must write
__recovery/{ulid}.jsonsidecars.Omnigraph::openin read-write mode runs the all-or-nothing sweep, andrefreshruns roll-forward-only recovery for long-lived processes. Do not add a new writer kind without sidecar coverage or an explicit proof that no Lance HEAD can move before manifest publish. -
Strong consistency is the default. Reads are snapshot-isolated, writes are durable before acknowledgement, and branch reads observe the current committed graph state. Any eventual-consistency mode must be explicit, read-only, auditable, and non-default.
-
Indexes are derived state. Reads must see the correct result for the branch they read even when index coverage is partial. Expensive index work should converge from manifest state instead of extending the critical write path. Scalar staged index builds and vector inline residuals are documented in writes.md and indexes.md.
-
Schema identity survives renames. Accepted schema identity must remain stable across type and property renames. Rename support belongs in migration planning, not in "drop and recreate" behavior. See the known gap below.
-
Schema/data integrity failures are loud. Type errors, required-field misses, invalid edge endpoints, cardinality violations, and unsupported mixed mutation modes fail before a graph commit is published. The system must not invent placeholder nodes or silently weaken integrity.
-
Query semantics are first-class IR concepts. Search modes, mutations, polymorphism, traversal, retrieval scores, imports, and policy predicates belong in typed AST/IR/planner structures. Do not smuggle semantics through strings, side tables, global state, or transport-specific flags.
-
Transport/auth stay at the boundary. Kernel crates should not depend on HTTP, OpenAPI, bearer-token parsing, or future transport protocols. The server resolves bearer tokens to actors; clients cannot set actor identity directly.
-
Bearer-token plaintext is not retained. Server startup hashes bearer tokens, authentication uses constant-time comparison, and request handling carries only the resolved actor identity and hash-derived match state.
-
Operational failures are bounded and observable. Timeout, memory, OOM, partial result, recovery, and conflict paths must fail loudly or degrade in a documented way. If a metric affects plan choice or operator behavior, it must be exposed through the relevant trait or observability surface.
-
Tests match the boundary being changed. Prefer extending the existing test that owns the area. Planner changes need planner-level coverage, storage changes need storage/recovery coverage, and end-to-end tests are not a substitute for missing lower-level assertions. Read testing.md before adding tests.
Current Truth Matrix
| Area | Current state | Source |
|---|---|---|
| Multi-table commit | Manifest CAS plus recovery sidecars; not a single Lance primitive | writes.md, architecture.md |
| Constructive mutations | In-memory MutationStaging, one end-of-query table commit per touched table, then one manifest publish |
writes.md, execution.md |
| Deletes | Inline-commit residual; delete-only queries allowed, mixed insert/update/delete rejected by D2 | query-language.md, writes.md |
| Branch delete | Manifest is the single authority, flipped atomically first; per-table forks + commit-graph branch 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, maintenance.md |
| Schema validation | Type checks, required fields, defaults, edge endpoint checks, and edge cardinality are enforced on write paths | schema-language.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<String> 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 |
| Storage trait | TableStorage (via db.storage()) is staged-only; the inline-commit residuals (delete_where, create_vector_index) are 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, architecture.md |
| Index lifecycle | ensure_indices is explicit today; reconciler-based convergence is roadmap |
indexes.md, maintenance.md |
| Traversal IDs | Runtime still builds TypeIndex; Lance stable row-id based graph IDs are roadmap |
architecture.md, query-language.md |
| Auth | Bearer token hashing and server-side actor resolution are implemented at the HTTP boundary | server.md, policy.md |
| Tests | Tempdir-backed Lance tests are the current substrate; there is no MemStorage test backend |
testing.md |
The branch-delete reconciler is authority-derived: it reclaims orphaned forks today and degrades to a no-op if Lance ships an atomic multi-dataset branch operation, so the design composes with that future rather than blocking it. This is the same shape as invariant 7 (indexes are derived state); prefer it over a recovery-sidecar-style approach for any new multi-dataset metadata operation, since the sidecar would be scaffolding to remove once the substrate closes the gap.
Known Gaps
Do not hide these behind invariant wording. Either move them forward or keep them explicit.
- Rename-stable schema identity: the invariant is that accepted IDs survive
renames. The current compiler still derives type IDs from
kind:name; this must be fixed before relying on renamed IDs across accepted schemas. - Storage abstraction:
TableStorageis present, sealed, and canonical for staged writes. MR-854 sealed it:db.storage()exposes only staged primitives- reads, and the inline-commit residuals are split onto a separate sealed
InlineCommitResidualtrait reached viadb.storage_inline_residual(), so a new writer cannot couple a write with a HEAD advance through the default surface. The dead legacy methods (append_batchon the trait,merge_insert_batch{,es},create_{btree,inverted}_index) were removed. The remaining residuals aredelete_where(gated on MR-A — Lance v7.x bump) andcreate_vector_index(gated on Lance #6666); see lance.md and writes.md. New write paths should use the staged shape unless a documented Lance blocker applies.
- reads, and the inline-commit residuals are split onto a separate sealed
- Deletes and vector indexes:
delete_whereand vector index creation still advance Lance HEAD inline because the required public Lance APIs are missing. Keep D2 and recovery coverage in place until those residuals are removed. - Blob-column compaction: Lance
compact_filesmis-decodes blob-v2 columns under its forcedBlobHandling::AllBinaryread ("more fields in the schema than provided column indices"), sooptimizeskips any table with aBlobproperty — reportingSkipReason::BlobColumnsUnsupportedByLance(loud, not a silent drop) behind theLANCE_SUPPORTS_BLOB_COMPACTIONgate. Reads and writes are unaffected; only space/fragment reclamation on blob tables is deferred. Remove the skip when the upstream Lance fix lands — thelance_surface_guards.rs::compact_files_still_fails_on_blob_columnsguard turns red on that bump to force it. - Manifest→commit-graph publish atomicity: a graph commit advances
__manifest(the visibility authority) and then appends_graph_commitsas two separate writes (commit_updates_with_actor_with_expected, failpointgraph_publish.before_commit_append). A crash between them leaves the manifest at version N with no commit-graph row for N. Live reads and durability are unaffected — the live version resolves via the manifest (GraphCoordinator::version()), not the commit-graph head — and the open-time recovery sweep does NOT repair it (lance_head == manifest_pinnedclassifiesNoMovement; a recovery sidecar would not change this). Impact is bounded to commit history:commit listmisses N, time-travel by commit id to N fails, and merge-base loses a node (a likely-benign off-by-one re-merge). This affects every publish, not a specific maintenance command. Eventual fix: make the commit graph reconcilable from the manifest (or the two writes atomic) — not a recovery-sidecar concern. - Planner capability/stat surfaces: cost-aware planning, complete capability advertisement, and explain-with-cost are roadmap. Do not describe them as implemented.
- Traversal execution: current multi-hop execution still uses
TypeIndex, ad-hoc ID filtering, and eager materialization in places. Stable row IDs, SIP, and factorization are target patterns, not current fact. - Retrieval ranks: hybrid search works, but rank/score are not yet carried everywhere as ordinary columns through the plan.
- Policy pushdown and
Source: Cedar enforcement is at the HTTP boundary today, and imports are still loader-shaped. Planner predicates and a unifiedSourceoperator are roadmap. - Resource bounds: some operations still lack enforced per-query memory or time budgets. New long-running work should add explicit bounds rather than widening the gap.
Deny-list
If a proposal fits one of these, the burden is on the proposer to prove why the case is exceptional.
- Custom WAL, transaction manager, buffer pool, page format, or storage engine.
- Per-table graph publishing outside the manifest publisher.
- Re-reading current branch head during a query instead of using the captured snapshot.
- New write paths that can advance Lance HEAD before manifest publish without a recovery sidecar.
- Cross-query
BEGIN/COMMITtransactions in the OSS engine. Use branches and merges for multi-query workflows. - Acknowledging writes before durable Lance and manifest persistence.
- Silent fallback to eventual consistency, partial results, or dropped rows.
- State that drifts from Lance or the manifest when it can be derived.
- Job queues for manifest-derivable state where a reconciler is the right shape.
- Synchronous inline vector/FTS index rebuilds on the query commit path, except for documented Lance API residuals.
- Side-channels for query semantics: hidden globals, magic strings, transport flags, or out-of-band metadata.
- Cost-blind plan choice when statistics are available or required.
- Hidden statistics for behavior that affects planning or operator choice.
- Hash-map iteration order in result ordering, plan choice, or migration output.
- String-flattened SQL/filter generation when a structured pushdown API is available.
- Eager multi-hop cross-product materialization when factorization fits.
- Ad-hoc
IN-list filtering where SIP or another structured selectivity path fits. - Discarding retrieval score/rank before fusion or projection decisions.
- Auto-creating placeholder nodes for orphan edges.
- Wire-protocol-specific code in compiler or engine crates.
- Cloud-only correctness fixes or forks of the OSS engine for correctness.
- Mutating immutable substrate state in place, including Lance fragments or index segments.
- Shipping observable behavior as if it were not part of the contract. Output ordering, error text, timestamp precision, defaults, and latency profiles all become dependencies once exposed.
Review Checklist
Use this as yes/no/NA for any non-trivial design or PR:
- Does it respect Lance/DataFusion instead of rebuilding them?
- Does it preserve manifest-atomic graph visibility?
- Does every query keep one snapshot for its lifetime?
- Do mutations publish once at the commit boundary?
- Can every Lance-HEAD-before-manifest gap recover all-or-nothing?
- Are schema and edge integrity checks strict by default?
- Are query semantics represented in AST/IR/planner structures?
- Are transport, auth, and policy boundaries preserved?
- Are failures bounded, typed, and observable?
- Are result ordering and plan choices deterministic within a snapshot?
- Are stats/capabilities exposed when behavior depends on them?
- Are existing known gaps left no worse and documented if touched?
- Does the test live at the same boundary as the change?
- Does the change avoid every deny-list pattern, or justify the exception?
Maintenance Policy
Update this file when an invariant changes, a known gap opens or closes, or a new review anti-pattern deserves deny-list treatment. Prefer stable headings over numbered sections so other docs can link here without churn.
Removing or relaxing a hard invariant requires the same review process as code. Adding a known gap is acceptable when it makes reality explicit; leaving stale claims is not.