fix(query): push the limit clause down into bm25 full-text scans - #574
Merged
azimafroozeh merged 4 commits intoSep 1, 2026
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
azimafroozeh
force-pushed
the
ranked-read-join-offset-overflow
branch
from
August 29, 2026 22:39
c7eff8f to
05e5983
Compare
aaltshuler
reviewed
Aug 30, 2026
aaltshuler
left a comment
Collaborator
There was a problem hiding this comment.
Verified one ranking regression against the exact PR head and its parent; details inline.
5 tasks
5 tasks
…fset-overflow # Conflicts: # crates/omnigraph/src/exec/query.rs # docs/releases/v0.10.0.md
This was referenced Sep 1, 2026
ragnorc
added a commit
that referenced
this pull request
Sep 1, 2026
The equivalence baseline before the executor stops inferring retrieval from order_by[0]. Most goldens already existed (bm25/nearest full rank orders, bm25 secondary keys, rrf fused lists, the #574 cap/retry pins); the two gaps were: - nearest_tie_broken_by_secondary_order_key_golden: a genuine distance tie resolved by a trailing user key — the #544 skip(1) tail path in its nearest form. - search_ordered_limit_pushdown_stays_disqualified: instrument-level pin (expand_cap_stops == 0) that limit pushdown into a final Expand stays off for search-ordered traversals, so a refactor cannot re-enable the cap while a small golden happens to survive. Part of the search-contracts RFC P1 groundwork.
ragnorc
added a commit
that referenced
this pull request
Sep 1, 2026
The executor discovered WHAT retrieval to run by re-inspecting the first order expression at execution (extract_search_mode/extract_sub_search_ mode/bm25_scan_limit) — query semantics living outside the typed plan, the root under two recorded bugs. Retrieval is now a first-class lowered plan field: - QueryIR gains retrieval: Option<RetrievalIR> (Nearest / Bm25 / FuseRrf); lowering decides the shape once — per-arm candidate counts, the #574 bounded-scan policy (limit x BM25_SCAN_OVERFETCH_FACTOR, disqualified by aggregates and secondary order keys) — while parameter values and String-query embedding stay execution-time, so one lowered plan serves every parameterization. - The engine's three inference fns are deleted; resolve_retrieval maps the lowered plan onto the existing SearchMode. SearchMode, the uncapped retry, search_score_orderings, execute_node_scan, and execute_rrf_fusion are untouched — the diff is confined to where the mode comes from, which is what makes equivalence reviewable. - order_by itself is unchanged (direction validation and secondary keys still read it); the trailing-rank-function rejection stays engine-side byte-identical. Equivalence evidence: the characterization goldens and the full search, ordering, aggregation, and proptest_equivalence suites pass unchanged; six new lowering unit tests pin the retrieval shapes and cap policy. Part of the search-contracts RFC P1.
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
Closes #563. A
bm25()orrrf()ordered read that also traverses an edge failed with an ArrowOffset overflowerror at corpus scale (reported at ~700k+ entities), because the two ranked orderings were asymmetric:nearest()threads the query limit into the scan as top-k, while the bm25 leg never set the full-text query's limit, so Lance returned every matching entity ranked and the pipeline materialized the whole matched set, applyinglimitonly at the very end. With a traversal, the joined text column was re-copied at edge fanout and crossed the 2 GiB i32 offset ceiling of a single Arrow string column; without one, a limit-20 read still hydrated the full matched corpus (the reporter measured 71 s at 1.05M entities).Measured with the timing instrument this PR adds to the repro target (6,000 matched entities x 200 KB text, warm, identical debug builds of the parent commit and this branch): the join-free limit-20 read drops from 119 ms to 2 ms median. The cost is hydration count (6,000 entities before, 80 after), so the gap widens with matched-corpus size.
The fix makes the bm25 leg carry the limit the way
nearestalways has, plus a safety net so the bound can never change an answer:limit: BM25 returns entities score-descending, so the capped scan yields the uncapped scan's leading entities (up to score ties), and the slack absorbs entities later dropped by traversals or non-pushed filters.limitresults, the query retries once uncapped: the cap is an optimization, never a result budget. Retries emit a debug trace event.count/sumanswers, not just cost (pinned by a test that failed red before this exemption, and by probe assertions proving the aggregate's single scan is uncapped).rrf()arms are now bounded. Previously the vector arm was capped at the limit while the FTS arm was exhaustive, so fusion was already one-sided top-N; this makes the arms consistent, at the cost that entities ranked past an arm's cap lose that arm's fusion contribution (tail ordering on large matched sets can differ from before; documented in the search docs).Backing issue / RFC
Checklist
#[ignore]d overflow-scale repro plus a timing instrument in the same target)docs/user/search/index.mdgains the bounded-scan paragraph incl. the rrf fusion-window note)Local verification
cargo test -p omnigraph-engine --test search— 34 passedcargo test -p omnigraph-engine --test repro_issue_563 -- --ignored --nocapture— both pass: the overflow repro returns its 20 rows in ~21 s (pre-fix:Offset overflow error: 2147489268), the timing instrument prints 2 ms median (119 ms on the parent commit, same instrument via a git-archive build)cargo test --workspace --no-fail-fast— green except three failures reproduced identically on pristine main (sandboxed special-file blob test, two merge stack overflows)cargo clippy --workspace --all-targets— cleancargo fmt --all --check— cleanNotes for reviewers
limitpays the capped-plus-uncapped double run on every execution: the result count cannot distinguish cap starvation from a small matched set. A scan-fill signal to skip the futile retry is deliberate future work.search()filters without a bm25 ordering still materialize unbounded (the pre-existing read-path memory-bound class), and at overflow scale a genuinely under-filled query still errors in the uncapped retry, as before this change.asc/descon abm25()ordering is ignored engine-wide (the cap assumes the score-descending order Lance actually returns), and the rrfkpositivity check accepts only literal integers, so a param-supplied non-positive k reaches runtime; this PR makes that cast saturate deterministically instead of wrap.Greptile Summary
This PR bounds standalone BM25 scans according to the query limit while preserving complete results through an uncapped retry when downstream filtering or traversal under-fills the result.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD Q[BM25 ordered query] --> A{Aggregate return?} A -->|Yes| U[Run uncapped BM25 scan] A -->|No| L{Limit present?} L -->|No| U L -->|Yes| C[Run capped BM25 scan] C --> P[Traverse filter project order and limit] P --> F{Returned fewer rows than limit?} F -->|No| R[Return result] F -->|Yes| U2[Retry once with uncapped scan] U2 --> R U --> P2[Execute remaining pipeline] P2 --> RReviews (5): Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." | Re-trigger Greptile