Retire the in-process BM25 text stack; tests and evals measure the store-native FTS - #527
Merged
Conversation
zzet
force-pushed
the
chore/drop-code-orphaned-by-sqlite-only
branch
from
August 14, 2026 14:11
95b0365 to
7e44615
Compare
zzet
changed the base branch from
chore/drop-code-orphaned-by-sqlite-only
to
main
August 14, 2026 16:09
…xture The tests that pin what indexing makes searchable ran against an in-memory graph and an in-process text index the daemon never builds, so they proved nothing about the corpus production serves. They now index into a sqlite store and read its native symbol FTS through the SymbolSearcherBackend, and each fixture refuses to run on an empty corpus. Queries that would be answered by the exact-name short-circuit were reshaped so the ranked tier decides the outcome.
…backend These fixtures test MCP-layer logic — post-filter escalation, soup split/merge, equivalence rewrite, over-fetch retention — given a candidate ordering, not the ranker that produced it. Replace the hand-tuned text corpora with an orderedBackend that answers a token with a literal list of node IDs and records every Search limit, so the premise each test rests on is stated instead of emergent.
… text backend Production always runs on sqlite, whose native FTS already answers search through SymbolSearcherBackend, so the in-process index the other arm built was a second copy of a corpus nobody queried. NullBackend indexes nothing and reports an empty corpus, which routes the query engine to the substring fallback it already takes for an Engine with no search backend at all. The remaining test construction sites move onto it; the two that still exercise BM25 itself keep their old backend.
stdbench indexed its corpus into an in-process BM25 index and claimed to measure what search_symbols runs; that stopped being true when search moved to the store's native FTS, so the reported numbers described a retriever no user queries. It now opens a throwaway sqlite store, writes the corpus through BulkUpsertSymbolFTS with search.Tokenize-split tokens (mirroring the indexer's write side), asserts every document reached the index, and ranks with SearchSymbols. The corpus never goes through search.Backend because the shipping adapter's Add is a no-op and would silently score an empty index.
…backend BM25Ranker only ever adapted a raw search.Backend, and its sole caller was its own unit test — the shipping eval path registers the "bm25" row through EngineRanker over Engine.SearchSymbols, which reads the store's native symbol FTS. Dropping it removes the last tie from this package to the retired in-process text stack. The "bm25" row key stays put so bench artifacts remain joinable; only the prose that mislabelled it changes.
…tive FTS Every store now yields either a SymbolSearcherBackend or a NullBackend, so the branches that fed an in-process text index have no live path: drop the backend-shape probe, the ngram-boundary install, and the bulk Add loop. buildSearchIndex is now vector-only and returns immediately without an embedder, and the two rebuilds that existed only to bootstrap a non-persistent backend go with it. Incremental Add/Remove stay put — the native adapter keeps its document counter that way.
…s arms Status resolution no longer recognises the in-process text backend, which the indexer stopped building. In its place the null backend gets its own arm reporting "none" with zero counts, so a store without native symbol search reads as "nothing indexes text here" rather than falling through to "unknown", which means "we could not identify the backend". The status protocol doc lists the names the field can now carry.
The bigram typo-rescue branch type-asserted the active text backend to an unexported bigramProvider interface that no wired backend satisfies: the method exists only on the BM25 backend, and neither the swappable wrapper nor the hybrid backend forwards it, so production's backend never matched. Delete the tier, its interface declaration, and the camelCase-boundary helper that only gated it. Behaviour-neutral: the branch was unreachable.
Search has run on the graph store's native symbol FTS since the indexer stopped building a parallel corpus, so the in-process BM25 index, its bigram typo rescue and the sparse-ngram tokenizer had no caller left. Delete them with the tests that only exercised them, move the hybrid de-chunk fixtures onto a map-backed text double and the path-scoping fixtures onto the ordered backend, and retarget the prose that still described the retired stack at the FTS adapter.
…g sweep
NullBackend was a field-less struct, so every escaping &NullBackend{}
pointed at runtime.zerobase and NewNull() == NewNull() held on any
production path. That made the type's own doc claim — a distinct
pointer per call, so identity comparisons behave as for every other
backend — false, and it quietly defeated Swappable.Swap's close gate:
a null-to-null swap took the "same backend, skip Close" arm instead of
the "displaced it, close it" arm. Harmless only because Close is a
no-op.
Fixed by giving the struct one unnamed byte rather than by returning a
package-level singleton. Swap's gate is an identity test that has to
separate "I installed something new" from "I re-installed what was
already here", and it only stays a real test if independently
constructed backends are independent objects. A singleton would make
the gate structurally unable to see a null-to-null replacement, and
would alias one instance across every Swappable in the process — safe
exactly as long as Close and the struct stay empty, and a leak the day
either stops being. One byte at wiring time buys the language's own
guarantee of distinct addresses instead.
TestNewNull_DistinctInstances passed only because inlining kept the two
values in distinct stack slots; it guarded nothing. It is replaced by a
test of the invariant it was standing in for: Swap closes the backend it
displaces, leaves a re-installed backend open, and takes the first arm
for two null backends. The implements-Backend-only assertions stay, with
the interface check lifted to a compile-time assertion.
Renderer fixtures that still carried a "bm25" backend name now use
"unknown" — the live name whose arm actually reports a heap figure,
since resolveSearchBackend emits only sqlite-fts5, none or unknown and
only the last carries bytes. Coverage is unchanged: the renderer prints
whatever name it is handed and branches on DiskResident alone. The
indexer's embed-cap warning, the skip_search config comment, the
SymbolSearcher contract doc and the FTS5 capability assertions no longer
describe an in-process text index as the live alternative.
Two families of reference remain and are deliberate: the eval "bm25" row
key, kept so historical bench artifacts stay joinable and already
annotated as involving no in-process index, and the search_symbols
timing fields and log keys, which measure the ranked lexical channel
that FTS5 still serves through its own bm25() function.
zzet
force-pushed
the
refactor/retire-in-process-bm25
branch
from
August 14, 2026 20:21
a8898f4 to
d1ae1a2
Compare
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
1 participant
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
Retires the in-process BM25 text stack —
bm25.go,bigram.go,sparse_ngram.go,ngram_weights.go(~960 LOC plus their dedicated tests) — by actualizing every test and eval consumer onto what production actually runs: the store-native SQLite FTS. Ten commits; the tree is green at every one.Stacked on the dead-code-sweep branch (#526); merges after it.
How the consumers were actualized (not just swept)
SymbolFTSCount() > 0gates), and every converted query was proven to fail when misspelled — no vacuous passes. Where the store's Tier-0 exact-name short-circuit would have answered from the graph instead of the ranked FTS tier, the fixtures use multi-word probes with the reasoning recorded in test comments.search.NewNull()— a backend implementing nothing beyond the interface, so the engine's corpus check routes to its existing substring fallback. TheNewAutoseam is deleted outright.BulkUpsertSymbolFTS, hard-error if the FTS row count mismatches the corpus; retrieval viaSearchSymbols), with pinned Recall/MRR values that mutation-probes verified are discriminating.BM25Rankerwrapper is gone; the productionbm25row was already served byEngineRankerover the store FTS adapter, so no published number moves. The row key staysbm25for artifact joinability; the prose stops lying about what it measures.Behavior notes a reviewer should not have to reverse-engineer
BigramCandidatesexisted only on the BM25 backend, nothing that reaches the engine forwards it, and its env gate was never set on a wired path."none"search-backend status arm is production-unreachable (serverstack opens sqlite only) and exists so a null backend reports honestly instead of"unknown".concept-ranker-bm25and its typo mirror) gold-answered the deletedBM25Rankersymbol and were retargeted at the liveEngineRanker; case IDs and count (156) are unchanged, so per-case history joins — but those two cases' difficulty drifted slightly at this PR.GORTEX_BIGRAM_TYPOSandGORTEX_SPARSE_NGRAMdisappear with the code that read them; both were documented but dead on every wired path (docs updated in the same commits).Measured cost
go test -raceper-package deltas vs the pre-stack base, same machine, same flags:internal/mcp+12.4s (+11.2%),internal/indexer+12.5s (+6.5%) — combined +24.8s, under the design's +33s estimate. The stub conversions are net-negative (each drops an in-memory corpus build); deleting 23 BM25 tests recovers more.Kept, deliberately
TokenizeandNormalizeFTSTokens(live in the FTS write/read paths),HybridBackend/Swappable/VectorBackend(not retired), thego-porterstemmerdependency (serves the live FTS normalizer), and generic "BM25" wording where it is still true — SQLite FTS5 genuinely ranks withbm25().Open corpus decision (deliberately not made here)
Five recall-fixture case groups gold-answer the deleted
internal/search/bm25.goitself (exact-BM25Backend,exact-NewBM25,concept-bm25-index,mh-bm25-api, and one ofmh-search-backends' five IDs, plus typo twins). Four of the five are unfixable by retargeting — their query text is the deleted API — so "fixing" them means deleting case groups and shifting the 156-case corpus and its tier distribution. That is a bench-comparability decision for the corpus owner; until then those cases warn and score as misses, exactly as they began doing at the deletion commit.Validation
go test -race ./...green at every commit and on the final head;golangci-lint0 issues; fence test green; the forbidden-symbol grep (NewBM25|BM25Backend|SparseNgram|NgramTable|BigramIndex) returns nothing.cmd/gortex's user-state guard trips on the pre-existinginternal/mcpquery-log leak (internal/mcp tests write the real ~/.gortex/cache/query-log.jsonl, tripping cmd/gortex's user-state guard in multi-package runs #518) — reproduced identically on the base commit;GORTEX_QUERY_LOG_DISABLE=1confirms. Every individual test passes.