Skip to content

feat(query): search plan truth — retrieval IR, projectable metrics, deterministic ties - #595

Closed
ragnorc wants to merge 10 commits into
mainfrom
search-contracts-p0-p1
Closed

feat(query): search plan truth — retrieval IR, projectable metrics, deterministic ties#595
ragnorc wants to merge 10 commits into
mainfrom
search-contracts-p0-p1

Conversation

@ragnorc

@ragnorc ragnorc commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

What & why

Phases P0 (substrate residuals) and P1 (plan truth) of the search-contracts RFC (dev graph: spc-rfc-0039-search-contracts, v0.2.2). No format change and no new query syntax beyond retiring fuzzy(); the two motivating bug classes — silently unranked/dropped search on traversal-bound targets, and silently case-sensitive matching on unindexed columns — become loud, and ranking metrics become honest: projectable, deterministically ordered, and self-describing. Eight staged commits, each green in isolation.

P0

  1. test(engine): fences for the Lance 11 update→optimize_indices stale-vector window (upstream lance#8342/#7371) — a pure-Lance fence on the staged merge-insert route plus a graph-level db.optimize() twin. Vector indexes are foldable (can_fold_index), so the window was reachable; nothing pinned the v11 fix.
  2. feat(compiler): fuzzy() retired with the stable T25 diagnostic. It was provably inert (the pinned one-edit typo matched nothing), so every use was a confident empty answer; a retired form never reaches execution.
  3. feat(query,api): advisory read warnings (QueryResult.notices → additive ReadOutput.warnings; legacy /read drops them by construction), first use full_text_search_unindexed: text search on a column with no FTS index still serves via Lance's flat fallback but that fallback tokenizes case-sensitively — now loud instead of silent.

P1
4. test(search): characterization goldens (nearest-tie secondary keys; instrument pin that limit pushdown stays disqualified under search ordering) — the equivalence baseline for the refactor.
5. refactor(compiler,query): retrieval stated in the plan. QueryIR.retrieval: Option<RetrievalIR> (Nearest/Bm25/FuseRrf) is lowered once — including the #574 bounded-scan policy and per-arm candidate counts — and the executor's order_by[0] re-inspection (extract_search_mode family) is deleted. SearchMode, the uncapped retry, and every scan/fusion path are untouched: the diff is confined to where the mode comes from.
6. feat(compiler,query): T26 — search/rank targets must be scan-rooted. The lowering component-root walk is shared with a new typecheck pass (the deferred-explicit-binding case is pinned), and the engine now refuses instead of silently dropping (hoist loop + retrieval-target backstops). Closes the restated iss-nearest-dropped-by-traversal.
7. feat(query): metric columns projectable (bm25(...) as score, nearest(...) as distance, rrf(...) as fusion observe the executed ranking; mismatch is a loud error) and deterministic ties everywhere: fused RRF sorts (score desc, entity id asc) with the fused score materialized as a real column, trailing order keys apply inside fused ties (previously silently ignored), aggregated search-ordered queries sort their tail + warn instead of ignoring the whole order clause, aggregate ties become total. One golden updated: it encoded an arrival-order tie.
8. feat(query,api): additive metrics/retrievals response metadata. Every executed source is described (recall is the source contract), and nearest() on an @embed-backed property reports exact ready/pending embedding coverage over the prefiltered population (via a new sealed TableStore::count_rows_matching), with an embedding_coverage_pending warning — a short or empty ranked answer is visibly incomplete rather than confidently wrong.

Deliberately deferred

Arm-level metric projection inside rrf (fused score owns {var}._score until the system-column work lands); FTS coverage in retrievals (index-status RFC territory, #552); moving the must-lead-order rejection to typecheck; CLI rendering of metrics/retrievals (warnings print to stderr); RRF rank derivation from score columns (entangled with #587's fusion rewrite).

Coordination

Validation

  • Canonical workspace graph (failpoints superset): 99 suites, 2,837 passed, 0 failed on the pinned 1.97.1 toolchain.
  • Both Clippy graphs -D warnings -W clippy::dbg_macro; cargo fmt --check; check-agents-md.sh; check-docs.py.
  • OpenAPI regenerated (ReadWarningOutput, MetricOutput, RetrievalOutput, EmbeddingCoverageOutput); vocabulary-guard inventory classifies the new occurrences as result_projection.
  • Focused owners per commit: lance_surface_guards 34, maintenance 39, compiler 329 (+6 lowering, +5 T26, T25), search 55, data_routes 56, openapi 102, forbidden_apis 22, plus ordering/aggregation/traversal/proptest_equivalence/literal_filters/end_to_end.

Release notes for every user-visible change are in docs/releases/v0.11.0.md; the search guide documents the new contracts (T25/T26, warnings, metric projection, determinism, coverage).


Note

High Risk
Changes query compile-time rules (T25/T26), ranking/RRF ordering semantics, and read response shape; incorrect tie-breaking on RRF fanout at limit was flagged in review as a remaining edge case.

Overview
Implements search-contracts P0+P1: ranked reads become explicit, loud, and self-describing instead of silently wrong.

Compiler & execution: QueryIR gains a lowered retrieval plan (Nearest / Bm25 / FuseRrf) with BM25 scan caps decided at compile time; the executor resolves that plan instead of inferring search from order_by[0]. T26 rejects search/match_text/rank functions on traversal-introduced bindings (shared scan-root logic with lowering); the engine backstops with hard errors. fuzzy() is retired at typecheck (T25).

Results & API: Canonical ReadOutput adds additive warnings, metrics, and retrievals (plus embedding ready/pending for @embed nearest); legacy POST /read omits them. CLI non-JSON formats print warnings on stderr. Rank expressions in return must match the executed retrieval; RRF gets deterministic fusion order, materialized fused scores, trailing order keys inside ties, and aggregation notices when rank cannot apply.

Operational signals: Unindexed full-text search emits full_text_search_unindexed; pending embeddings emit coverage metadata and embedding_coverage_pending. Lance 11 vector optimize-after-update fences and docs/skills drop fuzzy references.

Reviewed by Cursor Bugbot for commit 3e459aa. Bugbot is set up for automated code reviews on this repo. Configure here.

Greptile Summary

The PR makes retrieval strategy explicit in the query IR and expands ranked-query contracts, metadata, and deterministic ordering.

  • Retires fuzzy() and rejects search targets that are not rooted in a scan.
  • Adds projectable ranking metrics, retrieval metadata, embedding-coverage warnings, and API output types.
  • Materializes RRF scores and applies trailing ordering keys with deterministic ID tie-breakers.
  • Adds Lance optimization regression coverage and updates documentation and OpenAPI output.

Confidence Score: 5/5

The PR appears safe to merge because the previously reported RRF parallel-edge ordering instability is resolved and no blocking failure remains.

No blocking failure remains.

Important Files Changed

Filename Overview
crates/omnigraph/src/exec/query.rs Executes explicit retrieval plans, materializes ranking metrics, reports embedding coverage, and makes RRF tie ordering deterministic.
crates/omnigraph/src/exec/projection.rs Adds rank-metric projection and deterministic ID-based ordering tie-breakers.
crates/omnigraph-compiler/src/ir/lower.rs Lowers leading ranking expressions into explicit retrieval IR and centralizes scan-root analysis.
crates/omnigraph-compiler/src/query/typecheck.rs Retires fuzzy search and enforces scan-rooted search and ranking targets.
crates/omnigraph-api-types/src/lib.rs Adds additive warning, metric, retrieval, and embedding-coverage fields to canonical read responses.
crates/omnigraph/src/table_store.rs Adds snapshot-pinned filtered row counting used for embedding-coverage reporting.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Parse query] --> B[Type-check search contracts]
  B --> C[Lower RetrievalIR]
  C --> D[Execute nearest, BM25, or RRF]
  D --> E[Materialize ranking metrics]
  E --> F[Apply deterministic ordering]
  F --> G[Project rows and metadata]
  G --> H[API or CLI output]
Loading

Reviews (2): Last reviewed commit: "fix(query,cli): address PR #595 review f..." | Re-trigger Greptile

Context used:

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-01T13:29:33.549576Z a99c153 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Comment thread crates/omnigraph/src/exec/query.rs Outdated
Comment on lines +1111 to +1117
result_batch = apply_ordering(
result_batch,
&orderings,
&fused_batch,
params,
Some(rrf.limit),
)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 RRF fanout ties remain unstable

When an RRF result fans out through parallel edges to the same target and the limit cuts through those rows, apply_ordering gives them identical fused-score and node-ID keys, so its unstable sort can select different edge rows at the limit boundary. Include a row-level discriminator such as the physical edge ID to make this ordering total.

Knowledge Base Used: Query execution and projection

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declined after verification: apply_ordering's deterministic tie-break appends every .id-suffixed source column name-sorted — the traversal destination's id and, when an edge is bound, the edge binding's id are both in that set, so distinguishable fanout rows already order totally. Rows with no distinguishing column at all are content-identical, so their relative order at the limit boundary is unobservable. Happy to revisit if you have a concrete shape where two observably different rows share every id column.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I rechecked apply_ordering in crates/omnigraph/src/exec/projection.rs:543 and its RRF call in crates/omnigraph/src/exec/query.rs:1650: the tie-breaker includes every .id source column in canonical order, including bound edge IDs when present. Parallel edges with an edge binding are therefore ordered deterministically; when no distinguishing ID is exposed, the rows are observably identical. This finding is not actionable, so I withdraw it—no code change is needed.

Comment thread crates/omnigraph/src/exec/query.rs Outdated
Comment thread crates/omnigraph/src/exec/query.rs

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a99c153a30

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +421 to +422
IRExpr::Nearest { variable, .. } => {
let col_name = format!("{variable}._distance");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Validate metric projections against the full retrieval

When a return expression uses the same binding as the ordered retrieval but changes its property or query argument, this lookup still succeeds because every nearest search on that binding shares {variable}._distance (and BM25 has the analogous _score behavior). For example, ordering by nearest($d.embedding, $q1) while returning nearest($d.other_embedding, $q2) silently projects the first retrieval's distance and emits metadata describing the second, rather than producing the promised mismatch error. Compare the projected rank expression with QueryIR::retrieval, not merely the synthetic column name.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 3e459aa: rank projections are now validated structurally (source kind + target + query argument fingerprint) against the executed retrieval before execution, so return nearest($d.other, $q2) under order nearest($d.embedding, $q1) fails loudly instead of projecting the executed retrieval's distances under the wrong metadata. Pinned by metric_projection_mismatched_retrieval_errors.

Comment thread crates/omnigraph/src/exec/query.rs Outdated
},
descending: true,
}];
orderings.extend(ir.order_by.iter().skip(1).cloned());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Apply RRF secondary keys before selecting winners

When more than limit entities tie on the fused score, scored.truncate(rrf.limit) has already selected winners by entity ID before these trailing order keys run. Consequently, a query such as order { rrf(...), $d.priority desc } limit 2 can exclude higher-priority tied entities and only reorder the ID-selected subset, violating the stated secondary ordering. Retain all candidates tied at the cutoff, or apply the complete ordering before truncating the winner set.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 3e459aa: the winner cut now retains the full boundary tie plateau (tied fused scores are bit-identical, so the extension is exact and bounded by the plateau width), and the final row slice still enforces the limit. rrf_boundary_tie_honors_secondary_key pins a limit-1 cut inside a two-entity tie where the trailing key must pick the id-descending entity.

Comment thread crates/omnigraph/src/table_store.rs Outdated
Comment on lines +4361 to +4364
let batches = stream
.try_collect::<Vec<RecordBatch>>()
.await
.map_err(OmniError::storage)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Stream embedding coverage counts without retaining row IDs

For an @embed-backed nearest query over a large prefiltered population, each coverage count collects every matching id batch into a Vec<RecordBatch> before summing, and the caller performs this once for ready rows and again for pending rows. Thus an otherwise bounded top-k query can retain O(population) IDs and OOM even though only a scalar count is needed; fold the stream incrementally or use a count operation. This conflicts with the repository requirement that memory and hot-path resource use remain bounded.

AGENTS.md reference: AGENTS.md:L104-L106

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 3e459aa: count_rows_matching now folds the stream incrementally — no batch is retained, only the scalar count. Thanks for the AGENTS.md-grounded catch.

Comment thread crates/omnigraph-cli/src/output.rs Outdated
pub(crate) fn print_read_output(output: &ReadOutput, format: ReadOutputFormat) -> Result<()> {
// Human formats cannot carry structured warnings in the rendered rows, so
// surface them on stderr; JSON/JSONL callers read `warnings` in-band.
if !matches!(format, ReadOutputFormat::Json | ReadOutputFormat::Jsonl) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve warnings for JSONL callers

When the CLI uses --format jsonl, this branch suppresses stderr warnings on the assumption that they are rendered in-band, but render_jsonl only places query_name, target, and row_count in its metadata record. As a result, advisories such as full_text_search_unindexed and embedding_coverage_pending disappear entirely for JSONL consumers. Include warnings in the JSONL metadata record or continue printing them to stderr for this format.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 3e459aa: stderr warning printing now applies to every format except full JSON (render_jsonl's slim metadata record indeed has no warnings field). Extending the JSONL metadata record itself is left to the CLI-rendering follow-up noted in the PR body.

Lance 10 could keep a row's pre-update vector alive through vector-index
maintenance: merging delta segments retained rows updated in place
(upstream lance#8342) and per-segment search served stale rows after an
in-place column update (lance#7371) — KNN then returned a row twice, once
mis-ranked at its old position. Lance 11 fixed both, but nothing pinned
that: the bump's new tests are FTS-rebuild-focused, and the existing
vector guard covers the delete→optimize path only.

OmniGraph's optimize feeds every foldable index — vector indexes
included, per TableStore::can_fold_index — into optimize_indices, so the
window is reachable through db.optimize(). Two fences, per the
maintenance-and-substrate-fences ownership split:

- lance_surface_guards::vector_optimize_after_update_serves_only_current_vectors
  pins the substrate: staged merge-insert update of a scattered subset,
  a default optimize_indices fold, then an indexed read that must serve
  every row exactly once at its post-update position, distance-ordered.
- maintenance::optimize_after_vector_update_serves_updated_ranking is the
  graph-level twin: keyed merge re-load of one embedding, db.optimize(),
  per-name aggregate fragment coverage proof, and a nearest() ranking
  that must follow the new embedding exactly once.
fuzzy() was provably inert: the supported tokenizer swaps the query
analyzer to a bare non-stemming form while the index stores stemmed
lowercased terms, so a one-edit typo exceeded max_edits and every fuzzy
query returned a confident empty result with no signal. The two existing
tests pinned that contradiction deliberately (one asserted nothing, one
asserted emptiness with a promote-me note).

Rejection happens at typecheck — a retired form never reaches execution:
- the Expr::Fuzzy arm now returns the stable T25 retirement diagnostic
  (parse still succeeds, so the error is T25, not a parse error);
- the engine's dead build_fts_query arm is removed (IR variant removal
  rides the retrieval-IR refactor);
- fuzzy_search leaves the uncertified-refusal route list (compile fails
  before the certification gate can fire) and the two contradictory pins
  are replaced by a public-API T25 assertion;
- user docs, skill references, and the v0.11.0 release note record the
  retirement and the successor guidance.

Part of the search-contracts RFC P0 (dev graph:
spc-rfc-0039-search-contracts).
Adds the warning carrier the search-contracts RFC's P0/P1 phases share,
and its first use: a full-text function on a column with no FTS index
still serves (Lance plans a flat scan) but that fallback tokenizes with
a bare, case-sensitive analyzer — the mechanism behind the motivating
confident-false-negative bug (dev graph: iss-match-text-case-sensitive,
'anthropic' finds nothing while 'Anthropic' matches). The condition is
now loud without changing any result.

- QueryResult gains notices (stable snake_case code + message); the
  executor threads one NoticeSink explicitly — like search_mode — so the
  bm25 uncapped retry and RRF's forked arms cannot duplicate a notice.
- execute_node_scan emits full_text_search_unindexed (tracing::warn +
  notice) per FTS-targeted column with no FTS index, reusing
  TableStore::has_fts_index_on. Indexed columns are unaffected.
- ReadOutput gains an additive warnings array (serde-defaulted,
  skip-if-empty); the byte-stable legacy /read envelope drops it by
  construction; OpenAPI regenerated with ReadWarningOutput and the
  field pinned additive-optional. Human CLI formats print warnings to
  stderr; JSON/JSONL carry them in-band.
- Tests: engine pins (unindexed search serves + warns, bm25 warns,
  indexed negative control), server /query-vs-/read round-trip, OpenAPI
  field tests.

Part of the search-contracts RFC P0.
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.
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.
A search filter or rank expression targeting a traversal-introduced
(Expand-bound) binding was silently dropped or silently unranked: the
hoist pass removed the filter from the pipeline unconditionally but only
merged it into NodeScans, and a nearest/bm25 target resolved happily
through Expand dst bindings while no scanner ever ran the search (the
restated dev-graph bug iss-nearest-dropped-by-traversal).

- Lowering's component-root walk is extracted into shared
  scan_root_variables/deferred_binding_variables, and typecheck gains the
  T26 pass over match scopes (negations check their own roots) and the
  order clause (rrf arms included). The naive declared-equals-scanned
  rule would miss deferred explicit bindings — the shared helper keeps
  the rule and lowering from drifting, pinned by a dedicated test.
- Engine backstops (defense in depth for hand-built IR): the hoist loop
  now refuses instead of dropping, and execute_query validates every
  resolved retrieval target against the pipeline's NodeScan set.
- Five typecheck cases + the public-API reproduction; release note and
  search-guide contract update.

Part of the search-contracts RFC P1 (goal 17: nothing silently
ignored).
Rank values become data (search-contracts RFC P1, 'rank is data'):

- bm25()/nearest()/rrf() in RETURN project the synthesized score column
  the ordering used ({var}._score / {var}._distance / the fused score) —
  one computation, observed twice; a metric projected without its
  matching retrieval is a loud error, never a NULL column. (Arm-level
  metric projection inside an rrf query stays deferred; the fused score
  deliberately replaces any arm-raw score.)
- execute_rrf_fusion materializes the fused score as a real
  {primary_var}._score column, sorts winners deterministically (score
  desc, entity id asc — never arrival order), honors trailing order keys
  inside fused-score ties (previously silently ignored on the rrf path,
  with the single-search path's exact must-lead rejection), and runs
  apply_ordering's id tie-breaks over fanout rows.
- Aggregated search-ordered queries apply their trailing keys and emit
  the search_order_ignored_by_aggregation notice instead of silently
  ignoring the entire order clause.
- apply_ordering makes aggregate orders total too: with no .id columns,
  every source column joins the tie-break (group rows are distinct
  tuples).

One golden updated: rrf_fuses_two_vector_queries encoded an
arrival-order tie between ml-intro and dl-basics; the deterministic
policy orders the tie entity-id ascending. Eight new engine tests + one
ordering test pin the contract; release note records the observable-
order change where scores tie.

Part of the search-contracts RFC P1.
Ranked reads now explain themselves (search-contracts RFC P1, final
slice):

- QueryResult carries MetricDescriptor and RetrievalDescriptor sets. The
  metric descriptors resolve each projected rank column (kind, source,
  direction, recall — the source CONTRACT, so an index-accelerated
  nearest reports approximate even when the plan ran exactly; fusion is
  approximate whenever any arm is). Retrieval descriptors record every
  executed source even when no metric is projected, deduped across the
  bm25 uncapped retry and RRF's forked arms.
- For nearest() on an @embed-backed property, execute_node_scan computes
  exact ready/pending representation coverage over the PREFILTERED
  population, reusing the scan's own structured predicate through a new
  sealed TableStore::count_rows_matching (registered read-only in the
  forbidden-APIs registry). pending > 0 additionally raises the
  embedding_coverage_pending warning — a short or empty ranked answer is
  visibly incomplete rather than confidently wrong.
- ReadOutput gains additive metrics/retrievals arrays (string-typed
  enums, forward-tolerant); the legacy /read envelope drops them by
  construction; OpenAPI regenerated with the three new schemas and the
  additive-optional field pins.
- Tests: exact-coverage and prefilter-population engine pins, descriptor
  pins for bm25/rrf (including retrievals-without-projection), server
  ranked round-trip, OpenAPI field tests.

Part of the search-contracts RFC P1.
Rebase port for #587: ranking an Expand-introduced variable was one of
the gate's three Shape-fallback fixtures, but the bm25 never actually
ran on that shape — the fallback preserved a silent no-rank. T26 now
rejects it at typecheck, so the fixture asserts the stronger contract
(stable T26 diagnostic) while the gate's own expand-dst check remains
the engine backstop for hand-built IR. The other two Shape fixtures are
unchanged.
Five confirmed findings from the Cursor and Codex reviews:

- Aggregated rrf misaligned its sort (Cursor, high): group rows no
  longer align with fused rows, so ordering the projected result by the
  fused batch's score column could take out-of-bounds or silently
  misorder. Aggregated rrf now mirrors the single-search aggregated arm:
  trailing keys order the groups against the aggregate result and the
  rank is loudly ignored via search_order_ignored_by_aggregation.
- Embedding-coverage predicates lowercased identifiers (Cursor, medium):
  datafusion's col() normalizes unquoted names, so a camelCase @embed
  property failed count_rows_matching. Switched to ident() (#283
  precedent) and camelCased the coverage fixture to pin it.
- Metric projection validated by column name only (Codex): projecting
  nearest($d.other, $q2) under order nearest($d.embedding, $q1)
  silently observed the executed retrieval's distances under the
  un-executed one's metadata. Rank projections are now validated
  structurally (kind, target, query argument) against the executed
  retrieval before execution.
- RRF secondary keys ran after the winner cut (Codex): entities tied at
  the boundary score were pre-decided by entity id before trailing keys
  applied. The winner set now retains the full boundary tie plateau; the
  final row slice still enforces the limit.
- Coverage counts retained O(population) id batches (Codex):
  count_rows_matching now folds the stream incrementally — only the
  scalar count is kept.
- JSONL dropped warnings entirely (Codex, P2): render_jsonl's metadata
  record has no warnings field, so stderr printing now applies to every
  format except full JSON (which carries them in-band).

Greptile's fanout-tie finding is declined as invalid: apply_ordering's
tie-break appends every binding's id column (destination and bound-edge
ids included), so distinguishable fanout rows already order totally, and
indistinguishable rows are content-identical.
@ragnorc
ragnorc force-pushed the search-contracts-p0-p1 branch from a99c153 to 3e459aa Compare September 1, 2026 17:51

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 3e459aa. Configure here.

.iter()
.map(|f| f.name().to_string())
.collect();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aggregate ties sort unsortable columns

Medium Severity

When an aggregate result has no .id columns, apply_ordering now appends every source column as a tie-break. Group keys and return values can be lists, vectors, or other Arrow types that lexsort_to_indices cannot order, so a previously valid order on a scalar aggregate can now fail once a non-sortable column is also returned.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 3e459aa. Configure here.

),
);
}
Some(EmbeddingCoverage { ready, pending })

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coverage ignores full-text scan filters

Medium Severity

Embedding coverage is counted from the scalar filter_expr only. search() / match_text() on the same node scan still constrain the nearest population via full_text_search, so ready/pending and embedding_coverage_pending can describe rows the ranking never considered — including false pending warnings when every full-text match already has a vector.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 3e459aa. Configure here.

@ragnorc ragnorc changed the title feat(query): search plan truth — retrieval IR, projectable metrics, deterministic ties (RFC P0+P1) feat(query): search plan truth — retrieval IR, projectable metrics, deterministic ties Sep 1, 2026
@aaltshuler

aaltshuler commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Closing this PR under the repository governance process, not on the merits of the proposal.

This is a size-L design change: it changes query-language and ranking semantics (fuzzy(), T26, retrieval planning, tie behavior) and adds public read/OpenAPI contracts (warnings, metrics, retrievals, and embedding coverage). Governance defines that class of change as requiring:

issue → accepted → draft RFC → RFC accepted → code PRs

A ready implementation PR must reference an accepted issue or accepted RFC, and a change reclassified upward is closed and restarted on the correct path. See Pull requests and Enforcement.

The dev-graph draft referenced in this PR is useful design work, but it is not the required public repository RFC. The durable proposal must live in docs/rfcs/NNNN-title.md, use track: public, and follow the single public lifecycle described in the RFC guide. A draft RFC may merge for public discussion, but only status: accepted authorizes implementation.

Required path forward:

  1. Open a public Design / RFC proposal issue describing the problem, proposed direction, and blast radius.
  2. Once the issue records acceptance and the needs-rfc status, open an RFC-only PR under docs/rfcs/ with track: public.
  3. Resolve the query/API contracts, compatibility, rollout, alternatives, and evidence gates in that RFC; obtain explicit status: accepted.
  4. Then open focused implementation PRs that reference the accepted RFC.

The existing branch can remain as prototype/evidence for that RFC. This closure is a process reset, not a rejection of the idea.

@aaltshuler aaltshuler closed this Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants