omnigraph/docs/dev/writes.md
Ragnor Comerford 1bed998052
fix(engine): scalar index coverage + filter literal coercion (query latency) (#216)
* fix(engine): lower date/datetime filter literals as typed Arrow scalars

`literal_to_expr` lowered `Date`/`DateTime` query literals as Utf8 strings,
relying on DataFusion implicit casts. Against a physical `Date32`/`Date64`
column that can coerce the column side (`CAST(col AS Utf8)`), which defeats a
scalar BTREE and degrades the scan to a full filtered read. Lower to typed
`Date32`/`Date64` scalars instead (reusing the loader's
`parse_date32_literal`/`parse_date64_literal`, already used by the in-memory
comparison arm), so the predicate stays a direct column comparison and the
index is used. Malformed literals fall back to the Utf8 string so pushdown
behavior never regresses.

Tests: unit goldens asserting the lowered literal is typed (red before, green
after) + inline-binding pushdown equality in literal_filters confirming the
epoch conversion selects the right rows.

* fix(engine): build scalar BTREE for enum and orderable-scalar @index columns

`build_indices_on_dataset_for_catalog` only handled `String` (-> FTS) and
`Vector` (-> vector). Enums are physically `String`, so an enum `@index`
column (e.g. `status`) got an FTS inverted index, which Lance never consults
for `=`; and `DateTime`/`Date`/numeric/`Bool` `@index` columns fell through
and built nothing. Both meant equality/range filters degraded to full scans
with `indices_loaded=0`.

Dispatch index kind by property type via a shared `node_prop_index_kind`:
enum + orderable scalar -> BTREE, free-text String -> FTS, Vector -> vector,
list/Blob -> none. The helper is shared by the builder and
`needs_index_work_node` so they cannot drift — the latter decides recovery-
sidecar pinning, and under-reporting would leave a HEAD-advancing index build
uncovered (invariant 5).

Tests: scalar_indexes.rs asserts enum/DateTime/numeric @index columns report
`IndexCoverage::Indexed` while free-text String/un-annotated columns stay
`Degraded` (negative control). Docs: docs/user/indexes.md.

* feat(engine): reindex in optimize to keep index coverage current

A scalar/FTS/vector index only covers the fragments it was built over. Rows
appended after the build (e.g. `ingest --mode merge`, whose commit does not
rebuild an existing index) are scanned unindexed, and `compact_files` rewrites
fragments out of coverage. Nothing folded them back in, so coverage decayed as
the graph grew — even the id/src/dst BTREEs that power traversal.

`optimize_one_table` now runs Lance `optimize_indices` after `compact_files`
(incremental merge, not retrain — the same compact->optimize_indices sequence
LanceDB's `optimize()` uses) and enters the publish path on compaction work OR
stale index coverage (new `TableStore::has_unindexed_fragments`, reusing the
fragment_bitmap logic). `optimize_indices` is a committing call with no
uncommitted variant in lance-6.0.1, so it is an inline-commit residual covered
by the existing `SidecarKind::Optimize` recovery sidecar spanning both ops.
Blob-bearing tables are still skipped (the Lance blob-compaction bug is
compaction-specific; reindex-for-blob deferred as a noted follow-up).

Tests: maintenance.rs asserts an appended fragment is uncovered before and
covered after optimize, and idempotency holds (second pass is a no-op).
lance_surface_guards pins the `optimize_indices` signature and its incremental-
coverage behavior. The existing optimize Phase-B recovery failpoint now also
exercises a crash after reindex. Docs: maintenance.md, writes.md, invariants.md,
lance.md, AGENTS.md.

* fix(engine): coerce pushdown filter literals to the column type

Filter literals were pushed to Lance in their natural Arrow type (every integer
Int64, every float Float64). Against a narrower indexed column DataFusion widens
to the literal's type and casts the COLUMN (`CAST(n32 AS Int64)`), which defeats
the scalar BTREE and degrades to a full filtered read. A physical-plan probe
confirms it: an Int32 column filtered by an i32 literal uses `ScalarIndexQuery`;
by an i64 literal it does not.

Thread the scan's `arrow_schema` through `build_lance_filter_expr` ->
`ir_filter_to_expr` and coerce each literal operand to the opposite column's
exact Arrow type, reusing `projection::literal_to_array` + `arrow_cast` (the same
path the in-memory arm uses, so the two arms agree). Coercion never demotes a
filter to None: on failure it falls back to the natural literal, because a node
scan has no in-memory fallback for inline filters.

Supersedes the date-specific change in e4ef67b (PR1): the probe shows dates were
never index-defeated — temporal coercion casts the LITERAL, not the column — so
PR1's index-use rationale was wrong though harmless. The generic coercion
subsumes it; `literal_to_expr`'s date arms revert to the natural Utf8 fallback,
and its unit tests now assert the live coerced path.

Tests: surface guard `scalar_index_use_requires_matched_literal_type` pins the
substrate behavior (matched -> index, widened -> column-cast full scan); unit
tests cover Int32/UInt32/Float32 coercion, range op, reversed operand order, and
the natural fallback; `literal_filters` adds an I32 column with equality + range
and an F32 pushdown case.

* fix(engine): only coerce filter literals when the cast is lossless

The literal coercion in f064121 narrowed unconditionally. typecheck permits
numeric cross-type comparisons (`types_compatible`), so an out-of-domain literal
reaches `literal_to_typed_expr` and casts lossily: a fractional float vs an
integer column truncates (`{ count: 2.7 }` -> `count = 2`, wrongly matching the
count=2 row) and an out-of-range integer overflows to null (`count < 3e9` on I32
-> `count < NULL` -> empty). Both silently change results, and a node scan has no
in-memory fallback for inline filters.

Add a lossless guard for integer targets: round-trip the cast back to the natural
type and, on mismatch, return None so the caller keeps the natural literal
(correct via DataFusion coercion; the index is just unused for that out-of-domain
predicate). Float targets stay coerced -- narrowing F64 -> F32 is the column's own
precision domain, not a value error.

Resolves the two valid review findings on PR #216 (Codex float truncation, Greptile
out-of-range). Tests: unit cases for fractional/out-of-range fallback vs
whole-float/in-range coerce vs F32 exemption; e2e `{ count: 2.7 }` returns no rows.
2026-06-14 16:31:19 +02:00

21 KiB

Direct-Publish Write Path

History: the Run state machine and __run__<id> staging branches were removed in MR-771 (shipped v0.4.0). Writes now go directly to the target table; this document specifies that direct-publish path.

mutate_as and load write directly to the target table and call ManifestBatchPublisher::publish once at the end with expected_table_versions (the per-table manifest versions captured before the first write). Cross-table OCC is enforced inside the publisher; the publisher's row-level CAS on __manifest is the single fence.

What this means in practice

  • No RunRecord, no _graph_runs.lance, no _graph_run_actors.lance.
  • No omnigraph run * CLI subcommands and no /runs/* HTTP endpoints.
  • No __run__<id> staging branches; __run__* is no longer a reserved name. The branch-name guard was removed in MR-770, and any stale __run__* branch on an upgraded graph is swept off __manifest by the v2→v3 internal-schema migration on first read-write open. (The inert _graph_runs.lance bytes remain until a delete_prefix primitive lands.)
  • Cancelled mutation futures leave no graph-level state — only orphaned Lance fragments, which the existing omnigraph cleanup pipe reclaims.

Read-your-writes within a multi-statement mutation

A .gq query with multiple ops (e.g. insert Person … insert Knows …) must observe earlier ops' writes when validating later ops (referential integrity, edge cardinality). After MR-794 step 2+ this is implemented via an in-memory MutationStaging accumulator in crates/omnigraph/src/exec/staging.rs, shared by both mutate_as and the bulk loader:

  • On the first touch of each table, the pre-write manifest version is captured into expected_versions[table_key] (the publisher's CAS fence at end-of-query).
  • Each insert/update op pushes a RecordBatch into the per-table pending accumulator. Lance HEAD does not advance during op execution.
  • Read sites (validation, predicate matching for update) consume TableStore::scan_with_pending, which scans committed via Lance and applies the same SQL filter to the pending batches via DataFusion MemTable. Same-query writes are visible to subsequent reads.
  • At end-of-query, MutationStaging::finalize issues exactly one stage_* + commit_staged per touched table (concatenating accumulated batches; merge-mode dedupes by id, last-write-wins), and the publisher publishes the manifest atomically across all touched sub-tables. Cross-table conflicts surface as ManifestConflictDetails::ExpectedVersionMismatch.
  • Deletes still inline-commit. Lance's Dataset::delete is not exposed as a two-phase op in 6.0.1; deletes go through delete_where immediately and record their post-write state in MutationStaging.inline_committed. The parse-time D₂ rule (below) prevents inserts/updates from coexisting with deletes in one query, so the inline path is safe for delete-only mutations.

This upholds the manifest-atomic mutation and read-your-writes invariants tracked in docs/dev/invariants.md.

D₂ — parse-time mixed-mode rejection

A single mutation query is either insert/update-only or delete-only. Mixed → rejected at parse time with a clear error directing the user to split the query. Reason: mixing creates ordering hazards (insert→delete on the same row would silently no-op because the staged insert isn't visible to delete; cascading deletes of just-inserted edges break referential integrity). Until Lance exposes a two-phase delete API, the parse-time rejection keeps both paths atomic and correct. Tracked: MR-793, plus a Lance-upstream ticket.

MR-793 status (storage trait two-phase invariant) — partial

MR-793 hoists the staged-write pattern into a TableStorage trait surface with sealed-trait enforcement and opaque SnapshotHandle / StagedHandle types — see crates/omnigraph/src/storage_layer.rs. The trait is the canonical surface for new engine code; existing call sites still use the inherent TableStore methods (mechanical migration deferred to a follow-up cycle — tracked).

Three writers have been migrated onto staged primitives:

  • ensure_indices (db/omnigraph/table_ops.rs::build_indices_on_dataset_for_catalog) — scalar indices (BTree, Inverted) use stage_create_*_index + commit_staged. Which index a @index/@key property gets is dispatched by type via node_prop_index_kind (enum + orderable scalar → BTree, free-text String → Inverted/FTS, Vector → vector). Vector indices stay inline (residual — Lance build_index_metadata_from_segments is pub(crate) in 6.0.1; companion ticket to lance-format/lance#6658 needed). This build is existence-gated (it creates a missing index over current fragments); folding fragments appended afterward into an existing index is optimize's optimize_indices pass — an inline-commit residual, not a staged write (Lance exposes no uncommitted index-optimize), covered by the optimize recovery sidecar (see maintenance.md).
  • branch_merge::publish_rewritten_merge_table (exec/merge.rs) — merge_insert now uses stage_merge_insert + commit_staged. Deletes stay inline (Lance #6658 residual).
  • schema_apply rewritten_tables (db/omnigraph/schema_apply.rs) — rewrites use stage_overwrite + commit_staged, including empty-table rewrites via a zero-fragment Lance Operation::Overwrite.

A defense-in-depth integration test (tests/forbidden_apis.rs) walks engine source and fails if non-allow-listed code calls Lance's inline-commit APIs directly. The trait surface itself is the primary enforcement (sealed + only-callable-via-trait once call sites land); the grep test catches type-system bypass attempts.

The "finalize → publisher residual" described below applies equally to the migrated writers — Lance has no multi-dataset atomic commit primitive, so the per-table commit_staged → manifest publish gap is the same drift class. Closing it requires either upstream Lance multi-dataset commit OR the omnigraph-side recovery-on-open reconciler described in .context/mr-793-design.md §15 (deferred to MR-795).

Inline-commit residuals live on InlineCommitResidual, not db.storage() (MR-793 acceptance §1, by construction)

MR-793's acceptance criterion §1 ("TableStore (or successor) public API has no method that performs a manifest commit as a side effect of writing") holds by construction after MR-854. db.storage() (&dyn TableStorage) exposes only staged primitives + reads; the inline-commit writes Lance cannot yet stage live on a separate InlineCommitResidual trait reached via Omnigraph::storage_inline_residual(). A new engine writer cannot couple a write with a Lance HEAD advance through the default surface — it would have to name the residual accessor explicitly. The dead legacy methods (trait append_batch / merge_insert_batches, inherent merge_insert_batch{,es}, create_{btree,inverted}_index) were removed; appends/merges and scalar index builds all use the stage_* primitives.

Two methods remain on InlineCommitResidual, each named honestly at its call site:

Residual method Inline-commit reason Closes when
delete_where DeleteBuilder::execute_uncommitted is not in Lance v6.0.1 (closed upstream as #6658 but first ships in v7.0.0-beta.10); see docs/dev/lance.md MR-A: Lance v7.x bump migrates delete_where to staged, retires the parse-time D₂ mutation rule, and extends recovery sidecar coverage
create_vector_index Vector indices take Lance's "segment commit path"; build_index_metadata_from_segments is pub(crate) (Lance #6666 still open) Lance #6666 lands and stage_create_vector_index joins the staged surface

The tests/forbidden_apis.rs guard still catches direct lance::* inline-commit misuse outside the storage layer; the trait split makes the staged-only default a type-system guarantee on top of it.

LoadMode::Overwrite uses staged Lance Overwrite

The bulk loader's Append, Merge, and Overwrite modes all use the staged-write path described above. LoadMode::Overwrite accumulates replacement batches in memory, validates node/edge constraints, referential integrity, and edge cardinality before any Lance HEAD movement, stages each touched table with Lance Operation::Overwrite, then runs commit_staged under the normal SidecarKind::Load recovery sidecar before publishing __manifest. OMNIGRAPH_LOAD_CONCURRENCY applies to the fragment-writing stage only; the commit and manifest publish still run under the per-table write queues. Empty-table overwrite is represented as a valid zero-fragment Lance Overwrite transaction, not as truncate-then-append.

Open-time recovery sweep

The staged-write rewire eliminates one drift class by construction at the writer layer: an op that fails before pushing to the in-memory accumulator (validation errors, missing endpoints, parse-time D₂ rejection) leaves Lance HEAD untouched on every staged table. This is the case the partial_failure_leaves_target_queryable_and_unblocks_next_mutation test pins.

A second, narrower drift class — the finalize → publisher window — is closed across one open cycle by the open-time recovery sweep:

MutationStaging::finalize runs stage_* + commit_staged per touched table sequentially, then the publisher commits the manifest. Lance has no multi-dataset atomic commit, so the per-table commit_staged calls are independent operations: if commit_staged on table N+1 fails after commit_staged on tables 1..N succeeded, or if the publisher's CAS pre-check rejects after every commit_staged succeeded, tables 1..N are left at Lance HEAD = manifest_pinned + 1.

Recovery protocol (lifecycle of every staged-write writer — MutationStaging::finalize, schema_apply::apply_schema_with_lock, branch_merge_on_current_target, ensure_indices_for_branch, optimize_all_tables):

  1. Phase A: writer writes a sidecar JSON to __recovery/{ulid}.json BEFORE its first HEAD-advancing commit (commit_staged, or compact_files for optimize_all_tables, which advances the Lance HEAD via a reserve-fragments + rewrite commit rather than a staged write). The sidecar names every (table_key, table_path, expected_version, post_commit_pin) it intends to commit + the writer kind + actor_id.
  2. Phase B: writer's per-table commit_staged loop runs.
  3. Phase C: publisher commits the manifest.
  4. Phase D: writer deletes the sidecar.

Phase letter convention. Throughout the recovery code, log messages, failpoint names (e.g. branch_merge.post_phase_b_pre_manifest_commit), and the per-writer integration tests, "Phase A/B/C/D" refers exclusively to the four-step lifecycle above. The per-table staged-write contract (stage_* then commit_staged, two steps) is referred to by those API verbs — never by phase letters — so a reader of recovery.rs, failpoints.rs, or this document only encounters phase letters in the per-writer context.

A failure between Phase A and Phase D leaves the sidecar on disk. The next Omnigraph::open (gated on OpenMode::ReadWrite) runs the recovery sweep in crates/omnigraph/src/db/manifest/recovery.rs:

  • For each sidecar in __recovery/, compare every named table's Lance HEAD to the manifest pin. Classify per the all-or-nothing decision tree (RolledPastExpected / NoMovement / UnexpectedAtP1 / UnexpectedMultistep / InvariantViolation).
  • If any table is InvariantViolation (Lance HEAD < manifest pinned — should be impossible), abort with a loud error and leave the sidecar on disk for operator review.
  • Otherwise, if every table is RolledPastExpected, roll forward: a single ManifestBatchPublisher::publish call extends every pin atomically. SchemaApply sidecars are eligible only when schema-state recovery promoted the matching staging files in the same recovery pass; otherwise full open-time recovery rolls them back and refresh-time recovery leaves them for the next read-write open.
  • Otherwise roll back: per-table Dataset::restore to the manifest-pinned table version, then a single ManifestBatchPublisher::publish of the restored HEAD — symmetric with roll-forward, so manifest == HEAD after recovery (no residual drift). This convergence is what lets a failed-then-retried schema apply succeed instead of failing one version higher each iteration. The audit row's to_version records the logical rolled-back-to version (manifest_pinned); the manifest is published at the restore commit (manifest_pinned + 1, same content).
  • After a successful roll-forward or roll-back, an audit row is recorded — _graph_commits.lance carries a commit tagged actor_id = "omnigraph:recovery", and a sibling _graph_commit_recoveries.lance row carries recovery_kind, recovery_for_actor (the original sidecar's actor), operation_id, per-table outcomes. Operators run omnigraph commit list --filter actor=omnigraph:recovery to find recoveries.
  • Sidecar deleted as the final step.

Triggers for the residual: transient Lance write errors during finalize (object-store retry budget exhaustion, disk full); persistent publisher contention exceeding PUBLISHER_RETRY_BUDGET = 5 retries.

Long-running servers: the write entry points (load_as, mutate_as, apply_schema_as, branch_merge_as) and Omnigraph::refresh run roll-forward-only recovery in-process (recovery::heal_pending_sidecars_roll_forward) — the common Phase B → Phase C residual closes on the next write, without a restart and without an explicit refresh. The heal lists __recovery/ (one list_dir; empty in the steady state) and, per sidecar, acquires the same per-(table_key, table_branch) write queues every sidecar writer holds from before write_sidecar until after delete_sidecar — so it serializes against a live writer instead of rolling its in-flight sidecar forward from under it (a sidecar whose queues can be acquired belongs to a writer that finished or died; an existence re-check after the wait skips the finished case). Lock order is queues → coordinator, matching every writer's commit→publish path. Pinned by the four tests/failpoints.rs::*_after_finalize_publisher_failure_heals_without_reopen tests (load, mutation, schema apply, branch merge). The maintenance entries need the heal for more than liveness: without it, a schema apply re-plans rewrites from the manifest pin and orphans the drifted Phase-B commit (dropping its rows), and a branch merge publishes the drift as an unattributed side effect — both while the stale sidecar lingers to misclassify later. Sidecars that would require a Dataset::restore (mixed / unexpected state) are deferred to the next OpenMode::ReadWrite open: restore is unsafe under concurrency because Lance's check_restore_txn accepts the restore against in-flight Append/Update/Delete commits and silently orphans them (pinned by tests/staged_writes.rs::lance_restore_loses_to_concurrent_append_via_orphaning). When such a deferred sidecar blocks a write, the commit-time drift guard says so explicitly ("a pending recovery sidecar requires rollback — reopen the graph read-write") instead of pointing at omnigraph repair, which refuses while a sidecar is pending. Continuous in-process recovery for the rollback path is the goal of a future background reconciler. ensure_indices does not heal at entry itself — it runs inside the load / schema-apply flows after their entry heal, and its strict preconditions still fail loudly on drift when invoked directly.

The publisher-CAS contract is unchanged: a concurrent writer that advances any of our touched tables between snapshot capture and publisher commit produces exactly one winner. The residual above is about our abandoned commits in the failure path, not about concurrency races.

Sidecar I/O failure semantics (all sidecar I/O goes through the backend-generic StorageAdapter; the contracts below are pinned by the storage-fault failpoints recovery.sidecar_{write,delete,list} / recovery.record_audit and their tests in tests/failpoints.rs and tests/recovery.rs):

  • Phase A put fails (S3 PutObject / fs write): the writer aborts before its first HEAD-advancing commit — no sidecar, no drift, nothing to recover; a transient fault never wedges later writes.
  • Phase D delete fails (S3 DeleteObject): swallowed with a warning — the write already published, so failing the caller would report an error for a durable write. The stale sidecar is consumed by the next write's entry heal (or the next open) via the stale-sidecar audit-recovery path, recorded as RolledForward.
  • __recovery/ list fails (S3 ListObjectsV2): loud at every consumer — the write-entry heal fails the write, the open-time sweep fails the open. Silently skipping recovery would be consumer tolerance of drift.
  • Corrupt / unparseable sidecar: refused loudly by heal and open alike; the file stays on disk for operator inspection (read-only opens still work — the sweep is skipped there).
  • Audit append fails after a roll-forward publish: that recovery attempt errors and keeps the sidecar; re-entry sees the already-published manifest, records exactly one RolledForward audit row, and deletes the sidecar (the retry tolerance documented on record_audit).

Backend notes (the adapter is one implementation over object_store for every backend): local writes stage through name#<digits> temp files that the backend filters from listings and refuses to address — crash residue of that shape is invisible to the sweep, harmless, and reclaimed by delete_prefix/manual cleanup. Storage errors are backend-wrapped text without a typed NotFound discriminant — callers that need missing-vs-error (the cluster store) probe exists() first. exists() itself is object-store semantics everywhere: only objects (or non-empty prefixes) exist, and a permission failure is a loud error, not a silent false.

Conflict shape

Concurrent writers to the same (table, branch) produce exactly one success and one failure. The losing writer's error is OmniError::Manifest with kind Conflict and details ManifestConflictDetails::ExpectedVersionMismatch { table_key, expected, actual }. The HTTP server maps this to 409 Conflict with body {"error": "...", "code": "conflict", "manifest_conflict": { "table_key": "...", "expected": N, "actual": M }} — see docs/user/server.md.

Audit

actor_id lands in _graph_commits.lance via record_graph_commit (no intermediate run record). Audit history is queried via omnigraph commit list.

Migration code

db/manifest/migrations.rs carries the v2→v3 internal-schema step (MR-770): a one-time sweep that deletes legacy __run__* staging branches off __manifest. It runs in Omnigraph::open(ReadWrite) (via manifest::migrate_on_open, before the coordinator reads branch state) and again on the publisher's write path; both are idempotent once the stamp is at v3. Deleting the inert _graph_runs.lance / _graph_run_actors.lance dataset bytes is still deferred — it needs a StorageAdapter::delete_prefix primitive — but those bytes are invisible to graph-level state.

Mid-query partial failure: closed by MR-794

The pre-MR-794 design had a known limitation: a multi-statement .gq mutation where op-N inline-committed a Lance fragment and op-N+1 then failed left the touched table at Lance HEAD = manifest_version + 1, blocking the next mutation with ExpectedVersionMismatch.

MR-794 (step 1 + step 2+) closed this for inserts/updates by construction at the writer layer: insert and update batches accumulate in memory; no Lance HEAD advance happens during op execution; one stage_* + commit_staged per touched table runs at end-of-query, and only after every op succeeded. A failed op leaves Lance HEAD untouched on the staged tables, so the next mutation proceeds normally with no drift to reconcile.

The cancellation case (future drop mid-mutation) inherits the same guarantee — the in-memory accumulator evaporates with the dropped task and no Lance write was ever issued.

For delete-touching mutations the legacy inline-commit shape is preserved (Lance has no public two-phase delete in 6.0.1) — the same narrow window remains. The parse-time D₂ rule prevents inserts/updates from coexisting with deletes in one query, so a pure-delete failure cannot drift any staged-table state. If a delete-only multi-table mutation fails mid-cascade, the same workaround as before applies (retry; rely on omnigraph cleanup once a later successful commit moves HEAD past the orphan version). Closing this requires Lance to expose DeleteJob::execute_uncommitted; tracked in MR-793 and a Lance-upstream ticket.