fix(search): knn beside query takes the indexed route again (#892)

[xerj-org]

What

knn beside a query — the ES 8.x canonical hybrid — has been answered by a
full stored-document scan of every segment since #825/#879 landed. This
restores the indexed route without touching the correctness contract #825
established.

Root cause

pin_knn_clause (engine/crates/xerj-engine/src/index.rs) pre-executes the
vector leg and splices its top-k back into the tree as

Bool{ should: [Constant{score, Ids{[id]}} × k], filter: [Ids{all k}], msm: 1 }

Ids has no arm in the FTS projection — _id is not in the term dictionary —
so query_node_to_fts_*'s should loop ?-aborted on it, fts_query came
back None, needs_fts was false, and every segment fell through to
scan_stored_section_into.

It was not only slow

The stored scan admits documents in stored-layout order, and once
all_hits reaches materialisation_limit it materialises nothing further —
it continues to keep counting, with no score comparison
(scan_stored_section_into, the if all_hits.len() >= materialisation_limit
arm). So on any index with more lexical matches than the page cap, the
documents reached by BOTH halves — precisely the ones #825 exists to score
query_score + knn_score and rank first — never enter the page if they sit
late in the segment. hits.total stays exact; the page is wrong.

Live-verified on the 100 000-document corpus below, match + knn k=10 whose
vector top-10 contained exactly two lexical matches (96820, 83150):

rc.72   hybrid page[:20] = 20 lexical-only docs, all tied at 1.693147
        rank of 96820 = NOT IN TOP 20
        rank of 83150 = NOT IN TOP 20
fixed   hybrid page[:20] = [('96820', 3.152876), ('83150', 3.143735), ('0', 2.171479), …]
        rank of 96820 = 1
        rank of 83150 = 2

Fix

Give the FTS query language a leaf whose match set and per-document scores are
supplied by the caller, and project the pinned disjunct onto it per segment.

• xerj-fts — new Query::DocScores(DocScoresQuery { docs: Vec<(u32, f32)> })
  (crates/xerj-fts/src/search.rs). It is a leaf that emits one ScoredHit
  per (doc_id, score) pair; execute_bool is a set-and-score combiner over
  Vec<ScoredHit>, so it needed no other change. The vector leg has already
  run, so these scores are known facts — there is no term dictionary, no
  postings decode and no scorer involved.
• xerj-engine — pinned_constant_ids_pairs recognises exactly the
  sub-tree pin_knn_clause emits and returns its (id, score) pairs;
  PinnedIds carries either a shape probe (no segment in hand) or one
  segment's _id → stored-position map; query_node_to_fts_projected checks
  for the pinned shape before the match and lifts it to a DocScores leaf.
  Positions come from id_pos_map_for — the existing per-segment id index
  that ids queries and _mget already resolve through, cached per segment.

Everything else stays exactly as #879 left it. The query tree is unchanged,
so hits.total, the aggregation corpus, sort, collapse, pagination,
min_score, rescore, highlight and the memtable arm all produce the same
answers; only the projection learned to lift one more clause. That is why the
diff does not touch the 4 700-line body of search_inner beyond threading one
Option<PinnedIds>.

One correction to an earlier version of this sentence, which said those paths
"keep the code paths they already had": they do not, quite. query_needs_fts
flips false → true for this shape, which re-gates three branches — the
size:0 columnar agg fast path, count_authoritative, and the F1
total_count overwrite after the segment loop. All three are inert for a
pinned hybrid (the fast path additionally needs is_match_all || agg_filter.is_some() and query_node_to_agg_filter declines a should bool;
the other two only ever remove a bounded-count shortcut, leaving the FTS path's
own authoritative seg_hits.len() tally). The outcome is unchanged; the gates
are re-evaluated. That reasoning is now recorded at the query_needs_fts
binding rather than left for the next reviewer to re-derive.

Two supporting details:

• bool_has_nonprojectable_nonscoring short-circuits on the pinned sub-tree.
  Without that, the pinned tree's own (redundant) filter: [Ids] armed the
  residual_gate, which forces fts_cap = usize::MAX plus an O(matches)
doc_matches_query sweep — the same scan, one layer down.
• build_collection_stats gets the same projection, so a pinned hybrid now
  scores against index-wide BM25 statistics (BM25 length normalisation is per-arm: overwriting a document moves it from last to first #188) instead of silently
  reverting to per-segment ones.

Scope: append-only indexes only (gated on !deletes_present)

id_pos_map_for keeps ONE position per _id, so a segment physically holding
two copies of a document (an overwrite flushed alongside its predecessor) can
hand back the superseded one — and an FTS leaf, unlike the stored scan, never
re-reads _id per position to notice. This is the same rule
build_ids_prefilter_cached already applies to the ids prefilter. With
ghosts present the request keeps rc.72's scan: correct, and no slower than it
was. Per segment, a missing/incomplete id index also just falls back.

This is a one-way door, not an in-flight window, and it bounds who sees the
numbers below. The gate is

snap.segments.iter().any(|m| m.has_tombstones)
    || self.store.version_map.ghost_events() > 0

and ghost_events is monotonic by design (version_map.rs: "never
decremented on merge: once an index has seen updates, the delete-aware slow
paths stay on"). One PUT over an existing _id, or one DELETE, anywhere in
an index's history disables this route for the life of the open index — the
merge that purges the superseded copies does not clear it, and
force_ghost_event re-arms it at open while live < physical still holds on
disk. So an index that takes updates or deletes keeps rc.72's stored scan for
knn beside query: still correct, still no slower, but with none of the
speedups in the table below. Append-only indexes — bulk load,
reindex-and-swap, log and event corpora — get them.

Narrowing the gate is deliberately not attempted here. It would have to
prove the #825 union/sum contract with tombstones present: id_pos_map_for's
map.len() == expect_docs guard already rejects a duplicate-bearing segment,
but a tombstoned-yet-present row still resolves to a position and the FTS leaf
would score a deleted document. That is a separate piece of work with its own
proof obligation.

Measured

Corpus built for this PR: 100 000 documents, text + 8-dim dense_vector,
~10 % lexical selectivity on the probe term, one flushed segment, bulk-loaded
once and never updated — which, per the scope section above, is the only
state in which this route engages. Closed-loop,
fresh query vector per request so the query cache cannot answer twice;
medians of 2 rounds × 7 requests per shape; both arms on the same box (shared
with seven sibling build agents, so absolute numbers are noisy — the ratios
are the point, and the two control rows bound the noise).

request	rc.72	this PR
query alone (control)	0.45 ms	0.52 ms
knn alone, k=10 (control)	1.99 ms	2.12 ms
bool.should[match, match] (control)	0.52 ms	0.55 ms
query + knn k=10	265.9 ms	6.4 ms (41×)
query + knn k=100	326.9 ms	8.4 ms (39×)
query + knn k=1000	505.0 ms	27.5 ms (18×)
query + knn + aggs k=10	784.1 ms	534.4 ms (1.5×)

The agg row keeps most of its cost: a terms agg over this shape still
materialises the whole corpus (need_full_corpus in search_inner). That is a
different path and is untouched here.

Correctness capture over the same corpus, rc.72 vs this PR, fixed query vector:

shape	hits.total	agg buckets	page
query+knn k=10	10008 = 10008	—	changed — see above
query+knn k=1000	10891 = 10891	—	changed, same reason
query+knn+aggs	10008 = 10008	identical	changed
query+knn+sort	10008 = 10008	—	identical
query alone	10000 = 10000	—	identical, scores identical
knn alone	10 = 10	—	identical, scores identical

_score values change for this shape

The lexical half is now scored by exact BM25 — the same number the identical
query returns without a knn beside it (2.171479 in the capture above) —
instead of the stored scan's heuristic (1.693147). Ordering follows the #825
contract as before; absolute values move, so a min_score tuned against
rc.72's hybrid needs revisiting. This is called out in the CHANGELOG.

Test, proven to fail on unfixed code

pinned_knn_hybrid_is_answered_from_the_inverted_index
(crates/xerj-engine/src/index.rs, new pinned_knn_fts_892_tests): 1 200
documents, 400 lexical matches (more than the page cap), vectors fanned so the
k=10 neighbourhood is the last ten documents and three of them are also
lexical matches. It asserts hits.total == 407 and that the page leads with
d1197, d1194, d1191 — the three both-halves documents in vector order —
plus a route assertion on the new #[cfg(test)] SEG_FTS_HANDLED /
SEG_STORED_SCANS instruments (same pattern as #577's RESIDUAL_HITS_PEAK;
which route a search took is otherwise invisible from the response).

FAIL-BEFORE was run by reverting only the fix — pinned_probe forced to
None in search_inner, everything else including the instruments left in
place — exactly as #577 does. Result recorded in the commit body.

Two further unit tests cover the projection arm in isolation
(DocScores carries the resolved positions and pinned scores; ids the segment
does not hold are absent) and the shape guard rails (a must, an msm other
than 1, a duplicate id, or a non-Constant disjunct all decline).

Gates

• cargo fmt --all --check clean.
• cargo clippy --release -j 8 -p xerj-fts -p xerj-engine -- -D warnings clean.
• cargo test --release -p xerj-engine --lib pinned_knn_fts_892 — 3 passed.
• cargo test --release -p xerj-api --test knn_beside_query_with_aggs_stays_200
  — the knn beside query still silently drops the vector contribution when aggs (or sort/collapse/rescore/...) is present #825 contract tests, unchanged, 4 passed.
• ES-YAML conformance: not run locally — its runner does DELETE /_all and
  seven agents share this box. CI runs it. The one case that covers this shape,
  yaml/vectors/96_knn_beside_query_aggs.yml, asserts hits.total and agg
  buckets only, both of which the capture above shows unchanged.
• xerj-engine's full lib suite was not run to completion (it exceeds the
  time budget on a box shared with seven build agents); the adjacent in-file
  suites were.

Closes #892.

🤖 Generated with Claude Code

[Vinz2168]

Reviewed the full diff (index.rs + search.rs + CHANGELOG). No blocking issues — this looks correct and safe to merge.

Traced the scoring contract end-to-end: execute_bool sums should-clause scores, so once the pinned kNN disjunct lifts to a DocScores leaf and lands back inside the outer bool.should, the query_score + knn_score sum from #825 is preserved through the indexed path, not just the stored scan.

Checked for double-counting: the old heuristic IDF rescore pass (the one that manually re-added pinned_knn_scores) is gated by exact_bm25_page = fts_scored_applied && !heuristic_scored_applied. When #892 routes every segment through FTS, that gate is true and the whole rescore block — including the manual add-back — is skipped, so there's no risk of adding the vector score twice. In the mixed case (memtable non-empty, or a segment whose id_pos_map_for comes back incomplete), the gate correctly falls back to the pre-#892 heuristic + add-back path, so that case degrades to exactly the previously-correct behavior rather than a new one.

!deletes_present gate: reuses the same conservative signal build_ids_prefilter_cached already gates on, for the same reason (id_pos_map_for keeps one position per id, so a segment holding a superseded duplicate could hand back the wrong one). Good that this didn't try to be cleverer than the existing precedent.

pinned_constant_ids_pairs shape match: guard rails look right — rejects must/must_not, non-1 minimum_should_match, non-Ids filters, and duplicate ids. only_the_pinned_shape_is_lifted covers these. Agree with the docstring's framing that this is a sound shape-match (not an identity check) rather than a loophole.

DocScores as a no-op in collect_fts_query_fields/collect_fts_query_terms: correct — doesn't pollute the BM25 collection-stats pre-pass, and doesn't accidentally trip the count-only single-Term fast path (that one pattern-matches FtsQuery::Term specifically).

Test coverage: the pinned_knn_hybrid_is_answered_from_the_inverted_index fail-before test is well constructed — it builds a corpus where the both-halves documents sit late enough in the segment that the old scan's page-cap-without-score-comparison behavior would silently drop them, and asserts both hit ordering and SEG_FTS_HANDLED > 0 && SEG_STORED_SCANS == 0.

One non-blocking note: the "mixed" fallback (some segments FTS, some scan, within the same request) is correct but relies on exact_bm25_page degrading cleanly rather than being exercised by a dedicated test — a test with a non-empty memtable alongside a pinned hybrid query would make that path explicit rather than implicit. Not a reason to hold this up.

CI is green across the board. LGTM.

[Vinz2168]

Reviewed the full diff (index.rs + search.rs + CHANGELOG). No blocking issues — this looks correct and safe to merge.

Traced the scoring contract end-to-end: execute_bool sums should-clause scores, so once the pinned kNN disjunct lifts to a DocScores leaf and lands back inside the outer bool.should, the query_score + knn_score sum from #825 is preserved through the indexed path, not just the stored scan.

Checked for double-counting: the old heuristic IDF rescore pass (the one that manually re-added pinned_knn_scores) is gated by exact_bm25_page = fts_scored_applied && !heuristic_scored_applied. When #892 routes every segment through FTS, that gate is true and the whole rescore block — including the manual add-back — is skipped, so there's no risk of adding the vector score twice. In the mixed case (memtable non-empty, or a segment whose id_pos_map_for comes back incomplete), the gate correctly falls back to the pre-#892 heuristic + add-back path, so that case degrades to exactly the previously-correct behavior rather than a new one.

!deletes_present gate: reuses the same conservative signal build_ids_prefilter_cached already gates on, for the same reason (id_pos_map_for keeps one position per id, so a segment holding a superseded duplicate could hand back the wrong one). Good that this didn't try to be cleverer than the existing precedent.

pinned_constant_ids_pairs shape match: guard rails look right — rejects must/must_not, non-1 minimum_should_match, non-Ids filters, and duplicate ids. only_the_pinned_shape_is_lifted covers these. Agree with the docstring's framing that this is a sound shape-match (not an identity check) rather than a loophole.

DocScores as a no-op in collect_fts_query_fields/collect_fts_query_terms: correct — doesn't pollute the BM25 collection-stats pre-pass, and doesn't accidentally trip the count-only single-Term fast path (that one pattern-matches FtsQuery::Term specifically).

Test coverage: the pinned_knn_hybrid_is_answered_from_the_inverted_index fail-before test is well constructed — it builds a corpus where the both-halves documents sit late enough in the segment that the old scan's page-cap-without-score-comparison behavior would silently drop them, and asserts both hit ordering and SEG_FTS_HANDLED > 0 && SEG_STORED_SCANS == 0.

One non-blocking note: the "mixed" fallback (some segments FTS, some scan, within the same request) is correct but relies on exact_bm25_page degrading cleanly rather than being exercised by a dedicated test — a test with a non-empty memtable alongside a pinned hybrid query would make that path explicit rather than implicit. Not a reason to hold this up.

CI is green across the board. LGTM.

[xerj-org]

Review response — merged main, and both blocking findings fixed

Pushed as dd052239. No behaviour change: the merge, plus CHANGELOG text
and code comments. Diff of the merged tree against origin/main is
byte-identical to this branch's own diff against its merge base, hunk headers
aside — verified mechanically, not by eye.

Blocking 1 — the CHANGELOG described a one-way door as an in-flight window

It said the indexed route is "skipped while the index has tombstones or
overwrites in flight". That is false. The gate is

snap.segments.iter().any(|m| m.has_tombstones)
    || self.store.version_map.ghost_events() > 0

and ghost_events is documented in version_map.rs as "Monotonic by design
(never decremented on merge): once an index has seen updates, the delete-aware
slow paths stay on." A single PUT over an existing _id, or a single
DELETE, anywhere in an index's history turns this route off for the life of
the open index. The purging merge does not clear it, and force_ghost_event
re-arms it at open while live < physical still holds on disk.

The CHANGELOG bullet now says exactly that, and the same reasoning is recorded
at the gate in index.rs so the next reader of the code is not misled either.

Blocking 2 — the scope of the numbers was nowhere stated

Every row of the table was taken on a freshly bulk-loaded, never-updated 100k
corpus — the only regime in which the route engages. Someone reading the rc.73
notes while running a mutable ES-compat index would have expected 41× and got
rc.72's stored scan.

Fixed in both artifacts: the measurement lead-in now says the corpus was
bulk-loaded once and never updated, and the scope bullet says an index taking
updates or deletes keeps rc.72's stored scan with none of the speedups.
Append-only indexes — bulk load, reindex-and-swap, log and event corpora — get
them.

On narrowing the gate instead

Considered and deliberately not done. Widening the route to ghost-bearing
indexes has to prove the #825 union/sum contract with tombstones present:
id_pos_map_for's map.len() == expect_docs guard already rejects a
duplicate-bearing segment, but a tombstoned-yet-present row still resolves to
a position, and an FTS leaf never re-reads _id per position to notice — so
the deleted document would score. That is a separate change with its own proof
obligation, and it is named as such in the CHANGELOG, the PR body and the code
rather than being smuggled into a remediation pass.

Non-blocking items taken

• NB1 — the deletes_present hoist moved a live counter's read time ~850
  lines and two .await points earlier. Documented at the binding: the two
  pre-existing F1 consumers now see a marginally staler value; the race
  pre-dates this change and the hoist widens rather than creates it; and the
  knn beside query is answered by a full stored-doc scan: the pinned Ids clauses are unprojectable to FTS (follow-up to #825) #892 gate itself is paired with the snap taken on the line above, so the
  route it admits reads a consistent segment set.
• NB2 — "all keep the code paths they already had" was not strictly true.
  query_needs_fts flips false → true for this shape and re-gates three
  branches (the size:0 columnar agg fast path, count_authoritative, the F1
  total_count overwrite). All three are inert here — the fast path also needs
  is_match_all || agg_filter.is_some() and query_node_to_agg_filter declines
  a should bool; the other two only ever remove a bounded-count shortcut,
  leaving the FTS path's own authoritative seg_hits.len() tally. Outcome
  unchanged, gates re-evaluated. Recorded at the binding and corrected in the
  PR body.
• NB4 — "byte-identical" downgraded to "identical … one request shape per
  column, compared as JSON", which is what the capture supports.

NB3 (harness not in-tree), NB5 (the id_pos_map_for cache as new steady-state
RAM — it is budgeted under SegmentCacheCategory::IdPositions and participates
in eviction) and NB6 (no dedicated mixed-segment test) are acknowledged and not
changed here.

Merge

main moved by rc.72 plus #899, #900 and #894.

• CHANGELOG.md — the only textual conflict, a pure additive collision at
  the top of ## [Unreleased]. Both sides kept, knn beside query is answered by a full stored-doc scan: the pinned Ids clauses are unprojectable to FTS (follow-up to #825) #892 first and the two
  CI: Build+Test step 6 (workspace tests at default parallelism) intermittently hangs on hosted runners #751 entries after. Verified by diffing the merged file against origin/main:
  the delta is exactly the knn beside query is answered by a full stored-doc scan: the pinned Ids clauses are unprojectable to FTS (follow-up to #825) #892 block.
• index.rs — auto-merged; fix(engine): cold-segment hydration must not queue on tokio's blocking pool (#751) #899 (stored_values_for_async → background_pool()),
  perf(engine): an idle index costs 0.14 MB, not 0.64 MB (#873) #900 (per_index_map_shards, WAL-descriptor tests) and knn beside query is answered by a full stored-doc scan: the pinned Ids clauses are unprojectable to FTS (follow-up to #825) #892 touch disjoint
  regions. Both siblings confirmed present after the merge and their own tests
  re-run green here.

Gates, all on this merged tree

cargo fmt --all --check                                                   clean
cargo build  --release -j 8 -p xerj-fts -p xerj-engine                    clean
cargo clippy --release -j 8 -p xerj-fts -p xerj-engine -- -D warnings     clean
cargo test --release -p xerj-engine --lib pinned_knn_fts_892           3 passed
cargo test --release -p xerj-api  --test knn_beside_query_with_aggs_stays_200
                                          4 passed  (#825 contract, UNCHANGED)
cargo test --release -p xerj-engine --test agg_corpus_hydration_deadlock
                                                       1 passed  (#899 sibling)
cargo test --release -p xerj-engine --lib per_index_map_tests
                                                       1 passed  (#900 sibling)
cargo test --release -p xerj-engine --lib an_index_that_is_not_being_written_to_holds_no_wal_descriptors
                                                       1 passed  (#900 sibling)

The #825 contract tests are byte-unchanged — git diff origin/main -- engine/crates/xerj-api/ engine/tests/ is empty. ES-YAML conformance is left to
CI, as before: its runner does DELETE /_all and this box is shared.