fix(ci): validate_queries.sql — propagate LIMIT 10 to BMW index scan; document #365 trap

[tjgreen42]

Follow-up to #360, #364. Closes #363 (was a tied-cluster + K=1000 false-positive, not a real concurrent-insert bug).

Root cause

The validation watchdog kept producing non-deterministic concurrent-INSERT-only failures on MS MARCO (queries 1267, 130, 1517, 222, 9351, 9340 across runs), and no amount of investigation could pin them on the index. Turned out the validator itself had this shape:

SELECT
    row_number() OVER (ORDER BY content <@> q)::int as rank,
    passage_id,
    -(content <@> q)::float8 as score
FROM msmarco_passages
ORDER BY content <@> q
LIMIT 10;

Postgres can't push the outer LIMIT 10 past the WindowAgg, so the BM25 index scan is asked for pg_textsearch.default_limit (1000) rows. BMW with K=1000 reports subtly wrong scores for some docs on real corpora — see #365 for the full reproducer and code-level hypotheses. Validation then compares the standalone-recomputed per-rank score against ground truth and sees a mismatch — for ranks whose order BMW got wrong because of the under-reported scores.

Repro evidence

Local pg17 + concurrent-pgbench-built 8.8M MS MARCO corpus, doc 3906880 / query 1267:

	BMW K	BMW score	Standalone score
Query A: SELECT ... ORDER BY <@> LIMIT 10	10	-23.0376 ✅	23.0376
Query B: SELECT row_number() OVER (...), ... FROM ... ORDER BY <@> LIMIT 10	1000	-20.6486 ❌	23.0376
Query B with SET default_limit=10	10	-23.0376 ✅	23.0376

Same query, same data, same backend session. Only K differs.

The fix

Wrap ORDER BY ... LIMIT 10 in a subquery so the LIMIT actually reaches the index scan. row_number() runs on the resulting 10-row materialized output. Output identical to the buggy shape on healthy data, but K=10 is now used for the index scan, so BMW returns correct scores.

SELECT row_number() OVER ()::int as rank, t.passage_id, t.score FROM (
    SELECT passage_id, -(content <@> q)::float8 as score
    FROM msmarco_passages
    ORDER BY content <@> q
    LIMIT 10
) t;

Inline comment in the file explains why this shape matters.

Verification

Local pg17 + concurrent-pgbench 8.8M corpus, run the 80 validation queries:

	Failing queries	Worst diff
Before this fix	9 (1267, 1977, 158, 1522, 222, 807, 9351, 215, 9340)	1.46
After this fix	1 (158, allowlisted in #361)	0.62 (the genuine stemmer-drift)

The 8 queries that disappear were all false positives from BMW K=1000 underscoring. The single remaining failure (158) is the stale-pg17-ground-truth / Snowball stemmer drift case tracked in #361 — regenerating ground_truth_pg17.tsv against current pg17 will close it; that regen is running locally and will be a follow-up commit / PR.

Allowlist update

Allowlist entry for query 158 has its note updated to reflect the now-confirmed Snowball drift root cause (was previously "root cause not yet isolated").

Refs

• fix: sort posting list when spilling memtable (root-cause MS MARCO bucket-8 hang) #360 — segment-sort fix; also added the validation watchdog hardening that surfaced this
• ci(benchmark): validate by per-rank score, not per-doc set #364 — score-rank validation; tie-cluster handling
• validate_queries.sql: MS MARCO query 158 fails by 1.5 score points (pre-existing tokenization discrepancy) #361 — stale ground_truth_pg17.tsv (regen pending)
• validate_queries.sql: MS MARCO query 1267 fails only on concurrent-build path (rank-5 doc missing from top-10) #363 — "concurrent-only failures"; now closeable as a duplicate of BMW reports under-scored results for some docs when K (default_limit) is much larger than the actual LIMIT #365
• BMW reports under-scored results for some docs when K (default_limit) is much larger than the actual LIMIT #365 — BMW K-dependent score under-reporting; the real underlying bug, to be fixed separately

[claude]

LGTM — CI-only validator fix with solid repro evidence.

Extended reasoning...

Overview

This PR modifies a single CI/benchmark validation script (benchmarks/datasets/msmarco/validate_queries.sql) to fix a self-inflicted false-positive in the MS MARCO query watchdog. The previous SQL shape SELECT row_number() OVER (ORDER BY ...) ... ORDER BY ... LIMIT 10 couldn't push the outer LIMIT past the WindowAgg, so the BM25 index scan ran with K=1000 (default_limit) instead of K=10, which exposes the BMW K-dependent score under-reporting tracked in #365. The fix wraps the ORDER BY ... LIMIT 10 in a subquery so the LIMIT actually reaches the index scan, and row_number() runs on the 10-row materialized output. The PR also rewrites the validator to compare per-rank score (rather than per-doc set with tie-boundary heuristics) and updates the #361 allowlist note to reflect the now-confirmed Snowball stemmer drift root cause.

Security risks

None. This is a test/validation SQL script that runs against a benchmark corpus in CI. No auth, crypto, permission, user-input, or network-facing code is touched.

Level of scrutiny

Low. This is CI infrastructure for validating BM25 ranking against a precomputed ground truth file — it does not run in production, does not affect end users, and the worst-case failure mode is a noisy or quiet watchdog. The fix itself is mechanical (subquery wrapper around LIMIT 10) with strong repro evidence in the description (BMW score table for query 1267 / doc 3906880 across K=10 vs K=1000) and verification numbers (9 failing queries → 1 allowlisted) that match the stated mechanism.

Other factors

The only inline comment is a nit flagging that the new per-rank score comparison silently passes empty / partial tapir_results (the INNER JOIN on rank skips unmatched ranks, and docs_match is computed but excluded from the pass predicate). That's a real but minor weakening of defensive coverage in the watchdog and does not affect the validity of this fix — the BMW index returning fewer than 10 rows on the 8.8M-doc msmarco corpus is not currently observed, and the author can address the row-count guard in a follow-up if desired. The fix being applied here resolves a concrete, reproduced false-positive that was masking real signal in CI.

[claude]

🟡 The refactor makes the watchdog silently pass when tapir_results is empty or has fewer rows than ground truth for a query. v_all_match is initialized to true (line 68), the FOR loop's INNER JOIN on t.rank = gt.rank (line 115) skips unmatched ranks, and the pass/fail predicate at line 217 only checks NOT scores_match — docs_match is computed but excluded. Suggest adding a row-count guard before/after the loop (e.g., IF (SELECT count(*) FROM tapir_results) <> (SELECT count(*) FROM ground_truth WHERE query_id = p_query_id) THEN v_all_match := false; END IF;) or including docs_match in the pass predicate.

Extended reasoning...

What the bug is. The new score-validation logic decides pass/fail by comparing scores at each rank via an INNER JOIN between ground_truth and tapir_results on rank. If a future regression causes the BM25 query to return zero or fewer than 10 rows for some query, the missing ranks silently fall out of the join and the query is reported as passing.

Code path. v_all_match is initialized to true at line 68. The FOR loop at lines 106–133 iterates over ground_truth gt JOIN tapir_results t ON t.rank = gt.rank WHERE gt.query_id = p_query_id. Unmatched ranks produce no rows, so the loop body simply doesn't execute for them and v_all_match is never flipped to false. Then scores_match := v_all_match (line 144) returns true. The DO block at line 215–221 uses WHERE NOT scores_match exclusively; docs_match is computed at lines 136–141 but the in-file comment explicitly states it is 'supplementary info (not part of pass/fail).' The failure-detail SELECT (line 252) also filters on NOT scores_match, so an empty/partial result wouldn't even surface in the failure list.

Why existing code doesn't prevent it. The pre-PR code computed v_real_missing from unnest(v_gt_docs) WHERE d NOT IN (SELECT doc_id FROM tapir_results), with a tie-boundary tolerance, and the pass/fail predicate was WHERE NOT scores_match OR NOT docs_match. For the partial-result case (e.g., 5 of 10 returned, with a missing GT doc whose score is well outside any tie boundary), the old check would have caught it. The new code drops docs_match from pass/fail and rank-keys the comparison, so partial results silently pass.

Step-by-step proof (partial case). Suppose for query Q, ground_truth has 10 rows with ranks 1..10 and a future BMW regression causes tapir_results to be inserted with only 5 rows. The inner subquery in the INSERT returns 5 rows, row_number() OVER () assigns ranks 1..5, so tapir_results has rank IN (1,2,3,4,5). Suppose those 5 scores happen to match ground truth within 0.001 tolerance (e.g., the dropped ranks were 6–10 due to a K-propagation bug). The FOR loop iterates exactly 5 times; r.abs_diff <= p_tolerance for every iteration; v_all_match is never set false. scores_match returns true. docs_match is false (different array_aggs), but the DO block at line 217 ignores it. Result: VALIDATION PASSED for a query whose top-10 is half missing.

Step-by-step proof (empty case). If tapir_results ends up empty (e.g., index scan returns no rows for that query), the FOR loop body executes 0 times, v_all_match stays true, scores_match = true, query is counted as passing. (Note: the old code also silently passed the empty case via NULL propagation on v_tapir_min_score, so the strict regression is the partial-result regime — but flagging the empty case is still reasonable since catching it is exactly the watchdog's job.)

Impact and fix. This is CI/validation infrastructure rather than production code, and the triggering precondition (BM25 index returning fewer than the requested K rows on the 8.8M-doc msmarco corpus) is not currently observed — so this is a nit, not a blocker. But it weakens the watchdog's defensive coverage exactly in the partial-result regression class. A minimal fix: add a count guard at the top or bottom of the FOR loop, e.g.:

IF (SELECT count(*) FROM tapir_results) <> (SELECT count(*) FROM ground_truth WHERE query_id = p_query_id) THEN
    v_all_match := false;
    v_details := v_details || 'row count mismatch; ';
END IF;

Alternatively, include docs_match in the pass/fail predicate at line 217 (WHERE NOT scores_match OR NOT docs_match) — though that would re-introduce tied-cluster false positives the PR intentionally removes, so the row-count guard is the cleaner option.