perf(bm25): update the index incrementally instead of rebuilding it (#1199) - #1223
Conversation
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
Reviewer's GuideImplements incremental BM25 index updates that reuse unchanged document rows based on source fingerprints instead of rebuilding the full index on every mutation, updates the sidecar cache to leverage stale indexes for incremental rebuilds, and extends serialization to carry the necessary fingerprints, with a comprehensive test suite validating bit‑for‑bit identity with full rebuilds and correct cache wiring. Sequence diagram for BM25 sidecar incremental update vs full rebuildsequenceDiagram
actor RetrievalCaller
participant BM25IndexCache as BM25IndexCache
participant MemoryStore as MemoryStore
participant BM25Index as BM25Index
RetrievalCaller->>BM25IndexCache: get()
alt cached index present
BM25IndexCache-->>RetrievalCaller: return _index
else no cached index
BM25IndexCache->>MemoryStore: store_generation()
BM25IndexCache->>BM25IndexCache: _load_sidecar(require_fresh=False)
BM25IndexCache-->>BM25Index: stale_sidecar
BM25Index->>BM25Index: update_from(base, store,...)
alt update_from returns index
BM25Index-->>BM25IndexCache: updated_index
else update_from returns None
BM25IndexCache->>BM25Index: build(store,...)
BM25Index-->>BM25IndexCache: rebuilt_index
end
BM25IndexCache->>BM25IndexCache: _write_sidecar(index, generation)
BM25IndexCache-->>RetrievalCaller: return index
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[claim:review:garsecg:2026-07-30T22:13:11Z] |
|
Reviewed by re-deriving the correctness claim independently rather than reading the equality table. No blocking findings. Approving via The claim that matters here is not the 4x — it is that Independent identity probe — 32/32I wrote my own harness rather than trusting the shipped churn walk, comparing
All 32 took the incremental path (my harness reports a decline separately, so an "OK" produced by falling back to a rebuild would have been visible). The empty-string and punctuation-only cases were the ones I expected to break something — a document that tokenises to nothing still has to hold a row and a The shipped tests have teeth — checked by mutation, not assumedTwo mutations applied to the shipped source and reverted:
And the churn walk chains generations ( Points I checked specifically because they could fail silently
One thing worth keeping visible, not a change requestThe honest framing of the 4x — that the remaining ~300 ms is the change-detection read, and going below it needs a per-row change signal the schema does not have — is the most useful paragraph in the PR body, and it belongs in an issue rather than only here. Deferring that schema change on the strength of #1161 is the right read of the risk. If it is not already filed, it should be, so the ceiling is recorded where the next person profiling this will look. Adding |
|
[release:review:garsecg:2026-07-30T22:23:06Z] |
|
merge-train: blocked branch is not fast-forward on The |
Prerequisite for the incremental update (#1199): without a per-document record of what was indexed, a loaded sidecar cannot say which rows are still valid, and the only safe move is to re-tokenise everything. Fingerprints the text that was actually tokenised rather than reusing `beliefs.content_hash`. That column is written by callers and is not enforced by the store, so keying index validity on it would let one writer's wrong hash silently serve stale retrieval results. Anchor fingerprints sort before hashing: anchor order feeds per-term counts and a total length, both commutative, so hashing in iteration order would invalidate documents whose index is provably identical. Serialisation goes to v5, flagged rather than mandatory so a hand-built index still round-trips and scores. No scoring path reads these.
#1199 AC1 measured 86.2% of retrieval-running prompts rebuilding the index from scratch, 96% of that cost in build() re-tokenising ~45k documents to absorb a change ratio under 0.1%. update_from() reuses the rows whose fingerprint is unchanged and tokenises only the rest. On the real 45,931-belief store: 1.5s -> 0.4s, about 4x, flat in the size of the change set because the remaining cost is the fixed change-detection scan (178ms SQL + fingerprints), not the tokenisation it replaces. Identity, not approximation. Every value is derived the way build() derives it rather than patched forward: dl is copied for reused rows so no float re-rounding creeps in, df is counted off the assembled sparsity pattern rather than carried and adjusted, and idf/avgdl are computed from those at the end. Verified field-for-field on the production store and over randomised churn in both scoring modes. Declines to None -- caller builds as before -- when the base has no fingerprints, when anchor_weight or per_field differ so its rows describe different documents, when either side is empty, or past a 50% change ratio where bookkeeping costs more than it saves.
|
Merge-train bounced this on a stale base, not on the review: |
Carries the AC1 measurement that motivated it, including why the headline is 86.2% rather than the 27.3% an all-fires denominator would give, and states the identity guarantee and the conditions under which the path declines.
|
Rebased onto current The Still needs a reviewer — I authored it, so I can't self-review or merge. |
6bdb7fd to
bed0f6f
Compare
|
[claim:review:Toug:2026-07-30T22:57:22Z] |
|
Approving. I verified the bit-exactness claim independently against the live store rather than reading the table, added two mutations the PR did not try, and checked the one interaction I thought most likely to be missed. Everything held. Bit-exactness, re-measured on the live store (44,584 docs, 21,147 terms)Both scoring modes, four scenarios each, comparing
I constructed the third case deliberately. Worth a note for whoever touches it next: that correctness rests on an invariant nothing asserts. If a future change ever stores an explicit zero in either stream, Mutations, mine rather than the PR's
The first is the one that matters: an anchor-text edit that leaves content untouched is exactly the change a content-keyed fingerprint would miss, and it would surface as a stale index with no symptom. The churn walk catches it in both modes. The interaction I went looking for
It is taken from the call, and the comment says why. Verified behaviourally rather than by reading: Also confirmed: all four decline paths return Perf, independentlyBest of 3 on my copy of the live store, single-field: 882 ms full build → 197 ms Deferring the per-row Also checked
Nothing blocking. The |
|
[release:review:Toug:2026-07-30T23:03:45Z] |
|
Rebase verified: FF on |
|
merge-train: merged bed0f6f → |
Closes #1199.
AC1 was measured and posted on the issue; the operator ruled option 2,
incremental — target
build(), not serialisation (2026-07-30). This is that.What the measurement said
597 real
user_prompt_submitfires, 60 sessions, 2026-06-29 → 2026-07-30, onthe 45,931-belief ambient store:
(163/189); the median session rebuilt on every such prompt.
retrieval and never ask for an index — that denominator credits the sidecar
for work it never did.
do not overlap (warm tops out at 411 ms, cold starts at ≥1000 ms, zero fires
between).
BM25Index.build()— 1113 ms of tokenising andstemming against 44 ms serialise, 2 ms disk read, 52 ms deserialise.
bumps
store_generationin the same transaction.Result
On that store,
update_fromagainst a stale sidecar:Flat in the size of the change set, because what is left is the fixed
change-detection scan, not the tokenisation it replaces. Profiled, that
remainder is ~178 ms of
fetchalland ~120 ms of fingerprinting — 4x, notthe 20x the 96% figure might suggest, and the honest reason is that
detecting which documents changed still costs a read of every document.
Going below that needs a trusted per-row change signal, which the schema does
not currently have; see the note at the end.
Two design decisions worth reviewing against intent
Change detection does not use
beliefs.content_hash. That column iswritten by callers (
classification._content_hash,derivation._content_hash)and is not enforced by the store. Keying index validity on it would mean one
writer's wrong hash silently serves stale retrieval results — a correctness
bug with no symptom. The sidecar carries a digest of the exact text that was
tokenised instead. It costs the
SELECT id, contentthe build already does andowes nothing to a convention the store does not guarantee.
Anchor fingerprints sort before hashing. Anchor order feeds per-term counts
and a total length, both commutative, so hashing in iteration order would
invalidate documents whose index is provably identical.
AC3 — identity, not approximation
The invariant from #1135 is that retrieval output is a deterministic function
of store content, so "almost the same index" would be a correctness regression
wearing a performance improvement's clothes. Every value is derived the way
build()derives it rather than patched forward:dlis copied for reused rows, so no float re-rounding can creep in.dfis counted off the assembled sparsity pattern, not carried andadjusted, so a mis-tracked increment cannot accumulate across generations.
idfandavgdlare computed from those at the end, by the sameexpressions.
n_termsandevery column index after them drift from a fresh build.
Verified on the production store, field for field:
test_incremental_matches_a_full_rebuild_under_random_churnis thedistinguishing assert: 60 random mutations — inserts, content edits,
soft-deletes, hard-deletes, anchor-edge churn, and posterior bumps that change
no indexed text — asserting exact equality after each, in both scoring modes.
The test suite was checked for vacuity rather than assumed. The walk
exercises the incremental path 60/60 steps in both modes (pinned by an
assertion, since a version that declined every step would otherwise compare a
rebuild against itself). Two mutations were applied to the shipped source and
reverted:
dlfor reused rows instead of copyingThe first initially escaped the walk, because a 16-word shared vocabulary can
never orphan a term — the corpus generator now emits a document-unique rare
token half the time, and the mutation is caught. That is recorded in the
generator's docstring so it does not get "simplified" away.
test_cache_serves_a_stale_sidecar_incrementallypins the wiring separately byreplacing
BM25Index.buildwith a bomb, so the only wayget()can return isthe incremental path; it has a negative control asserting a build still happens
when there is no sidecar to update from.
Where it declines
Returns None and the caller builds exactly as before: no fingerprints on the
base, a different
anchor_weightorper_field(its rows describe differentdocuments), an empty side, or more than a 50% change ratio, where the
bookkeeping costs more than the tokenisation it saves.
Serialisation goes to v5 for the fingerprints — one rebuild per store, the same
posture as v3 and v4. A blob without them still round-trips and scores; it just
cannot seed an update.
Not in scope
The ~300 ms floor is the change-detection read. A per-row generation or
indexed_atcolumn would let the query skip unchanged rows entirely and takethis toward the ~53 ms sidecar-hit cost — but that is a schema migration on the
hot table, and #1161 is a live reminder of what a bad one-shot migration does
to this store. Worth its own issue with its own gate rather than riding along
here.
Full suite: 6487 passed, 69 skipped, 71 xfailed.
Summary by Sourcery
Introduce incremental BM25F index updates that reuse unchanged rows from a stale sidecar instead of rebuilding the full index on most retrievals, while preserving bit-for-bit equality with a full rebuild.
New Features:
Enhancements:
Documentation:
Tests: