Repository navigation
perf(engine): remove the per-query metadata re-derivation tax on warm reads - #268
Merged
Merged
Conversation
…t tests Prerequisite seam for the query-latency fixes. Adds crates/omnigraph/src/instrumentation.rs: - CountingStorageAdapter: a StorageAdapter decorator counting per-method reads (read_text/exists/read_text_versioned/list_dir), for the schema-contract reads on the query path. - A per-query task-local (QueryIoProbes) carrying Lance WrappingObjectStore wrappers per open category plus a probe counter, delivered via with_query_io_probes. open_dataset_tracked attaches the wrapper so the open itself is counted (ObjectStoreParams.object_store_wrapper). Wires the wrappers into the manifest open (open_manifest_dataset) and the commit-graph opens (CommitGraph::open/open_at_branch). Production leaves the task-local unset, so nothing attaches. Makes Omnigraph::open_with_storage public so tests can inject the counting adapter. lance-io is a dev-dependency (IOTracker named only in tests). No runtime behavior change.
Cost-budget test using Lance IOTracker at the object-store boundary (the LanceDB IO-counted-test pattern). On a 20-commit-deep graph, a warm same-branch query re-opens a fresh coordinator, which opens both the commit graph and __manifest. Asserts the read opens the commit graph zero times and performs exactly one cheap version probe; today it does neither (it scans the commit graph on re-open and never probes). The freshness guard already passes. Adds the commit_many helper for history-depth fixtures. Red half of the Fix 1 red->green pair; turns green with the next commit.
query()/resolved_target re-opened a fresh GraphCoordinator from storage on every read (full __manifest scan + two commit-graph scans), so a warm read's cost grew with commit history (invariant 15) though the data was unchanged. resolved_target now serves same-branch reads from the warm in-memory coordinator, gated by a cheap version probe (latest_version_id, one object-store op) instead of a full re-open: - fresh (probe == cached version): return the in-memory snapshot under the read lock, with a synthetic (branch, version) id and no commit-graph access (reads pin the snapshot by manifest version, not the commit DAG; invariant 2). - stale: take the write lock, re-probe (double-checked; tokio RwLock has no read->write upgrade), then refresh_manifest_only (no commit-graph scan), preserving strong consistency for external writers (invariant 6). Cross-branch and snapshot targets keep the existing cold-resolve path. Adds ManifestCoordinator/GraphCoordinator::probe_latest_version and GraphCoordinator::refresh_manifest_only. Nothing on the read path needs a real commit ULID (only RuntimeCache keys on the id, where synthetic is consistent), per a caller audit. A warm same-branch read on a 20-commit graph now does zero commit-graph opens and exactly one probe (down from a deep commit-graph scan) and still observes external commits. The residual per-table __manifest scans are removed later by Fix 2.
ensure_schema_state_valid runs twice per query (query()/run_query_at AND resolved_target/snapshot_at_version), each reading 3 contract files + 2 existence probes. A warm query thus does 6 read_text + 4 exists where one validation (3 + 2) suffices, measured via CountingStorageAdapter. Adds a drift guard (schema_source_drift_is_caught_on_read) that already passes. Red half of the finding-A red->green pair.
ensure_schema_state_valid ran on every query AND again inside resolved_target / snapshot_at_version, so each query validated the schema contract twice (~10 storage ops). Removes the redundant query()/ run_query_at() calls; the validation inside resolved_target / snapshot_at_version still runs, so drift is detected exactly as before. A source-only fast path was rejected: a long-lived handle must detect external drift of the schema source, IR, OR state on its next operation (lifecycle::long_lived_handle_rejects_schema_*), which a source-only compare would miss. So the only safe latency win is not validating twice. A warm query now does one validation (3 read_text + 2 exists) instead of two (6 + 4).
…(red) After Fix 1 a warm same-branch read still scans __manifest ~44 times at 20-commit depth: not from resolution (Fix 1 removed that) but from the per-table open path, which routes through the Lance namespace and full-scans __manifest twice per touched table (describe_table + describe_table_version). Tightens the warm test to assert manifest read_iops == 0 and adds a multi-table (traversal) test asserting the same, pinning the "2 tables = 2x" tax. Red half of the Fix 2 red->green pair.
…mespace (Fix 2)
SubTableEntry::open routed every read-path table open through
DatasetBuilder::from_namespace(BranchManifestNamespace), whose describe_table
full-scans __manifest and, with managed_versioning, makes Lance scan again
(describe_table_version) -- two full __manifest scans per touched table. That
was the residual that made warm-read manifest IO grow with history and the
'2 tables = 2x' multi-table tax.
The resolved Snapshot already holds each table's path/version/branch, so open
directly: from_uri(table_uri_for_path(root, path, branch)).with_version(v).
The branch-qualified location is the dataset that physically holds the version
(main: {path}; branch: {path}/tree/{branch}, Lance native-branch storage), and
with_version resolves it within THAT dataset's _versions. 0 namespace calls +
1 HEAD via the native ConditionalPutCommitHandler.
The read namespace (BranchManifestNamespace) is now unused in production
(writes use StagedTableNamespace), so it, its constructor, and the helpers only
it used (to_namespace_version, publish_requests, their imports) are gated
#[cfg(test)] -- retained to validate the namespace contract in unit tests.
Removes the dead open_table_at_version_from_manifest.
Warm same-branch + multi-table reads now scan __manifest zero times; branch +
time-travel reads stay correct (branching.rs, point_in_time.rs, 2 lib
regression tests); production-lib warnings unchanged (baseline).
…reads (matrix)
Extends the read-path cost-budget tests across more of the morphological matrix:
- warm_branch_read_does_no_manifest_scans: a warm read on a non-main branch
(handle synced to it) scans __manifest zero times, exercising Fix 2's
branch-owned-table open (tree/{branch} + with_version) on Fix 1's warm path --
the cell that regressed when the open used with_branch against the base.
- stale_read_refreshes_manifest_only: an external commit makes the next read
take the stale path, which re-reads the manifest (read_iops > 0) but never
scans the commit graph (refresh_manifest_only), pinning Fix 1's manifest-only
refresh.
Cold paths (cross-branch, time-travel) stay behavior-covered (branching.rs,
point_in_time.rs) and are cold by design (Fix 1 warm-paths only same-branch), so
there is no manifest==0 contract to assert there.
…he commit DAG (red)
…branch DAG fork A same-branch write that follows an external commit committed a fresh manifest version (commit_all rebases the pin from a fresh coordinator) but appended off the coordinator's stale in-memory commit-graph head, forking the commit DAG (the new commit and the external commit shared a parent). Pre-existing for non-strict inserts; widened to strict ops by Fix 1's refresh_manifest_only freshening the read-time pin. record_graph_commit now refreshes the commit-graph head from storage before append_commit, so the parent is the true current head. record_merge_commit is unaffected (it passes explicit parents).
… (Fix 3) A warm same-branch read still re-opened every touched table per query (the "never warms up" residual after Fix 1+2). A per-graph held-handle cache keyed by (table_path, branch, version) now serves repeat reads with zero table opens, and one shared lance::Session per graph warms metadata/index caches across opens. Validated against LanceDB upstream (rust/lancedb/src/table/dataset.rs DatasetConsistencyWrapper): hold an Arc<Dataset> and reuse it for 0-IO warm reads; one Session per connection threaded into opens; writers never serve from the read cache; time-travel bypasses. One adaptation: omnigraph keys by version (snapshot-pins-version model) where LanceDB keys per-table+HEAD, reusing the in-repo GraphIndexCache LRU template. - ReadCaches (session + TableHandleCache) injected onto live-Branch-read snapshots in resolved_target; Snapshot::open serves from the cache or opens once with the session on a miss (via the instrumented open_table_dataset). - Writes (resolved_branch_target -> open HEAD) and time-travel / Snapshot-id reads bypass the cache. Version-in-key makes a write a new key (old handle ages out via LRU); invalidate_all at branch-switch/refresh is hygiene only. - Cost tests: a 2nd identical warm read does 0 table opens; a write re-opens only the changed table at its new version. Full engine suite green.
Extend the forbidden-API guard with Dataset::open / DatasetBuilder::from_uri / from_namespace so the read/exec layer (exec/, loader/, changes/, db/omnigraph/) cannot bypass Snapshot::open and the held-handle cache (Fix 3). The instrumented opener (instrumentation.rs) is allow-listed; two legitimate non-read opens (a test editing __manifest, Hard-drop version GC) carry sentinels. The storage/manifest layers stay allow-listed. Lean P2 scope, per LanceDB-upstream + minimize-liability: the data-read boundary already exists (SubTableEntry::open); this guard pins it so a future read cannot open around the cache. Centralizing all internal opens behind one opener is deferred.
…-budget testing Records the principle behind the query-latency work: Lance and the manifest are the source of truth, everything else a derived view held warm and refreshed by a cheap probe; the two failure modes (a drifting parallel copy, and cold re-derivation whose cost grows with history) are deny-listed. Adds the cost-budget testing discipline (assert a warm read's open/IO count is flat at commit-history depth, the LanceDB IO-counted pattern) and the warm_read_cost.rs row. Updates the read-path-re-derivation known gap to reflect what Fix 1/2/3 + finding A close, and adds the commit-graph-parent-under-concurrency gap.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be5388ea81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+999
to
+1000
| if coord.probe_latest_version().await? == coord.version() { | ||
| return Ok(warm_resolved_target(&coord, target)); |
There was a problem hiding this comment.
When this handle is already synced to a non-main branch, another process can delete and recreate that branch name (branch recreation is supported). Lance branch datasets can then have the same latest version number as the deleted incarnation, e.g. recreate from the same parent and apply the same number of writes, so this equality takes the warm path and returns coord.snapshot() from the old in-memory manifest instead of opening the recreated branch. In that scenario reads of that branch keep serving the deleted incarnation until a manual refresh or branch switch; the freshness probe needs an incarnation-sensitive token such as manifest location/eTag or commit identity, not only the version id.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Addressed in 0cc55db1 (Fix A). Warm-read freshness now compares a ManifestIncarnation (version plus the manifest e_tag, with a manifest-timestamp fallback, then version-only), not the version id alone. main cannot be deleted/recreated so it stays version-cheap (one HEAD); a non-main branch loads latest_manifest() and compares the e_tag/timestamp, so a delete+recreate that reuses a version number is detected and the read refreshes (and invalidates the read caches). Regression warm_read_on_recreated_branch_observes_new_incarnation forces the same-version collision and asserts the reader observes the new incarnation.
Comment on lines
+127
to
+130
| struct TableHandleKey { | ||
| table_path: String, | ||
| table_branch: Option<String>, | ||
| version: u64, |
There was a problem hiding this comment.
Even if snapshot resolution opens a recreated branch, this cache key can still hit a Dataset from the deleted branch because per-table branch datasets reuse the same .../tree/<branch> URI and can reuse version numbers after delete/recreate. A long-lived reader that cached feature before another process deletes and recreates it can therefore scan the old table for a fresh manifest entry with the same (table_path, table_branch, version); include a branch incarnation/snapshot token in the key or clear these handles on cold branch resolution.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Addressed in 0cc55db1 (Fix A). TableHandleKey now includes the per-table manifest e_tag (from SubTableEntry.version_metadata, zero extra IO), so a recreated branch table can no longer hit a stale (table_path, branch, version) handle. As the fallback for e_tag-less table locations, a detected same-branch incarnation refresh also clears the handle cache. Regression recreated_branch_owned_table_handle_uses_table_etag forces the table-version collision.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1008
to
+1011
| if coord.probe_latest_version().await? != coord.version() { | ||
| coord.refresh_manifest_only().await?; | ||
| } | ||
| return Ok(warm_resolved_target(&coord, target)); |
There was a problem hiding this comment.
The stale path calls probe_latest_version under the read lock (detects stale), drops the lock, then calls it again under the write lock for the double-checked re-check — so a stale read increments probe_count by 2. The warm-path test correctly asserts probe_count == 1. The stale-path test (stale_read_refreshes_manifest_only) does not check probe_count at all, so if the stale path were ever changed to probe more times, the cost budget would silently regress.
Contributor
Author
There was a problem hiding this comment.
Addressed in 0cc55db1 (Fix C). The stale-path probe count is now pinned: warm_read_on_recreated_branch_observes_new_incarnation asserts probe_count == 2 (read-lock detect plus write-lock re-check), so a regression to more probes fails the cost test.
Comment on lines
+200
to
+210
| fn insert(&mut self, key: TableHandleKey, value: Dataset) { | ||
| self.entries.insert(key.clone(), value); | ||
| self.touch(key); | ||
| while self.entries.len() > TABLE_HANDLE_CACHE_CAP { | ||
| let Some(oldest) = self.lru.pop_front() else { | ||
| break; | ||
| }; | ||
| if self.entries.remove(&oldest).is_some() { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
while self.entries.len() > TABLE_HANDLE_CACHE_CAP pops a key from lru, then calls self.entries.remove(&oldest) and breaks if the remove succeeded. If the invariant ever drifts (a key in lru absent from entries), the loop continues popping until it finds a key that is in entries — potentially evicting more entries than intended. Using an unconditional break after pop_front, or adding a debug_assert!(self.entries.remove(&oldest).is_some()), would harden the loop against future drift.
Contributor
Author
There was a problem hiding this comment.
Addressed in 0cc55db1 (Fix D). Both GraphIndexCache and TableHandleCache now share one LruMap<K, V> whose eviction is unconditional (the is_some()-break is gone), covered by the lru_map_evicts_oldest_and_touch_refreshes_order unit test. The invariant lives in one place.
Resolve AGENTS.md always-on rules: keep both rule 7 (logical contract over physical state, from main) and add rule 8 (one source of truth, cheaply derived, this branch); invariants.md already carries both. Sentinel three new test Dataset::open calls (optimize.rs, table_ops.rs fork-classification tests) for the read/exec open-guard this PR introduces. Engine builds and the full engine suite passes post-merge.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 53dae77. Configure here.
…red LruMap (PR #268 review) Phase 6 A-D, correct-by-design responses to the Codex/Greptile P2 review comments. A: warm-read freshness and the table-handle cache key use the manifest incarnation (e_tag, manifest-timestamp fallback, then version), so a deleted+recreated non-main branch reusing a version number cannot be served stale; main stays version-cheap, non-main loads latest_manifest; a detected stale refresh also invalidates read caches; two regression tests force the version collision. B: unify the two cache invalidations into Omnigraph::invalidate_read_caches() at the four sites. C: assert the stale path's probe count. D: shared LruMap behind both caches with unconditional eviction, plus a unit test. Full engine suite green; multi-process lineage fork and O(history) write refresh remain known gaps for Phase 6E/7.
ragnorc
added a commit
that referenced
this pull request
Jun 17, 2026
…nced edges Two post-#268 read-path fixes for the graph topology index (CSR + TypeIndex), each verified red->green. A1, cross-branch reuse: key the graph-index cache by each edge table's physical identity (table_key, version, table_branch, e_tag) rather than the branch-specific synthetic snapshot id. A lazy-fork branch whose edge tables are physically main's now reuses main's built index rather than rebuilding it from a cold scan (the ~40s fresh-branch first-read tax). The topology is a pure function of the edge tables, so same key means same topology; an invariant comment at GraphIndex::build pins it. A2, referenced-edges build: QueryIR::referenced_edge_types() walks the pipeline (Expand + AntiJoin, recursive); execution builds only the edges a query traverses rather than every catalog edge, shrinking the cold-engine first build (the prior 30s-guard / 428s first traversal). Building the referenced set together preserves the shared TypeIndex, so cross-edge multi-hop stays correct; an under-collected edge fails loudly rather than returning a wrong result. Tests (warm_read_cost.rs, serial + OMNIGRAPH_TRAVERSAL_MODE=csr): a fresh lazy-fork branch reuses main's index with 0 rebuilds; a one-edge query builds a single edge rather than all catalog edges. Plus a compiler unit test for referenced_edge_types (Expand + nested AntiJoin, dedup, sorted). Residual (documented in invariants.md): on stores without per-table e_tags (local FS) a recreated forked-branch table at the same version can reuse stale topology; production object stores carry real e_tags, so this is local-dev-only.
5 tasks done
ragnorc
added a commit
that referenced
this pull request
Jun 18, 2026
Brings in #268 (perf: warm-read metadata cache), #278 (compiler NanoError rename), the CODEOWNERS removal, and the docs sweep. One conflict, in schema_apply.rs: #268 made a rustfmt one-line collapse of the serial-key push inside the queue-acquire block that bug-4 RELOCATED to before the snapshot. Kept bug-4's relocation (the block now lives earlier; the pointer comment stays here); #268's reformat of the now-moved line is moot. Verified compile-compatible: the CAS methods call the unchanged record_graph_commit + manifest.commit_changes_with_expected; #268's record_graph_commit change is internal (new e_tag-aware SnapshotId::synthetic), not a signature change. #278's NanoError rename touched only compiler-crate files and auto-merged.
Merged
aaltshuler
added a commit
that referenced
this pull request
Jun 19, 2026
Patch release over v0.7.0: three correctness fixes (#283 camelCase filters, #284 cluster-apply crash-loop, #277 branch-merge OOM on embedding tables), the #280 queries-list catalog-metadata improvement, and the #268 warm-read perf fix. No breaking changes, no on-disk format change, no migration. Version coherence: all 7 crate manifests + path-dep constraints, Cargo.lock, openapi.json, and AGENTS.md surveyed version bumped 0.7.0 -> 0.7.1. Full workspace gate green (1472 tests). Claude-Session: https://claude.ai/code/session_01FQ1Hf4eXLsJmeLUkTYBEw7 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ragnorc
added a commit
that referenced
this pull request
Jun 20, 2026
* docs(rfc): RFC-013 write-path latency design + index link * perf(engine): open write-path tables directly, bypassing the namespace builder Write opens routed through DatasetBuilder::from_namespace, whose describe_table opened the whole dataset just to return a location and then re-resolved the latest version — an O(commit-depth) double latest-resolution per table open that missed Lance's O(1) version-hint fast path. On an object store this dominated write latency (~70%, RFC-013 section 2.4). TableStore::open_dataset_head_for_write now delegates to the direct opener (open_dataset_head: Dataset::open by URI + checkout_branch, routed through the tracked opener so cost tests can count it; a no-op in production). The manifest already holds every sub-table's location, so the namespace catalog lookup was redundant; ensure_expected_version still validates head == pinned for strict ops. This completes PR #268's open-by-location migration on the write side. With both reads (PR #268) and now writes bypassing it, nothing in production routes through the per-table Lance namespace. The dead open chain (load_table_from_namespace, open_table_head_for_write) is deleted and the StagedTableNamespace contract apparatus is gated #[cfg(test)], mirroring the already-test-only read namespace; __manifest commit coordination (GraphNamespacePublisher) is a separate component and is unaffected. See docs/dev/rfc-013-write-path-latency.md sections 2.4 and 9 (step 3a). * test(engine): write-path cost-budget gate on a shared harness Adds tests/helpers/cost.rs, a store-agnostic cost harness (IoCounts/StagedCounts, measure/measure_with_staged, assert_flat, local_graph/s3_graph) that the read-side warm_read_cost.rs, write_cost.rs, and write_cost_s3.rs share, so the IOTracker / task-local plumbing lives in exactly one place instead of duplicated per test. write_cost.rs (local, every-PR) gates the internal-table scan term flat in commit-history depth (a RED #[ignore]'d LOCK, the acceptance for bringing the internal tables into compaction) plus green guards: a single insert's data writes are bounded, a per-write read-op ceiling fails the moment a round-trip is added, and a keyed insert routes through stage_merge_insert once with no stage_append or vector-index build. write_cost_s3.rs (bucket-gated, rustfs CI) gates the data-table opener term flat across depth — the object-store-RPC phenomenon local FS cannot reproduce, and the red->green proof of the opener bypass. Wired into the rustfs_integration CI job and its path filter. Guards the "hot-path cost is bounded by work, not history" invariant on writes. See docs/dev/rfc-013-write-path-latency.md section 5.1, docs/dev/testing.md. * docs(rfc): RFC-013 step 3a landed; write-skew coupling; cost-gate test map - Section 9: mark step 1 (gate + harness) and step 3a (opener bypass) landed; record the per-table namespace retirement to test-only and the corrected measurement note (the opener win is S3-only; the local data-table growth is the merge-insert/RI fragment scan, a compaction term, not the opener). - Sections 7.1/6/11/5.5/10: correct the cross-table write-skew analysis after a prototype proved the scoped expected-set fix is a no-op against the per-object_id manifest (disjoint writers never share a row, so Lance never conflicts, the publisher never retries, and the expected check is a non-atomic pre-check evaluated once against stale state). The fix needs a shared contention row (Phase-7 graph_head / a minimal head row / commit-time re-validation), so it is coupled to that row, not standalone; that contention is load-bearing for correctness, not a drawback. Split the concurrent face (read-set + head) from the sequential face (inbound-RI validation on node removal) -- two different fixes. - testing.md: add write_cost.rs / helpers/cost.rs / write_cost_s3.rs to the test map; document the local-vs-S3 backend split; extend the cost-budget checklist item to the write/open path and point at the shared harness. * test(engine): isolate the opener in the S3 cost gate; fail loud on S3 setup errors Addresses two PR review findings on the bucket-gated write_cost_s3 gate: - The data-table opener was not isolated: `data_reads` also counts the merge-insert/RI scan, which reads O(fragment-count) and so grows with history for a different reason (compaction's domain, not the opener) -- the same term that made the local data-table count grow. The flat assertion would false-RED or misattribute scan growth to the opener on rustfs. Fix: compact (db.optimize) before each measurement so the table holds ~1 fragment, bounding the scan and leaving the opener's latest-version resolution as the only history-varying term. Compaction preserves version history, so the opener still faces a deep _versions/ chain -- the thing under test. - s3_graph used `.ok()?`, so when OMNIGRAPH_S3_TEST_BUCKET was set but the store was down/misconfigured, init/seed failures collapsed to None and the gate skipped + passed vacuously. Fix: skip only when the bucket env var is absent; once it is set, init/seed failures panic (mirrors tests/s3_storage.rs). * test(engine): isolate the S3 opener with a per-prefix IO probe (correct-by-design) Replaces the fixture-bounded isolation (compact-before-measure) from the prior commit with the root fix: a path-classifying ObjectStore wrapper (PrefixCounter) that attributes each data-table read to the opener term (_versions/.manifest) vs the scan term (data/*.lance). IoCounts now exposes data_opener_reads / data_scan_reads, so write_cost_s3 asserts the opener flat *directly* -- no compaction or fixture massaging, and the assertion measures the opener, not the conflated total. Closes the "harness conflates two IO terms" class: any cost test (read or write) can now isolate the opener. PrefixCounter implements only the object_store 0.13 core ObjectStore methods; the convenience surface (get/put/head/...) routes through get_opts/put_opts via ObjectStoreExt's blanket impl, so every read/write is still counted. Validated locally (every-PR) by write_cost::data_table_reads_split_into_flat_opener_ and_growing_scan: opener stays flat (7 -> 3) while scan grows (11 -> 91) and opener + scan == data_total exactly -- proving the classifier and confirming the local data-table growth is the fragment scan, not the opener. warm_read_cost (12 tests) stays green under the shared-harness change. * refactor(tests): remove cost-harness duplication and namespace cfg(test) noise Branch self-review (no behavior change) — pay down three liabilities the write-path work left: - warm_read_cost.rs kept its own probes() (three IOTrackers + a QueryIoProbes + a probe counter) and read raw .stats().read_iops — duplicating the shared helpers::cost harness this branch introduced. Migrated all 12 tests onto measure()/IoCounts; deleted the local probes(). (This also makes IoCounts' version_probes field used rather than dead.) - insert_cost was copy-pasted verbatim into write_cost.rs and write_cost_s3.rs. Hoisted to helpers::cost::measure_insert so the measured write is defined once. - The per-table Lance namespace (namespace.rs) became entirely test-only after step 3a, but was gated with ~22 per-item #[cfg(test)] attributes. Collapsed to a single `#[cfg(test)] mod namespace;` and stripped the per-item attributes; merged the import groups the gating had split. Verified: lib in-source 162 passed; write_cost 4 + warm_read_cost 12 passed; forbidden_apis passed.
5 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Warm reads paid a flat per-query metadata re-derivation tax (coordinator re-open,
__manifest+ commit-graph re-scans, per-table re-open, double schema validation) that scaled with commit history and never warmed up. This reuses the warm coordinator behind a cheaplatest_version_idprobe (Fix 1), opens tables by location+version (Fix 2), validates the schema once per query (finding A), and holds openDatasethandles plus one shared LanceSessionper graph (Fix 3), so a warm same-branch read now does one probe, one schema read, and zero opens on a warm repeat. It also closes a commit-DAG fork on the write path (a same-branch write after an external commit could append off a stale cached head;record_graph_commitnow refreshes the head before append) and adds a forbidden-API guard so the read/exec layer cannot bypass the held-handle cache.Backing issue / RFC
Maintainer-internal query-latency work (RCA + plan in
.context/query-latency-rca-handoff.md); no public issue. Per the template, the link requirement is for external contributions.Checklist
warm_read_cost.rsIO cost-budget tests,branching.rsfork red→green,forbidden_apis.rsopen guard)Notes for reviewers
rust/lancedb/src/table/dataset.rsDatasetConsistencyWrapper): hold-and-reuse for 0-IO warm reads, oneSessionper connection, writers never serve from the read cache, time-travel bypass. omnigraph keys the cache by(table, branch, version)to fit its snapshot-pins-version model.git log(P1 fork:a6f154dtest,96acbc7fix; the cost tests assert a warm repeat does 0 table opens).omnigraph-clisystem_local.rse2e tests (lines 1821, 1993) fail pre-existing and unrelated (auth/server-boot at the HTTP boundary; the diff is engine-only). Full engine suite green.Sessioninto manifest/commit-graph opens; internal-table compaction; the read/write coordinator split.Note
Medium Risk
Touches core read resolution, caching, and commit-graph parent selection; strong test coverage (IO budgets, DAG linearity, branch recreation) limits stale-read risk, but cache invalidation must stay aligned on refresh paths.
Overview
Warm read path: Same-branch reads no longer re-open coordinators or scan
__manifest/ the commit graph on every query.resolve_target_innerprobes manifest incarnation (version plus e_tag/timestamp on non-main branches); on a match it returns a synthetic snapshot from the in-memory coordinator, and on stale it manifest-only refreshes under a double-checked write lock. Live branch snapshots get per-graphReadCaches: one shared LanceSessionand an LRUTableHandleCachekeyed by(table, branch, version, e_tag)so repeat reads clone heldDatasethandles with zero table opens. Sub-tables open by location + version (namespace read path removed from production), andquery/run_query_atdrop redundantensure_schema_state_validbecauseresolved_targetalready validates once.Write correctness:
record_graph_commitcallscommit_graph.refresh()beforeappend_commitso same-branch writes after an external commit parent off the true head instead of forking the commit DAG.Guardrails & docs: New
instrumentationmodule (task-local IO probes, tracked opens, counting storage adapter) powerswarm_read_costandbranchingregression tests;forbidden_apisblocks directDataset::openin read/exec layers. Invariant 15 and cost-budget testing guidance document “one source of truth, cheaply derived.”Reviewed by Cursor Bugbot for commit 0cc55db. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
This PR removes the per-query warm-read tax on same-branch reads: instead of re-opening a fresh coordinator and scanning
__manifest+ the commit graph on every query, a cheaplatest_version_idprobe gates coordinator reuse, tables open by location+version (no namespace scan), schema is validated once, and a per-graphTableHandleCache(LRU-64Datasethandles keyed by(table, branch, version, etag)) plus one shared LanceSessionserve table opens with zero IO on a warm repeat. It also closes a single-process commit-DAG fork — a long-lived writer that commits after an external write was appending off a stale cached head — by refreshing the commit-graph head beforeappend_commit.resolve_target_innerprobeslatest_version_idunder the coordinator read lock; on a stale detection it upgrades to a write lock with a double-check and callsrefresh_manifest_only(manifest only, not the commit graph). Non-main branches additionally compare manifest etag/timestamp to guard against delete-and-recreate at the same version number.SubTableEntry::opennow usesDatasetBuilder::from_uri(location).with_version(v)directly; theTableHandleCachesits in front and returns a cheapDatasetclone on a hit. A newinstrumentationmodule provides task-local IO-probe wrappers so cost-budget tests can assert 0 manifest opens, 0 commit-graph opens, and 0 table opens on a warm repeat without mocking.record_graph_commitrefreshes the commit graph from storage before selecting the append parent, so a same-branch write after an external commit parents off the true head rather than a stale cached one.Confidence Score: 5/5
Safe to merge. The warm read path, stale double-check, commit-DAG fork fix, and handle cache are all well-reasoned and backed by IO-budget tests plus red-green regression tests. No user-facing API or schema changes.
The double-checked locking in resolve_target_inner is correct. ManifestIncarnation::matches handles main vs. non-main branches asymmetrically but deliberately and correctly. The LruMap refactor fixes the old break-on-success eviction edge case. The commit_graph.refresh() insertion closes the documented single-process DAG fork without touching data visibility.
db/manifest.rs (probe_latest_incarnation asymmetry between main and non-main branches is intentional but subtle) and db/omnigraph.rs (stale path holds write lock during probe IO for non-main branches)
Important Files Changed
Reviews (4): Last reviewed commit: "fix(engine): branch-incarnation identity..." | Re-trigger Greptile