fix(bmw): include non-pivot term max scores in block-skip safety check (#365)

[tjgreen42]

Fixes #365.

Bug

score_segment_multi_term_bmw decided whether to short-circuit a block via block_max_skip_advance using only the pivot terms' block-level upper bound:

block_upper = compute_block_max_at_pivot(terms, pivot_len);
if (block_upper <= threshold) {
    block_max_skip_advance(...);   /* advances one pivot term past current block */
    continue;
}

block_max_skip_advance then advances one of the pivot terms past its current block. But docs in that block may also appear in non-pivot terms' posting lists — when a different pivot iteration later picks those docs up, score_pivot_document only iterates [0..pivot_len), so the contribution from the term that was already advanced past is silently missing from the doc's reported score.

The looser the heap threshold (large K), the more frequently the old check fires for blocks whose docs are still top-K candidates once non-pivot contributions are included — exactly the K-dependent score under-reporting seen on the MS MARCO benchmark.

Fix

Tighten the safe-skip condition to:

block_upper(pivot) + sum(max_score(non-pivot)) <= threshold

This is a true upper bound on any score we could later compute for a doc in the skipped block, no matter which pivot picks it up.

Verification

Local repro on MS MARCO (8.8M docs, pg17, concurrent inserts) — query 1267 doc 3906880:

K	Before	After
10	-23.0376 ✓	-23.0376 ✓
1000	-20.6486 ✗ (off by 2.39)	-23.0376 ✓

Regression tests: bmw, bmw_skip_advance, wand, scoring1..6 all pass.

(max_shared_memory fails locally for me both with and without this patch — unrelated, looks like a DSA-state issue on my long-running pg17 cluster; will verify in CI.)

This is the v1.2 blocker called out in #365.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[claude]

LGTM — small, mathematically sound BMW correctness fix with empirical MS MARCO verification.

Extended reasoning...

Overview

Single-file, ~30-line change to src/scoring/bmw.c that tightens the block-skip safety check in score_segment_multi_term_bmw. The previous condition (block_upper(pivot) <= threshold) ignored that a doc in the skipped block could later be picked up via a different pivot iteration with one term already advanced past — silently under-scoring top-K candidates. The fix adds sum(max_score(non-pivot)) to the upper bound, giving a true safe-skip condition.

Security risks

None. This is a pure scoring-algorithm correctness fix — no auth, IO boundary, untrusted input handling, or memory-management changes. The only behavioral effect is that BMW now skips fewer blocks (more conservative), trading some performance for correctness.

Level of scrutiny

Moderate. BMW is core scoring infrastructure, so changes warrant care, but the diff is small, self-contained, and the math is straightforward to verify: ts->max_score has query_freq applied at init (bmw.c:1049), and compute_block_max_at_pivot independently multiplies by query_freq, so the two addends in block_upper + non_pivot_max are in consistent units. The non-pivot terms have cur_doc_id > pivot_doc_id, so their global max_score is a valid upper bound on any contribution to a hypothetical doc in the pivot's current block, regardless of which of their own blocks contains it.

Other factors

The PR description shows concrete repro evidence (MS MARCO 8.8M doc index, query 1267 doc 3906880): score was off by 2.39 at K=1000 before the patch and exact after. All regression tests (bmw, bmw_skip_advance, wand, scoring1..6) pass. In-code comments thoroughly explain the failure mode and the invariant being restored. No bugs were flagged by the bug-hunting pass.

[tjgreen42]

Benchmark run status

Manually-triggered benchmark on this branch: https://github.com/timescale/pg_textsearch/actions/runs/25713784697

✅ MS MARCO validation — was failing on main, now passes with this fix (the #365 reproducer query 1267 doc 3906880 now scores correctly at all K).

⚠️ Wikipedia validation — fails with score mismatch on 1/80 queries and 0/80 top-10 doc-set matches. After investigating: this is a pre-existing dataset/validator rot (filed as #368), not a regression from this fix.

Briefly: before #360, the MS MARCO validator aborted on an ambiguous query_id error before its DROP FUNCTION cleanup, leaving its validate_single_query overload in bench_test. The Wikipedia validator then collided with that overload at LATERAL validate_single_query(int, text) and aborted too — but the pre-#360 watchdog lacked set -o pipefail and the "VALIDATION PASSED" marker check, so both silently "passed". The hardening in #360 plus the MS MARCO column fix unmasks the Wikipedia GT/index mismatch that's been latent for a while.

See #368 for full root-cause analysis and proposed unblock paths.

CI: all 17 standard checks pass (sanitizers pg17/18, scoring, bmw, wand, bmw_skip_advance, performance, format, etc.).