#825 knn beside query scores as a sum over the union, aggs included

[MavenRain]

fix(engine): #825 knn beside query scores as a sum over the union, aggs included

Closes #825.

Contract

ES 8.x semantics, option 1 on the issue as pinned by the follow-up comment: knn beside query is a disjunction. The hit set is the union of both halves, each document scores query_score + knn_score (boost weights each side), and aggregations are computed over the union. Not RRF.

Two wire-visible consequences, stated up front rather than buried:

• New 400. {query, knn:{field: <not a vector field>}, aggs} now answers 400 (the field is not answerable by knn) where it used to answer 200 with lexical-only hits. pin_knn_clause runs the same knn against an index with no dense_vector field returns 0 hits / HTTP 200 with no error (silent false-negative) #498 unanswerable-field check as the single-knn and multi-knn arms, so an empty non-timed-out leg fails loud instead of degrading to the lexical half. That matches ES and is consistent with knn against an index with no dense_vector field returns 0 hits / HTTP 200 with no error (silent false-negative) #498/Extend the unanswerable-knn-field 400 to multi-knn arrays and nested knn #542, but it is a behaviour change for existing callers.
• explain: true renders the pinned clauses. Membership and the summed _score are right, but the explanation tree shows constant_score(ids) per pinned neighbour, not anything resembling ES's knn explanation. Previously wrong in a different way (the kNN half was absent from the explanation because it was absent from the answer), so this is not a regression — but "explain applies to the union" means membership, not explanation text.

Root cause

The es-compat layer folded knn + query into bool.should, but the generic scorer has no Knn arm: doc_matches_query_typed falls to the catch-all false and score_query_against_doc contributes 0.0. With the default minimum_should_match, a knn-only document was excluded entirely and a lexical match lost its vector score. The #458 hybrid_safe carve-out routed one narrow shape through RRF, which diverges from ES scoring and rejects aggs with a 400; every other shape (aggs, sort, collapse, rescore) hit the silent drop.

Fix

Engine-side rewrite so every request feature works through the one generic path:

• pin_knn_clause (index.rs) pre-executes the knn leg with the existing executors. HNSW only when the leg is plain (no filter, boost, or similarity cutoff, and no aggs on the request, preserving the rc.6 exact-aggs rule); otherwise brute force. An empty non-timed-out leg still raises the knn against an index with no dense_vector field returns 0 hits / HTTP 200 with no error (silent false-negative) #498 unanswerable-field 400.
• The top-k comes back as bool.should of Constant{score, Ids{[id]}} pinned clauses. Bool sums clause scores, so each pinned doc adds exactly its knn score to whatever the query half scores. The aggregation pass re-runs the matcher over the full corpus, so pinned docs reach facets with no extra plumbing. Sort, collapse, rescore, highlight, and min_score all see the rewritten tree through the normal path.
• compound_bool_direct_knn / replace_direct_knn_with_pinned do the tree surgery; the rewrite only fires for a compound bool with exactly one direct Knn child (the shape the compat fold emits). The native hybrid query type and the multi-knn-array 400 are untouched.
• es_compat.rs: the fix(es-compat): knn beside query returned the lexical answer and called it hybrid #458 hybrid_safe block and RRF fold are deleted; the fold is now unconditionally bool.should[query, knn].

Review round 2 — the pinned score survives the post-scan rescore

The summed-score contract held only while the lexical half was a single scoring text clause. With a bool carrying two or more ({"query":{"bool":{"must":[{"match":{"title":…}},{"match":{"body":…}}]}},"knn":{…}}) the post-scan IDF heuristic rescore in search_inner fired on the pinned tree and rewrote _score from term frequencies alone — dropping the vector half for exactly the documents reached by both halves, while a vector-only document (tf 0, heuristic 0) kept its kNN score and could then outrank them. Watched fail-before on the two-document fixture: both came back at 0.61739457, byte-identical, vector contribution gone.

query_uses_bool_text could not see the difference: the user's inner bool supplies the ≥2 text clauses and the pinned sub-tree contributes 0 text children, so it neither disqualifies nor suppresses. Nothing else declines either — scored_fast_plan bails on Constant{Ids} and the FTS projection declines on Ids, so both scored_fast_applied and exact_bm25_page are false.

Fixed by carrying the pinned clauses' id → knn-score map (collect_pinned_knn_scores, read back out of pin_knn_clause's own output only — a user-supplied constant_score{filter:{ids}} keeps its pre-existing behaviour) and adding it back when the rescore rewrites a hit, rather than suppressing the rescore: the lexical half keeps the IDF weighting the pass exists to provide. The neighbouring TF-IDF max_score < 0.001 fallback is inert here — its extract_query_text answers None for any Bool — verified by reading, not assumed.

Review round 2 — the semantic_text hint stops nagging the hybrid

Build + Test was red on the reviewed head: es_compat::semantic_text_lexical_hint_tests::a_top_level_knn_over_the_companion_vector_suppresses_the_hint. The hand-rolled ES hybrid over a semantic_text companion vector came back carrying the lexical_on_semantic_text hint, telling the caller their embedding "was NOT consulted" — when it was. That hint runs on the effective body, after knn is folded into query and body.knn is None, so suppression depends entirely on dispatching_vector_fields recognising the folded tree; its bool arm only descended into a bool holding exactly ONE candidate. Under #458 the request folded to {"hybrid":…} and the hybrid arm walked it; unconditional bool.should[query, knn] is two candidates. The bool arm now mirrors compound_bool_direct_knn clause for clause, scoped to knn; semantic keeps the #394 rule.

Tests

knn_beside_query_with_aggs_stays_200.rs, reframed from #458 to #825:

• aggs test: a vector-only document (query vector match, lexically unreachable) must appear in hits AND in its term bucket (aggs over the union), and vector scores must separate two BM25-tied lexical docs.
• union test: the doc matching both halves must outrank a perfect vector-only doc (sum ordering).
• rescore test: renamed, core assertion flipped. The vector doc must now be PRESENT (under fix(es-compat): knn beside query returned the lexical answer and called it hybrid #458 it was asserted absent).
• bool-query test: two documents with identical lexical content and different vectors, behind a two-clause bool lexical half — the one shape that reaches the IDF rescore. near is indexed second so a collapsed tie would sort it last rather than accidentally passing.

New ES-compat YAML case vectors/96_knn_beside_query_aggs.yml (per CONTRIBUTING): over HTTP, knn beside query with a terms agg returns the 3-document union and buckets over it (a:2 including the lexically-unreachable vector doc's sibling, z:1 the vector-only doc); a no-aggs variant asserts the same union. It runs as part of the full suite, and the ES-compat YAML conformance job was green on the reviewed head.

Correction to the original evidence claim. The first commit body said "the union, facet, ordering, and rescore assertions fail on the unfixed code". That is only true of the aggs test and the rescore test. knn_beside_query_returns_the_vector_only_document, including its hits[0] == "lex" sum-ordering assertion, passes unchanged on the unfixed code: that request is hybrid_safe under #458, so it went to the RRF executor, which also returns both documents and also ranks "lex" first (1/61 + 1/62 = 0.0325 vs 1/61 = 0.0164). The assertion does not discriminate sum from RRF on that fixture. The real fail-before evidence is the aggs test, the rescore test, and the new bool-query test.

Known cost — measured, not asserted

A pinned tree contains ids clauses, which query_node_to_fts cannot lift (its should loop ?-aborts on the first unprojectable clause), so fts_query is None and the request is answered by a stored-document scan of every segment instead of the inverted index.

100 000 documents (text + 8-dim dense_vector, ~10 % lexical selectivity, 1 shard), closed-loop latency, fresh query vector per request so the query cache cannot answer twice (an identical-body loop reported 0.4 ms for a 213 ms request — the cache mirage). Medians over 2 interleaved rounds × 7 requests on one data directory:

request	main (RRF where eligible)	reviewed head	this PR now
query alone	0.3 ms	0.3 ms	0.3 ms
knn alone, k=10	1.2 ms	1.1 ms	1.1 ms
query + knn k=10	2.5 ms	248 ms	208 ms
query + knn k=100	3.5 ms	436 ms	227 ms
query + knn k=1000	18.4 ms	2 309 ms	385 ms
query + knn k=10000	168 ms	24 437 ms	5 071 ms
query + knn + terms agg, k=10	476 ms (kNN half dropped — wrong answer)	643 ms	605 ms

Read honestly, that is two different trades:

• For the shapes that already carried the silent drop (aggs/sort/collapse/rescore), the cost is about a quarter more and the answer goes from wrong to right.
• For the no-extras shape, fix(es-compat): knn beside query returned the lexical answer and called it hybrid #458 routed to RRF over two indexed sub-searches at 2.5 ms. This PR answers it correctly at ~208 ms — an ~80× regression on the canonical hybrid. That is real and it is not hidden.

Where the floor comes from: it is not new machinery. Any bool.should with an unprojectable clause already costs the same, on main as here — control on the same corpus, no knn anywhere:

request (no knn anywhere)	main	reviewed head
bool.should[match, match] (projectable)	10.4 ms	9.8 ms
bool.should[match, ids] (unprojectable)	227.6 ms	237.1 ms

What changed is the routing: the no-extras hybrid now lands where every other shape of the same request already landed. Restoring an indexed route means projecting the lexical half alone and merging the ≤ k pinned documents back in afterwards (the FTS scored path already holds a score for every lexical match) — that touches hit merging, hits.total, the agg corpus, sort/collapse/pagination and the memtable arm, and is deliberately not a drive-by inside a correctness fix. Tracked with the full design and these numbers as #892.

The O(docs × k) surcharge — fixed

On the reviewed head each document also paid k clause evaluations, each re-reading _id out of the source map — the 24.4 s at k=10000 above. Two changes remove most of it:

• pin_knn_clause emits the pinned sub-tree with a redundant filter: [Ids{all k}] accelerator (same match set, contributes nothing to _score), and the scan evaluates filter before should — so a document outside the top-k is rejected by one _id lookup plus a memcmp sweep instead of recursing into all k clauses. minimum_should_match: 1 is written out explicitly so the tree still says disjunction.
• doc_matches_query_typed's should count short-circuits at min instead of .filter(…).count()-ing every clause on every document. Same predicate — and a document the lexical clause already matched no longer walks the pinned sub-tree at all.

k is still unbounded on the wire (knn_clause_k takes explicit k, else num_candidates, else 10), so a large-k request on a large index remains expensive — bounded by the 30 s default deadline, not by a cap. Enforcing ES's own num_candidates ≤ 10000 limit would be a separate wire-behaviour change and is not smuggled in here.

CHANGELOG

An Unreleased / Fixed entry carries both user-visible consequences — the new 400 and the measured slowdown — because the repo ships that file and the cost belongs in it.

[xerj-org]

Review response

Both blocking findings are accepted — neither is refuted. One was reproduced as a failing test before being fixed; the other was measured on a 100 000-document index and partly mitigated, with the residual cost written into the PR body and the CHANGELOG instead of a one-line disclaimer. Four commits on top of the reviewed head (892304df, cef2dba1, 5fea5df7, baaca1a0), including a fix for a CI-red failure the review could not see because the checks were still pending at the time.

---

Blocking 1 — pinned kNN score dropped by the post-scan IDF rescore

Confirmed exactly as described, and it is worse than "silent": the two documents come back with a byte-identical score.

892304df fix(engine): #825 pinned kNN scores survive the post-scan IDF rescore

Fail-before, on the reviewed head, with a two-clause bool as the lexical half and two documents that differ only in their vector:

near=Some(0.61739457) far=Some(0.61739457)

0.61739457 is the IDF heuristic score. The vector contribution is gone for both. Your trace was right in every step: query_uses_bool_text walks the outer bool, the user's inner bool returns (2, true), the pinned sub-tree returns (0, false) and neither disqualifies nor suppresses; scored_fast_plan bails on Constant{Ids} and the FTS projection declines on Ids, so scored_fast_applied and exact_bm25_page are both false and nothing else stops the block.

Fixed by adding the pinned constant back rather than suppressing the rescore — the lexical half keeps the IDF weighting the pass exists to provide, and query_score + knn_score is restored. collect_pinned_knn_scores reads the map out of pin_knn_clause's own output only, never out of the user's tree, so a user-supplied constant_score{filter:{ids}} keeps its pre-existing behaviour. Vector-only hits score 0 under the heuristic and were already skipped by the score > 0.0 gate, so they keep the pinned constant untouched.

Also checked, since it is the same class: the neighbouring TF-IDF max_score < 0.001 fallback is inert on a pinned tree — its extract_query_text answers None for any Bool. Read, not assumed.

New guard: knn_beside_bool_query_keeps_the_vector_contribution. Your point that the existing suite could not catch this is why it uses a two-clause bool, and near is indexed second so a collapsed tie would sort it last rather than accidentally passing.

---

Blocking 2 — performance regression on the canonical hybrid

Accepted, measured, and partly fixed. baaca1a0 perf(engine): #825 bound the pinned-kNN doc scan; measure the real cost

100 000 documents (text + 8-dim dense_vector, ~10 % lexical selectivity, 1 shard), closed-loop, fresh query vector on every request so the query cache cannot answer twice — an identical-body loop reported 0.4 ms for a 213 ms request, so the first version of this measurement was a mirage. Medians over 2 interleaved rounds × 7 requests on one data directory:

request	main	reviewed head	now
query alone	0.3 ms	0.3 ms	0.3 ms
knn alone k=10	1.2 ms	1.1 ms	1.1 ms
query + knn k=10	2.5 ms	248 ms	208 ms
query + knn k=100	3.5 ms	436 ms	227 ms
query + knn k=1000	18.4 ms	2 309 ms	385 ms
query + knn k=10000	168 ms	24 437 ms	5 071 ms
query + knn + terms agg k=10	476 ms (wrong answer)	643 ms	605 ms

Two separate things were in that number, and only one of them is this PR's doing.

The O(docs × k) surcharge is fixed. pin_knn_clause now emits the pinned sub-tree with a redundant filter: [Ids{all k}] accelerator — same match set, no effect on _score, and the matcher evaluates filter before should, so a document outside the top-k is rejected by one _id lookup plus a memcmp sweep instead of recursing into all k Constant{Ids} clauses and re-reading _id k times. minimum_should_match: 1 is written out explicitly so the tree still says disjunction. And doc_matches_query_typed's should count now short-circuits at min instead of .filter(…).count()-ing every clause on every document — same predicate, and a document the lexical clause already matched no longer walks the pinned sub-tree at all. That is the 2 309 → 385 ms and 24 437 → 5 071 ms column.

The ~200 ms floor is not new machinery, and it is not fixed. Control on the same corpus, no knn anywhere:

request (no knn anywhere)	main	reviewed head
bool.should[match, match] (projectable)	10.4 ms	9.8 ms
bool.should[match, ids] (unprojectable)	227.6 ms	237.1 ms

Any bool.should with an unprojectable clause already costs that, on main as here. What this PR changed is the routing: the no-extras hybrid now lands where every aggs/sort/collapse/rescore shape of the same request already landed. Read honestly that is two different trades — the previously-broken shapes cost about a quarter more and go from wrong to right; the no-extras shape is an ~80× regression against an RRF route that returned ES-divergent scores. Both are now in the PR body and in the CHANGELOG, with the numbers, not as "routing can be tightened in a follow-up if profiles demand".

Not fixed, and why. Restoring an indexed route means projecting the lexical half alone and merging the ≤ k pinned documents back in afterwards — the FTS scored path already holds a score for every lexical match, so the pieces exist — but it touches hit merging, hits.total, the agg corpus, sort/collapse/pagination and the memtable arm inside a 4 700-line function. Doing that as a drive-by inside a correctness fix is how the next silent-drop bug gets written. Filed as #892 with the full design and these numbers.

On the k cap: k is still unbounded on the wire (knn_clause_k: explicit k, else num_candidates, else 10), bounded in practice only by the 30 s deadline. ES's own limit is num_candidates ≤ 10000 with k ≤ num_candidates — enforcing it is a separate wire-behaviour change and I did not smuggle it into this PR. Worth its own decision.

---

Not in the review — CI went red after you looked (your NB5)

cef2dba1 fix(api): #825 a pinned knn inside a compound bool DOES reach the vector

Build + Test failed on the reviewed head: es_compat::semantic_text_lexical_hint_tests::a_top_level_knn_over_the_companion_vector_suppresses_the_hint. The hand-rolled ES hybrid over a semantic_text companion vector came back carrying the lexical_on_semantic_text hint — telling the caller their embedding "was NOT consulted", when it was. That hint runs on the effective body, after knn is folded into query and body.knn is None, so suppression depends entirely on dispatching_vector_fields recognising the folded tree; its bool arm only descended into a bool holding exactly ONE candidate. Under #458 the request folded to {"hybrid":…} and the hybrid arm walked it. Unconditional bool.should[query, knn] is two candidates.

The rule was out of date, not merely unlucky — #825 changed the underlying truth. The bool arm now mirrors compound_bool_direct_knn clause for clause (exactly one direct knn child across must + should), scoped to knn; semantic keeps the #394 rule, and a_semantic_clause_that_never_reached_the_vector_is_still_flagged still proves it. 229 xerj-api lib tests pass.

---

Non-blocking notes

• NB1 (inaccurate evidence claim) — accepted and corrected in the PR body. You are right: knn_beside_query_returns_the_vector_only_document, including the new hits[0] == "lex" assertion, passes unchanged on the unfixed code, because RRF also returns both documents and also ranks "lex" first (1/61 + 1/62 = 0.0325 vs 1/61 = 0.0164). The assertion does not discriminate sum from RRF on that fixture. The real fail-before evidence is the aggs test, the rescore test, and now the bool-query test.
• NB2 (new wire-visible 400) — promoted out of a parenthetical. It is now the first bullet of the Contract section and a called-out upgrade note in the CHANGELOG.
• NB3 (explain over-claim) — corrected. The PR body now says plainly that explain applies to membership, and that the explanation tree renders the pinned constant_score(ids) clauses rather than an ES-shaped knn explanation.
• NB4 (doc nit) — fixed. compound_bool_direct_knn's comment now says that a Knn sitting directly in filter or must_not also keeps the dropped behaviour, with the reason (they are the non-scoring lists, a pinned constant there would contribute nothing, and the compat fold only ever emits bool.should[query, knn]).
• NB5 (CI) — see above; the one real failure is fixed. The ES-compat YAML conformance job was green on the reviewed head, 96_knn_beside_query_aggs.yml included.
• NB6 — thank you for listing what you had already checked; it is why this pass went into the rescore interaction and the routing rather than re-deriving the union argument.

---

Follow-up: #892

The union is FTS(lexical) ∪ {k known documents}. The FTS scored path already computes a score for every lexical match (seg_hits) and materialises only the top prefix, so:

1. project the lexical half alone (exact BM25, existing fast path);
2. add the pinned kNN score to any seg_hits entry in the pinned set;
3. materialise the pinned documents FTS did not return, at knn_score plus their own lexical score (0 when the lexical half does not match them);
4. hits.total = lexical total + |pinned \ lexical|;
5. the agg corpus pass keeps the rewritten-tree route, or takes the same union.

The alternative — teaching query_node_to_fts an Ids arm — needs _id in the FTS term dictionary, which is an on-disk format change and strictly larger.

Gates: cargo fmt --all --check clean; cargo clippy --release -p xerj-engine -p xerj-api -- -D warnings clean; xerj-engine lib 664 passed / 0 failed; xerj-api lib 229 passed / 0 failed; all four tests in knn_beside_query_with_aggs_stays_200.rs green.

[xerj-org]

CI verdict on the repaired head (9da056a8)

19 of 20 checks green, including both gates that matter here:

• ES-compat YAML conformance: pass — vectors/96_knn_beside_query_aggs.yml runs in it. I also replayed that case by hand against a throwaway node on a private port after the pinned-tree change (the redundant filter: [Ids{all k}]), to be sure the accelerator did not move the union: hits.total 3, buckets a:2 / z:1, and the no-aggs variant hits.total 3. Unchanged.
• Format + Clippy: pass. Also Release build (fat LTO), API smoke, ONNX, Fuzz, Use-case harnesses, Autoindex FD smoke on all three OSes.

Build + Test is red, and it is not this PR. The xerj-api --lib failure the review's NB5 could not see is fixed — 229 passed; 0 failed, with a_top_level_knn_over_the_companion_vector_suppresses_the_hint green in both passes, and all four knn_beside_query_with_aggs_stays_200 tests green including the new knn_beside_bool_query_keeps_the_vector_contribution. What kills the job now is one step further on:

test script_bucketed_agg_past_the_call_depth_limit_is_an_error_not_empty_buckets has been running for over 60 seconds
##[error]The action 'Workspace tests (all targets) at default parallelism (contributor's `cargo test`)' has timed out after 12 minutes.

That same test, in that same step, hangs the same way on main at 5f3b6ea3 — this branch's merge base — in run 33327063643 (job 99298970619), which is why main itself is red. It passes in the job's earlier restricted-parallelism pass and only hangs at default parallelism on the 2-core runner, so it reads as the CI-runner blind-spot class rather than a product bug — but it is pre-existing either way, and unrelated to knn, aggregations over the union, or anything this PR touches. It needs its own fix on main; this branch cannot be green until it lands.

Local runs on the repaired head, for completeness:

• xerj-engine --lib: 664 passed / 0 failed
• xerj-api full suite (60 binaries): 513 passed / 0 failed
• cargo fmt --all --check clean, cargo clippy --release -p xerj-engine -p xerj-api -- -D warnings clean