index: make the BM25 MaxScore bound authority-aware (fixes k-dependent ranking) - #54
Merged
Merged
Conversation
BM25 ranking depended on k: 48 of 60 golden queries returned a different top-10 at k=10 than at k=50 on prod. The scan terminated when remainingMax < theta, where theta is the k-th largest raw score. The final ranking is raw * Multiplier(host), applied later at metadata-resolution time because the multiplier needs the document URL, so an unseen document's real ceiling is remainingMax * maxMult. With the default alpha of 2.0 the bound was 3x too loose and dropped genuine top-k members. An unknown host scores 0.5, i.e. a 2x multiplier, so this was the common case rather than a tail case. resolveTopKPool already carried the equivalent bound on the candidate side and needed no change; the tracker's note to apply it there too was mistaken. Phrase queries now opt out of MaxScore entirely: theta cannot threshold a phrase-filtered subset, so early termination could return empty. No test covered MaxScore and authority together, which is the gap this shipped through: one test exercised MaxScore with a nil scorer and compared only the first hit, the other attached a scorer but disabled MaxScore. Adds that case asserting full top-k membership, a k-independence pin, and an alpha table covering alpha=0 where the new bound must reduce to the old one. Corrects two false comments: top-k membership is approximate, not lossless (break also freezes documents already in scores -- deferred, tracked), and boost seeding rescues only zero-overlap documents in boost sets of 256 or fewer. Measured against a lossless oracle (COSIFT_BM25_DISABLE_MAXSCORE=1, exact by construction) on a frozen 3,070-document store: overlap@10 versus ground truth 0.8362 -> 0.9793, rank displacement 0.844 -> 0.135, worst score ratio 0.5119 -> 0.7637. One query of 58 regressed, 0.80 -> 0.70. No latency change on the shared benchmark: pool +0.46%, resolve-all -0.86%, both within noise. The prod k=10 versus k=50 verdict and the POOL_FACTOR reassessment still need a measurement window on the box.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
The prod A/B for the MaxScore fix came back negative: k-dependence was unchanged (51/60 goldens before, 53/59 after) and p50 at k=10 went 56ms -> 184ms. Measuring why found the real cause, and it is not MaxScore. poolCap = factor*k makes the resolved candidate set proportional to k. When the cap binds, k=10 and k=50 sample the same candidates to different depths and cannot agree. From the cap-bind log at factor 200: a median bound query resolved 2,000 of ~68,500 in-band candidates, under 3%; the worst resolved 2,000 of 403,943. At k=50 the pool is 10,000 -- a 5x deeper look at the same set. That difference is the k-dependence. Cap-bind events were essentially unchanged across the two arms (15 vs 13 over comparable windows), which is what you would expect if the two approximations are independent -- they are. This contradicts the 2026-09-12 reading that "MaxScore dominates": that came from raising the factor 50 -> 200 and seeing little movement, which shows raising the factor does not help, not that the pool path is innocent. Adds COSIFT_BM25_TOPK_POOL_MIN, an absolute floor on the pool. Default 0 keeps today's behaviour exactly, so this is inert until configured. Also gates the MaxScore authority bound behind COSIFT_BM25_MAXSCORE_AUTHORITY (default on, =0 selects the old bound) so the two changes can be measured independently on the box without rebuilding. TestPebbleBM25PoolMinMakesTopKIndependentOfK reproduces the defect on a 400-doc fixture and pins the fix. It fails first if the fixture stops reproducing k-dependence, so it cannot go vacuous. Mutation: making bm25TopKPoolCap ignore the floor fails it with "top-10 still depends on k with the pool floor set". The floor's production value is not settled -- a deeper pool costs resolution work, and that trade is the next measurement.
Production measurement of the T0.5 branch came back negative, and finding out why turned up a second, larger defect that no BM25 change can reach. Writing it down so the next attempt does not repeat this one. Findings, all measured on the box at 15.7M docs: Time decay is the bigger defect. /search applies decay after retrieval over a k-sized candidate list, so a deeper fetch lets decay promote different documents into the top 10. On one binary at one moment: 50/59 goldens disagree with decay on, 0/59 with decay=0. On stock v0.2.5 it is 51/60 vs 19/60. Not fixed here, and not fixable from internal/index. BM25 is genuinely k-dependent underneath, 19/60, because two decisions branch on the caller's k: the MaxScore theta and the resolution pool. Flooring both at kEff takes it to 0/59. Depth 15 is the knee: mean overlap@10 vs the k=50 reference goes 0.9450 -> 0.9817 for +31% p50 and +9% p90, and it is where the tracker's example query flips from 0.20 to 1.00. Depth 50 costs 147ms p50 for no further accuracy. Also records why the first two attempts failed -- the authority-aware bound left theta k-shaped and cost 3.3x p50 for no symptom improvement, and flooring the pool alone left theta k-shaped too -- plus the /search fetchK widening rules, which anyone reasoning about "the k the index sees" needs first. No production defaults change: COSIFT_BM25_RANK_DEPTH is 0 unless set.
andreimarinescu
marked this pull request as draft
September 15, 2026 17:43
TeoSlayer
marked this pull request as ready for review
September 16, 2026 14:45
This was referenced Sep 16, 2026
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
2 participants
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.
BM25 ranking currently depends on
k: the same query returns a different top-10 atk=10than atk=50. Measured on production 2026-09-12: 48 of 60 golden queries differ. RaisingCOSIFT_BM25_TOPK_POOL_FACTORfrom 50 to 200 (which prod runs today, at a +25–35 ms p50 cost) onlymoved that to 46/60 — so the pool cap is the smaller component and MaxScore dominates.
Root cause
pebble_bm25.go:240-245terminates the posting scan whenremainingMax < theta, wherethetaisthe k-th largest raw score. But the final ranking is
raw × Multiplier(host), applied later atmetadata-resolution time (
:305-311,:376-378) because the multiplier needs the document's URL.Since
Multiplier ∈ [1, 1+alpha], an unseen document's final ceiling isremainingMax × maxMult,not
remainingMax. The correct bound is:The shipped code is the
maxMult == 1special case. With the defaultalpha = 2.0the bound is3× too loose, so genuine top-k members are dropped. This is the common case rather than a tail
case:
Scorer.Scorereturns0.5for an unknown host (authority.go:104-110), putting unknowns at2.0×.resolveTopKPoolwas already correct —:399readspool[i].score*maxMult < kthRaw, thealgebraically equivalent form on the candidate side. The tracker's note to apply "the same bound in
resolveTopKPool" was based on a wrong assumption; the pool path needed no change. Recorded here sonobody re-opens it.
Why no test caught this
TestPebbleBM25MaxScorePreservesTopKMembership:143exercises MaxScore but builds the index with ahelper whose
authorityis nil, and it only compareshits[0].URL.TestPebbleBM25AuthorityReorderWithinPool:441attaches a real scorer but setsCOSIFT_BM25_DISABLE_MAXSCORE=1at:443. Nothing exercised MaxScore and authority together —exactly the gap this bug shipped through.
Changes
maxAuthorityMult()helper; the scan bound becomesremainingMax*maxMult < theta.thetacannotthreshold a phrase-filtered subset, so a phrase query could terminate early and return empty.
:218-219. It claimed "Top-K membership stays lossless", which is falseand is not made true by this fix — see the deferred defect below. It now states what is
actually guaranteed.
:276-277that boost seeding "closes the MaxScoreearly-termination gap". It only rescues zero-overlap boosted docs, and only for boost sets ≤ 256.
alpha table covering
alpha = 0(where the new bound must reduce exactly to the old one).Measurement
The assumption going in was that a 3,070-document local rig could not measure this. That was
wrong, and the distinction matters: the pool cap does need production scale, but the MaxScore
early-termination bound fires on any multi-term query regardless of corpus size.
Ground truth is the same binary run with
COSIFT_BM25_DISABLE_MAXSCORE=1— every posting listscanned, so the ranking is exact by construction. Both arms scored against it, 60 golden queries at
k=10 and k=50 on a frozen store:
Both inside run-to-run noise. This does not settle the production latency question. The fix
prunes less, and the concern is p50 at 15.7M documents under
POOL_FACTOR=200, where these queriesrun 58–60 ms rather than the 2–14 ms seen locally. If it proves over budget, the tracker's
instruction is to lower
alphafrom 2.0 rather than revert.Gate
In
golang:1.25-bookworm(go1.25.11, matching CI):gofmtclean,go vetclean, arm64cross-compile ok,
go test -race -timeout 10m ./...green.Reviewed by two independent adversarial reviewers plus a verifier that mutation-tested every new
test and re-ran the gate independently.
Deliberately not fixed
breakalso freezes documents already inscores. The comment's reasoning is sound only fordocuments not yet in
scores; becausebreakexits the whole term loop, a document alreadypresent with a partial
s < thetaalso stops accumulating, and its true score can approach2·theta.Making membership genuinely lossless needs the classical essential/non-essential term split, which
carries real latency risk and cannot be measured at local scale. Operator ruling: authority gap
only. Filed to
cosift-engine-open-items.The residual 0.9793 above is this defect, plus boost seeding (
boostIDsmultipliers up to 50× areapplied entirely outside the MaxScore bound) — the worst remaining cases are
site=arxiv.org diffusion modelsandMuse Glimmer open-weight model, both boost queries. Neitheris a regression; both are pre-existing and now quantified.
Before merge
Per
CONTRIBUTING.md's eval gate, this needs the production measurement: goldens at k=10 and k=50on the box, control arm vs this binary. Local evidence is strong but the k-dependence headline
(48/60) is a production number and should be re-measured against a production arm. After that
lands,
COSIFT_BM25_TOPK_POOL_FACTORshould be reassessed — it was raised to 200 specifically tokeep the pool lossless until this shipped, and it costs p50.