Repository navigation
feat(planner): use indexes at plan time, index facts become a planner input - #924
Merged
azimafroozeh merged 3 commits intoOct 9, 2026
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This was referenced Oct 9, 2026
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
This PR carries the maintainer-track RFC "Index facts as planner input" (
docs/rfcs/2026-10-08-index-facts-as-planner-input.md) and all four of its rollout steps: the v2 planner reads which scalar indexes exist on every table a query reads, at the dataset version the plan is pinned to, and makes every access-path decision at plan time; execution consumes the decision and never re-checks it. The motivating defect is #854: a single String@keyequality scanned the key column while the__idBTREE went unused, and no.gqtcase could pin index use because the plan never stated it.PlanSourceexposes one index fact per index name and keyed column at the pinned version; coverage unions same-name segments and intersects current fragments, an unreadable bitmap stays unknown, and a metadata or table-open failure propagates from planning with its own error class, never as an empty catalog.Indexed, socsragainstindexed_scanis priced on real coverage; saved Expand inputs carry coverage provenance, and older saved plans without it keep the runtime coverage probe.access:sequential, orindex_probewith the index-query tree and residual that Lance's own scanner put in the read, built at plan time at the pinned version; execution setsuse_scalar_indexfrom the plan. A scan that gains inputs at execution (a ranked read, a join-generated filter, a dynamic expression, a dependent scan) recordsruntimeorid_lookupand leaves index use to Lance as today.key_to_idpass): a cast-free equality on a single String@keygains an__idequality (for that key domain the key string is the id) and keeps its written predicate; numeric, temporal, float and composite keys are never narrowed.index_facts(<table>)statistics row per read table,accesson every fresh physicalScan, andexpect planclaims of the formscan Person as $p: access index_probe __id.__idlookup, where base scanned the key column.Backing issue / RFC
cases/v2/planner/issue_854_key_index_probe.gqtpins a String@keylookup probing the__idBTREE with its written equality kept, which the base tree cannot claim (noaccessgrammar) and did not do (the key column was scanned).docs/rfcs/2026-10-08-index-facts-as-planner-input.md(maintainer track, accepted, implementation complete) lands here with its implementation; it composes with RFC 0047's FTS presence fact and leaves RFC 0046's status surface unread.Checklist
.gqtcases undercases/v2/planner/(the corpus serial run is 289 cases green) pin theaccessclaims, both Expand coverages, key narrowing on literal, parameter, escaped and empty keys and its refusal on casts and composite keys; Rust tests only where the format cannot reach: index-fact gathering on real snapshots, the cold-planning request and byte ceilings, the 200,000-row I/O comparison, saved-plan provenance round-trips, and a serial failpoint test that a source failure during planning keeps its error classdocs/user/queries/explain.md(theaccessfields, theindex_factsstatistic, the key-narrowing note),docs/dev/architecture.md,docs/dev/execution.mdchangelog.d/planner-index-facts.performance.mdLocal verification
cargo test --workspace --exclude omnigraph-gqt --exclude omnigraph-dst --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints --lib engine::plan_source::: 14 passed: cold-planning ceilings (modern 2 manifest requests / 839 bytes; overlay adds one deletion and one row-id request; legacy adds 3 zero-byte index-discovery probes), the 200,000-row key lookup at 12 execution requests / 70,664 bytes for both the key query and the__idquery, and a saved plan forced tosequentialreading more than twice the bytescargo test -p omnigraph-engine --locked --features failpoints --test failpoints planning_source_failures_keep_their_error_class: 1 passed: both planning seams return the injected error asOmniError::Manifestwith kindBadRequest, and the next read runscargo test -p omnigraph-gqt -p omnigraph-gqt-core --locked -- --test-threads=1: 289 corpus cases green in 431 s, no deadline hitcargo test -p omnigraph-gqt --locked(default parallelism): red locally, 17 cases hit their 10 s deadline under load on this machine, none touching a path this branch changes; the same cases pass serially abovecargo test --workspace --exclude omnigraph-gqt --exclude omnigraph-dst --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints --no-fail-fast: in the sandbox: 120 suites ok, red only on 3 CLI session-lock tests and 1 blob Unix-socket test (sandbox) and 6 bench watchdog tests that pass rerun alone; the same command passed 4,068 tests with 0 failures outside the sandbox before the review-round fixesNotes for reviewers
use_scalar_indexfrom its recordedaccess; a saved plan withoutaccesskeeps Lance's default, and saved Expand inputs without coverage provenance keep the runtime coverage probe, so an existing store and its stored plans read as before.MaterializeIndexExec, whose query the public API does not expose; the split then reconstructs it throughPlanner::create_filter_planover the dataset'sscalar_index_info. When that reconstruction yields no query, or the scan holds two probes, planning fails with amanifest_internalerror for a read that would have executed. This follows the RFC's failure posture: failures while opening a pinned table or loading its index metadata propagate from planning and are never treated as an empty catalog. The reconstruction is qualified by pinned source inspection and a public reconstruction guard on both file formats; no legacy-vintage graph fixture with pre-existing indexes exists yet, so a legacy dataset that defeats the reconstruction is a documented limit of this PR, not a tested path.@keyequality into an__idequality, which returns the key string unchanged for a single non-list String key. Its soundness rests on the identity guarantee of single String keys: every row of such a type, historical or current, carries the key string as its__id. The persisted-historical-keys test shows Date, F32 and composite keys are left alone; it has no String row whose stored id differs from its key, because the loader never wrote one. A graph whose single-String-key rows were written with a non-identity id by an older writer would be narrowed to the wrong id; no such writer exists in the repository's history, and that is the claim this PR relies on.