Repository navigation
refactor(engine): make the plan the single source of truth for v2 reads - #769
Merged
Merged
Conversation
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 makes the bound plan the single source of truth for engine v2 reads: once planning and binding finish, execution reads the plan and nothing else, so one plan means one execution.
Nearest,TextSearchandRankFusenodes of feat(engine): introduce planned query execution #760 lowered to nothing; the ranking that ran was rebuilt fromQueryIR::order_byintoSearchModeafter planning, so explain printed one plan and the engine ran another, and a planner pass on a ranking node changed no result.Scan(ranked: kind, property, query expression,fetch, the declared overfetch ladder,nprobes);NearestandTextSearchare deleted;RankFusetakes two arm subtrees, one rankedScaneach; the planner emits the scoreSortand declares everySort'stiebreak, so an id column is read only when something above the scan needs it.BoundPlanis the plan plus its value table (resolved parameters includingnow(), query vectors by node id).gatheris the one read of session settings and environment, before planning; the plan recordsassumptions(parameter names, settings, env caps, gate policy, memory limit, dataset pins by path, branch and version);executetakes aBoundPlanand noQueryIRorSessionSettingsby type, andengine/reads no environment outsideplan_source.rs(a test greps).HashJoinnode with its declaredfallback,Expand.policy,RankedAccess.overfetch. The planner walks the plan post-order through aLowertrait the engine implements once per node kind; every operator carries theNodeIdthat built it, and the run reports per node (status,attempts[].{rung, ran, actual_rows, drained}) asprofilerows and asidon physical explain rows.BoundPlanruns again on a snapshot whose pinned datasets match, with equal rows and the same trace, covered for a hash join, a nearest ladder, an rrf fusion, a zero limit, a bulk anti-join, an aggregate, a boundnow()andann_nprobesacross sessions.ann_nprobesbecomes arequestsetting (set ann_nprobes = N, the HTTPsettingsfield, the CLI--set), because the plan now carries the cap asnprobes.nearest()orbm25()ordering on a traversal destination is refused at planning as a user error; on v2 it failed at run time with a missing score column.Backing issue / RFC
docs/rfcs/2026-09-16-session-settings.md) for theann_nprobesscope.Checklist
ann_nprobesscope flip is its consequence)ranked_access_on_the_scan,sort_tiebreak_declared,switch_ran_on_the_report,input_ann_nprobes,input_engine_route,input_index_build_state,search_order_on_an_unranked_binding_and_a_destination; plannerlower_walkandbound_plan; engineengine_v2_plan_replayandengine_v2_scrubbed_replay; the existing v2 corpus passes with unchanged rows)docs/user/queries/explain.md,docs/user/queries/index.md,docs/user/search/index.md,docs/releases/v0.12.0.md, the session-settings RFC; dev guidesdocs/dev/execution.md,docs/dev/testing.md)Sortwithoutfetchholds its whole input under the query pool and does not spill)Local verification
cargo test --workspace --exclude omnigraph-gqt --exclude omnigraph-dst --locked --features omnigraph-engine/failpoints,omnigraph-cluster/failpoints— 110 targets green on the tree before the last review's five corrections; the only reds were the sandbox refusals (three CLI credential tests, the blob special-file test) andforbidden_apison the two lines added at commit; after the corrections the touched suites were rerun green (planner, api-types, engine v2 replay and memory suites, CLI in-source tests, serverdata_routes), the full workspace run is owed before the commitcargo test -p omnigraph-gqt --locked— 142/142 cases plus the runner's own tests green after the correctionscargo clippy -p omnigraph-planner -p omnigraph-engine -p omnigraph-gqt -p omnigraph-api-types -p omnigraph-server -p omnigraph-cli --all-targets— 0 warnings after the correctionscargo test -p omnigraph-server --test openapi— 104 green after regeneratingopenapi.jsoncargo fmt --all --check— cleanpython3 scripts/check-docs.py— OK, 145 files;bash scripts/check-agents-md.sh— OKomnigraph-vocabulary-guard check --surface openapi --base 70abf65e— 20 current, 20 base, OK;--surface rust-string— fails on 779merge baseinventory entries at70abf65e, none from this diffNotes for reviewers
enginestaysv1; everything here runs underset engine = v2except theann_nprobesscope, which v1 also accepts in a request and reads at execution without a plan (refusing it under v1 is a follow-up); an older server drops the HTTP field silently, accepted with the version gate and stated in the RFC.explain_versionstays 1 with the row schema unchanged; physical rows gainid,Scangainsrankedandnprobes,Sortgainstiebreak,HashJoinprintsfallback,RankFusehas two inputs, one trailingassumptionsrow; theNearestandTextSearchrows are gone, so a consumer keyed on node names changes.pub#[doc(hidden)]doors onSession, both read-only:query_inspected(rows, plan, report in one run) andreplay_bound_plan; the GQ logic-test runner and the replay tests are their callers.RankFusedeclares no tie-break yet; the operator orders deterministically (score, fused id, other ids); declaring it on the node is a follow-up.AggregateExecand its consumer since the wrapper operator left; closing that is a follow-up.engine/lower.rs,search.rsandmod.rsrewritten around theLowerwalk andBoundPlan; six omnigraph operators replace the DataFusion ones a node used to lower to (FilterExec,ProjectionExec,LimitExec,CrossJoinExec,SortExec,HashJoinExec), each counting its polls and charging what it allocates; planner gainsbound.rsandmirror.rs(serde mirrors, because arrow'sserdefeature is not in the lockfile).