diff --git a/server/internal/chunksfts/chunksfts.go b/server/internal/chunksfts/chunksfts.go index 618d659..cf662f0 100644 --- a/server/internal/chunksfts/chunksfts.go +++ b/server/internal/chunksfts/chunksfts.go @@ -28,6 +28,7 @@ import ( "context" "database/sql" "fmt" + "sort" "strings" ) @@ -255,11 +256,24 @@ func SearchProject(ctx context.Context, db *sql.DB, projectPath, query string, l return out, nil } -// searchProjectsBatch caps how many project_path values go into one IN -// list. SQLite's default bound-variable ceiling is 999; 500 leaves room -// for the query and limit parameters and matches the batch size the -// vector store already uses for its own IN lists. -const searchProjectsBatch = 500 +// searchProjectsBatch caps how many project_path values go into one IN list, +// and payloadFetchBatch does the same for the rowid list of the second +// statement. +// +// 500 is NOT a headroom number. Measured through this driver, `rowid IN (...)` +// takes 32,766 placeholders and fails at 32,767 — SQLite raised +// SQLITE_MAX_VARIABLE_NUMBER from 999 to 32,766 in 3.32.0 and modernc tracks a +// recent upstream, so the ceiling is two orders of magnitude away and neither +// constant is anywhere near it. The reason for 500 is consistency: it is +// hydrateBatch (internal/vectorstore/search.go), the batch size the vector +// store already uses for its own IN lists, and one number for both is worth +// more than a tuned one for each. Raising it would want a measurement, and +// 43 x 50 = 2,150 rowids in five statements is nothing against a multi-second +// BM25 scan, so there is nothing to gain by measuring. +const ( + searchProjectsBatch = 500 + payloadFetchBatch = 500 +) // SearchProjects answers the same question as SearchProject for many // projects at once, returning each project's top `perProject` hits keyed @@ -277,19 +291,22 @@ const searchProjectsBatch = 500 // fan-out's total work, and each query slowed from ~400 ms standalone to // as much as 7 s when 43 of them ran against the index at once. // -// The window function does the partitioning SQLite would otherwise make -// us do with N queries: rank within each project, keep the top rows of -// each. +// The per-project trim is a bounded heap in Go, not a window function in +// SQL: a window has to sort the ENTIRE match set to find N rows per +// project, and the match set here is the whole server's index. See +// workspaceScanQuery for why that is the dominant cost and what it +// measured. // -// Both forms order by (bm ASC, rowid ASC). The rowid is defensive, not a -// fix for an observed bug: bm25 ties are the norm rather than the -// exception in a trigram index over real code — in this package's own -// test corpus 14 of 16 hits share a score with another hit — and today -// SQLite happens to return tied rows in rowid order for both the LIMIT -// and the window form, so they agree without being told to. That is -// unspecified behaviour of the sorter. Naming the tiebreak makes the -// agreement a property of the queries instead of a coincidence that a -// future planner is free to break. +// Both paths order by (bm ASC, rowid ASC) — the per-project query in its +// ORDER BY, the workspace path in rankedRow.betterThan. The rowid is +// defensive, not a fix for an observed bug: bm25 ties are the norm rather +// than the exception in a trigram index over real code — in this package's +// own test corpus 14 of 16 hits share a score with another hit — and today +// SQLite happens to return tied rows in rowid order for the LIMIT form, so +// the two would agree without being told to. That is unspecified behaviour +// of the sorter. Naming the tiebreak on BOTH sides makes the agreement a +// property of the code instead of a coincidence a future planner is free +// to break. func SearchProjects(ctx context.Context, db *sql.DB, projectPaths []string, query string, perProject int) (map[string][]Hit, error) { if perProject <= 0 { perProject = 20 @@ -315,74 +332,270 @@ func SearchProjects(ctx context.Context, db *sql.DB, projectPaths []string, quer func searchProjectsBatchInto(ctx context.Context, db *sql.DB, projectPaths []string, fts5Q string, perProject int, dst map[string][]Hit) error { - args := make([]any, 0, len(projectPaths)+2) + args := make([]any, 0, len(projectPaths)+1) args = append(args, fts5Q) for _, pp := range projectPaths { args = append(args, pp) } - args = append(args, perProject) - rows, err := db.QueryContext(ctx, workspaceRankQuery(placeholders(len(projectPaths))), args...) + rows, err := db.QueryContext(ctx, workspaceScanQuery(placeholders(len(projectPaths))), args...) if err != nil { return fmt.Errorf("chunks_fts workspace search: %w", err) } - defer rows.Close() + tops := make(map[string]*topHits, len(projectPaths)) for rows.Next() { var pp string - h, err := scanRankedHit(rows, &pp) - if err != nil { - return err + var r rankedRow + if err := rows.Scan(&pp, &r.rid, &r.bm); err != nil { + rows.Close() + return fmt.Errorf("scan chunks_fts ranking row: %w", err) + } + t := tops[pp] + if t == nil { + t = &topHits{n: perProject} + tops[pp] = t } - dst[pp] = append(dst[pp], h) + t.offer(r) } - if err := rows.Err(); err != nil { + err = rows.Err() + rows.Close() + if err != nil { return fmt.Errorf("iterate chunks_fts: %w", err) } + + ranked := make(map[string][]rankedRow, len(tops)) + var rids []int64 + for pp, t := range tops { + ordered := t.sorted() + ranked[pp] = ordered + for _, r := range ordered { + rids = append(rids, r.rid) + } + } + // Sorted because rids was built by ranging a map, so without this the IN + // lists — and therefore the batch boundaries — differ between two runs of + // the same query. The results do not (payload is keyed by rowid and each + // project is assembled in rank order), but a statement whose bound + // parameters come out of Go's map iteration cannot be compared plan-to-plan + // between runs, which is exactly what someone timing this will want to do. + // Ascending rowids also probe both B-trees in order rather than at random. + // Not measured as a speedup; the reason to do it is the determinism. + sort.Slice(rids, func(i, j int) bool { return rids[i] < rids[j] }) + + payload, err := fetchPayload(ctx, db, rids) + if err != nil { + return err + } + collectHits(ranked, payload, dst) return nil } -// workspaceRankQuery builds the partitioned statement for a placeholder list. +// collectHits pairs each project's ranked rows with the payload fetched for +// them, and writes the projects that still have hits into dst. +// +// A row whose chunk vanished between the ranking scan and the payload fetch is +// dropped. Two statements cannot be atomic the way the one they replaced was, +// and a hit fewer is the right answer for a chunk that no longer exists; +// erroring would fail a whole workspace search because one file happened to be +// getting reindexed. A project that loses ALL of its survivors that way is left +// out of dst entirely, because this package's contract is that a project with +// no match is absent rather than present with an empty slice. +// +// What makes the split safe is not in this package: a rowid can only go +// MISSING, never come back pointing at a different chunk. chunks_meta.rowid is +// INTEGER PRIMARY KEY AUTOINCREMENT (internal/db/schema.go), so SQLite never +// re-issues a rowid after a delete. Drop the AUTOINCREMENT and a reindex could +// hand project B's chunk back under project A's score, with no error and +// nothing in failed_repos — at which point this needs cm.project_path in the +// payload SELECT and a check against the ranked row. +func collectHits(ranked map[string][]rankedRow, payload map[int64]Hit, dst map[string][]Hit) { + for pp, rows := range ranked { + hits := make([]Hit, 0, len(rows)) + for _, r := range rows { + h, ok := payload[r.rid] + if !ok { + continue + } + h.Score = -r.bm + hits = append(hits, h) + } + if len(hits) > 0 { + dst[pp] = hits + } + } +} + +// rankedRow is a matched chunk before its payload is fetched: the two columns +// the ranking needs and nothing else. +type rankedRow struct { + rid int64 + bm float64 +} + +// betterThan orders rows the way the per-project query's +// ORDER BY bm ASC, rowid ASC does. SQLite gives more-negative bm25 to better +// matches, so smaller wins; ties break on the lower rowid. The tiebreak is not +// cosmetic — in a trigram index over real code most hits share a score with +// another hit, and without it the surviving set would depend on the order the +// scan happened to visit rows in. +func (r rankedRow) betterThan(o rankedRow) bool { + if r.bm != o.bm { + return r.bm < o.bm + } + return r.rid < o.rid +} + +// topHits keeps the best n rows seen for one project. +// +// h is a max-heap on `betterThan`: h[0] is the WORST row kept, which is the +// one a new row has to beat. That makes the common case — a row that does not +// make the cut — a single comparison, which is the whole point of doing this +// here instead of in SQL. See workspaceScanQuery for why. +type topHits struct { + n int + h []rankedRow +} + +func (t *topHits) offer(r rankedRow) { + if t.n <= 0 { + return + } + if len(t.h) < t.n { + t.h = append(t.h, r) + t.up(len(t.h) - 1) + return + } + if t.h[0].betterThan(r) { + return + } + t.h[0] = r + t.down(0) +} + +// sorted returns the kept rows best-first, leaving the heap unusable. +func (t *topHits) sorted() []rankedRow { + out := t.h + sort.Slice(out, func(i, j int) bool { return out[i].betterThan(out[j]) }) + t.h = nil + return out +} + +func (t *topHits) up(i int) { + for i > 0 { + p := (i - 1) / 2 + if !t.h[p].betterThan(t.h[i]) { + return + } + t.h[p], t.h[i] = t.h[i], t.h[p] + i = p + } +} + +func (t *topHits) down(i int) { + for { + worst := i + for _, c := range [2]int{2*i + 1, 2*i + 2} { + if c < len(t.h) && t.h[worst].betterThan(t.h[c]) { + worst = c + } + } + if worst == i { + return + } + t.h[i], t.h[worst] = t.h[worst], t.h[i] + i = worst + } +} + +// fetchPayload reads the chunk columns for the rows that survived ranking. +func fetchPayload(ctx context.Context, db *sql.DB, rids []int64) (map[int64]Hit, error) { + out := make(map[int64]Hit, len(rids)) + for start := 0; start < len(rids); start += payloadFetchBatch { + end := start + payloadFetchBatch + if end > len(rids) { + end = len(rids) + } + batch := rids[start:end] + args := make([]any, len(batch)) + for i, rid := range batch { + args[i] = rid + } + rows, err := db.QueryContext(ctx, payloadQuery(placeholders(len(batch))), args...) + if err != nil { + return nil, fmt.Errorf("chunks_fts payload fetch: %w", err) + } + for rows.Next() { + var rid int64 + var h Hit + var chunkT, symName, language sql.NullString + if err := rows.Scan(&rid, &h.FilePath, &h.StartLine, &h.EndLine, + &chunkT, &symName, &language, &h.Content); err != nil { + rows.Close() + return nil, fmt.Errorf("scan chunks_fts payload row: %w", err) + } + h.ChunkType = chunkT.String + h.SymbolName = symName.String + h.Language = language.String + out[rid] = h + } + err = rows.Err() + rows.Close() + if err != nil { + return nil, fmt.Errorf("iterate chunks_fts payload: %w", err) + } + } + return out, nil +} + +// workspaceScanQuery streams every matched row's project, rowid and BM25 +// score. It deliberately does no ordering and no trimming: the caller keeps a +// bounded per-project heap as the rows go past. // -// Split out so the test that pins the query PLAN builds the same string this -// does. A copy in the test would drift, and drifting is the entire failure -// mode being guarded against. +// The obvious form asks SQLite for the answer directly, with +// ROW_NUMBER() OVER (PARTITION BY project_path ORDER BY bm) and rn <= N. That +// is correct and it is what this shipped first, but a window function has to +// sort the ENTIRE match set to find N rows per project, and the match set here +// is the whole server's index: the trigram tokenizer makes a common short word +// match a quarter of the corpus, because "and" occurs inside command, handler, +// standard and random. On the load-test fixture a six-term query matched +// 623,913 rows to keep 2,300, and a six-term query with the word "test" in it +// matched 1,288,739. Sorting those to keep fifty per project is the single +// largest cost in the statement. // -// Rank on the cheap columns, then fetch the payload for the rows that -// survived. The obvious form — selecting file_path, content and the rest -// inside the CTE — makes SQLite materialise all of it for EVERY matched -// row before ROW_NUMBER trims to `perProject` per project, and the match -// set is the whole server's index. On the load-test fixture a four-common- -// word query matched 263,515 rows to return 2,300: carrying the payload -// through cost 15.1 s against 2.8 s for this form, and a 624k-row match -// cost 29.6 s — worse than the 46 per-project queries this replaced. The -// rowid round-trip is the cheap half of the trade. +// A bounded heap looks at each row once and rejects most of them in one +// comparison. Measured on that fixture, window form vs heap: 144 ms vs 119 ms +// on a 21k match set, 2,466 vs 1,455 on 624k, 5,216 vs 3,014 on 1.29M. It wins +// at every size, and it wins by more as the match set grows. // -// Those numbers came from a bench harness that is NOT in this repository: -// /loadtests/ is gitignored, corpus and tools alike. To recreate it, time -// this statement against a corpus with a large match set alongside the -// same statement with the payload columns moved into `hits` — the gap only -// appears when the match set is orders of magnitude larger than the result. -// TestSearchProjects_FetchesPayloadAfterTheTrim is the in-repo guard. -func workspaceRankQuery(ph string) string { +// The result is IDENTICAL, not merely close — same rows, same order. That is +// what TestSearchProjects_MatchesPerProjectQueries checks, against the +// per-project statement as the oracle. +// +// Those timings came from a bench harness that is NOT in this repository: +// /loadtests/ is gitignored, corpus and tools alike. To recreate it, time this +// statement plus the Go-side heap against the same statement wrapped in the +// window form, over a corpus whose match set is orders of magnitude larger +// than the result. +func workspaceScanQuery(ph string) string { return fmt.Sprintf(` - WITH hits AS ( - SELECT cm.project_path AS pp, cm.rowid AS rid, bm25(chunks_fts) AS bm - FROM chunks_fts cf - JOIN chunks_meta cm ON cm.rowid = cf.rowid - WHERE chunks_fts MATCH ? AND cm.project_path IN (%s) - ), - ranked AS ( - SELECT pp, rid, bm, - ROW_NUMBER() OVER (PARTITION BY pp ORDER BY bm ASC, rid ASC) AS rn - FROM hits - ) - SELECT r.pp, cm.file_path, cm.start_line, cm.end_line, - cm.chunk_type, cm.symbol_name, cm.language, cf.content, r.bm - FROM ranked r - JOIN chunks_meta cm ON cm.rowid = r.rid - JOIN chunks_fts cf ON cf.rowid = r.rid - WHERE r.rn <= ? - ORDER BY r.pp, r.rn`, ph) + SELECT cm.project_path, cm.rowid, bm25(chunks_fts) + FROM chunks_fts cf + JOIN chunks_meta cm ON cm.rowid = cf.rowid + WHERE chunks_fts MATCH ? AND cm.project_path IN (%s)`, ph) +} + +// payloadQuery fetches the chunk columns for rows that already survived +// ranking. Keeping the payload out of the scan matters for the same reason the +// ranking is not done in SQL: carrying file_path and content through a +// 600k-row scan materialises them for every match to return a couple of +// thousand. Both halves of that lesson cost a shipped regression to learn. +func payloadQuery(ph string) string { + return fmt.Sprintf(` + SELECT cm.rowid, cm.file_path, cm.start_line, cm.end_line, + cm.chunk_type, cm.symbol_name, cm.language, cf.content + FROM chunks_meta cm + JOIN chunks_fts cf ON cf.rowid = cm.rowid + WHERE cm.rowid IN (%s)`, ph) } // placeholders builds "?,?,?" for an IN list. n is always >= 1: SearchProjects @@ -395,18 +608,15 @@ func placeholders(n int) string { return strings.TrimSuffix(strings.Repeat("?,", n), ",") } -// scanHit reads one single-project ranking row. -func scanHit(rows *sql.Rows) (Hit, error) { return scanRankedHit(rows, nil) } - -// scanRankedHit reads one ranking row, optionally preceded by a project_path -// column (pass nil for the single-project query, which does not select one). +// scanHit reads one row of the single-project ranking query. // -// Both queries share this because they must produce byte-identical Hits and -// their only structural difference is that leading column. Two hand-written -// scans agreeing is exactly the kind of thing that stays true until it -// quietly does not — and the equivalence test compares the two paths against -// EACH OTHER, so a mistake made symmetrically in both would pass. -func scanRankedHit(rows *sql.Rows, pp *string) (Hit, error) { +// The workspace query no longer shares this: it scans (project, rowid, score) +// and fetches the payload separately, so the two paths now build a Hit in two +// different places. They must still produce byte-identical Hits — +// TestSearchProjects_MatchesPerProjectQueries compares them — and a mistake +// made symmetrically in both would pass that test, so keep the two column +// lists side by side when editing either. +func scanHit(rows *sql.Rows) (Hit, error) { var ( h Hit chunkT sql.NullString @@ -414,17 +624,8 @@ func scanRankedHit(rows *sql.Rows, pp *string) (Hit, error) { language sql.NullString bm float64 ) - // Built in one allocation rather than prepending to a finished slice: the - // workspace query returns up to len(projects) * perProject rows — ~2,150 - // on the 43-project load-test fixture — and this runs per row, inside the - // path the rest of this change exists to speed up. - dest := make([]any, 0, 9) - if pp != nil { - dest = append(dest, pp) - } - dest = append(dest, &h.FilePath, &h.StartLine, &h.EndLine, - &chunkT, &symName, &language, &h.Content, &bm) - if err := rows.Scan(dest...); err != nil { + if err := rows.Scan(&h.FilePath, &h.StartLine, &h.EndLine, + &chunkT, &symName, &language, &h.Content, &bm); err != nil { return Hit{}, fmt.Errorf("scan chunks_fts row: %w", err) } h.ChunkType = chunkT.String diff --git a/server/internal/chunksfts/chunksfts_test.go b/server/internal/chunksfts/chunksfts_test.go index 33beeba..e55a1b4 100644 --- a/server/internal/chunksfts/chunksfts_test.go +++ b/server/internal/chunksfts/chunksfts_test.go @@ -4,6 +4,8 @@ import ( "context" "database/sql" "fmt" + "math/rand" + "sort" "strings" "testing" @@ -431,10 +433,14 @@ func TestSearchProjects_DoesNotPrefixMatchProjectPaths(t *testing.T) { } } -// TestSearchProjects_SpansTheBatchBoundary checks the IN-list batching. The -// map is filled across several statements, so a batch that overwrote instead -// of appending, or that dropped its last slice, would only show up above the -// batch size. +// TestSearchProjects_SpansTheBatchBoundary checks the project IN-list batching. +// dst is filled across several statements and each project is written to +// exactly once — a batch that replaced the map instead of adding to it, or that +// dropped its last slice, would only show up above the batch size. +// +// It does NOT reach the rowid batching inside fetchPayload; one hit per project +// keeps that list at exactly searchProjectsBatch. See +// TestFetchPayload_SpansTheBatchBoundary. func TestSearchProjects_SpansTheBatchBoundary(t *testing.T) { d := openTestDB(t) ctx := context.Background() @@ -478,65 +484,333 @@ func TestSearchProjects_EmptyInputs(t *testing.T) { } } -// TestSearchProjects_FetchesPayloadAfterTheTrim pins the query PLAN, which is -// the only place this bug can live: every equivalence test in this file passes -// against the slow form too, because the two forms return the same rows. -// -// Carrying file_path/content through the CTE makes SQLite materialise them for -// every globally-matched row before ROW_NUMBER trims — and FTS5 evaluates MATCH -// over the whole server's index, so the match set has nothing to do with how -// many rows come back. Measured on the load-test corpus: 15.1 s against 2.8 s -// on a 263k-row match, and 29.6 s on a 624k-row one, which was slower than the -// per-project queries the partitioned form replaced. -// -// FTS5 reports a rowid-equality lookup as "0:=" and a MATCH scan as "0:M...". -// The rank-first form does both — scan to match, point lookups for survivors. -// The payload-in-CTE form only ever scans. -func TestSearchProjects_FetchesPayloadAfterTheTrim(t *testing.T) { +// TestSearchProjects_ScanDoesNotSortTheMatchSet pins the reason the ranking +// moved out of SQL. The window form has to sort every matched row to find N +// per project; on the load-test fixture that is up to 1.29 million rows to +// keep 2,300. The scan query must stay a plain scan — no sorter of any kind. +func TestSearchProjects_ScanDoesNotSortTheMatchSet(t *testing.T) { d := openTestDB(t) ctx := context.Background() seedCorpus(t, d, []string{"p1", "p2"}) - plan := explain(t, ctx, d, workspaceRankQuery(placeholders(2)), - `"retry" OR "backoff"`, "p1", "p2", 3) - if !strings.Contains(plan, "VIRTUAL TABLE INDEX 0:=") { - t.Errorf("chunks_fts is not looked up by rowid — the payload is being "+ - "carried through the window sorter again:\n%s", plan) + plan := explain(t, ctx, d, workspaceScanQuery(placeholders(2)), + `"retry" OR "backoff"`, "p1", "p2") + if strings.Contains(plan, "TEMP B-TREE") { + t.Errorf("the ranking scan sorts the whole match set again:\n%s", plan) } } -// TestExplainDistinguishesTheTwoQueryShapes is the mutation check for the test -// above, kept in the tree rather than run by hand: it builds the slow form and -// asserts the plan assertion would REJECT it. Without this, a change to how -// SQLite reports plans could turn the guard into a tautology that passes on -// everything, and nothing would say so. -func TestExplainDistinguishesTheTwoQueryShapes(t *testing.T) { +// TestExplainRejectsTheSortingForms is the mutation check for the test above, +// kept in the tree rather than run by hand: it builds forms that DO sort the +// match set and asserts the assertion above would reject each one. Without +// this, a change in how SQLite reports plans could turn the guard into a +// tautology that passes on everything, and nothing would say so. +// +// Two shapes, not one. The window form is what this PR deleted; a plain +// ORDER BY added back to the scan is the regression far more likely to +// actually happen, and a guard is worth exactly what it rejects. +func TestExplainRejectsTheSortingForms(t *testing.T) { d := openTestDB(t) ctx := context.Background() seedCorpus(t, d, []string{"p1", "p2"}) - slow := ` + window := ` WITH hits AS ( - SELECT cm.project_path AS pp, cm.rowid AS rid, - cm.file_path, cm.start_line, cm.end_line, - cm.chunk_type, cm.symbol_name, cm.language, - cf.content, bm25(chunks_fts) AS bm + SELECT cm.project_path AS pp, cm.rowid AS rid, bm25(chunks_fts) AS bm FROM chunks_fts cf JOIN chunks_meta cm ON cm.rowid = cf.rowid WHERE chunks_fts MATCH ? AND cm.project_path IN (?,?) + ), + ranked AS ( + SELECT pp, rid, bm, + ROW_NUMBER() OVER (PARTITION BY pp ORDER BY bm ASC, rid ASC) AS rn + FROM hits ) - SELECT pp, file_path, start_line, end_line, - chunk_type, symbol_name, language, content, bm - FROM (SELECT *, ROW_NUMBER() OVER ( - PARTITION BY pp ORDER BY bm ASC, rid ASC) AS rn - FROM hits) - WHERE rn <= ? - ORDER BY pp, rn` - - plan := explain(t, ctx, d, slow, `"retry" OR "backoff"`, "p1", "p2", 3) - if strings.Contains(plan, "VIRTUAL TABLE INDEX 0:=") { - t.Errorf("the payload-in-CTE form reports a rowid lookup, so the plan "+ - "assertion no longer distinguishes the two shapes:\n%s", plan) + SELECT r.pp, cm.file_path, r.bm + FROM ranked r + JOIN chunks_meta cm ON cm.rowid = r.rid + WHERE r.rn <= ?` + + for _, tc := range []struct { + name string + query string + args []any + }{ + { + name: "the window form this replaced", + query: window, + args: []any{`"retry" OR "backoff"`, "p1", "p2", 3}, + }, + { + // Written out rather than derived from workspaceScanQuery: this + // subtest is a claim about how SQLite REPORTS a sort, not about + // production code, and appending to the real statement made it + // fail for the wrong reason whenever that statement was itself + // mutated to sort. + name: "an ORDER BY added back to the scan", + query: ` + SELECT cm.project_path, cm.rowid, bm25(chunks_fts) + FROM chunks_fts cf + JOIN chunks_meta cm ON cm.rowid = cf.rowid + WHERE chunks_fts MATCH ? AND cm.project_path IN (?,?) + ORDER BY bm25(chunks_fts)`, + args: []any{`"retry" OR "backoff"`, "p1", "p2"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + plan := explain(t, ctx, d, tc.query, tc.args...) + if !strings.Contains(plan, "TEMP B-TREE") { + t.Errorf("this form no longer reports a sorter, so the plan "+ + "assertion no longer distinguishes it:\n%s", plan) + } + }) + } +} + +// TestSearchProjects_FetchesPayloadByRowid guards the second half of the same +// lesson: file_path and content are fetched for the rows that survived, by +// rowid, and never carried through the scan. +// +// The assertion is on the WHOLE FTS5 idxStr, not a prefix of it. FTS5 packs its +// plan into one string: "0:=" is a bare rowid lookup, and a MATCH adds an "M" +// plus the matched column, so a payload fetch that ALSO matched reports +// "0:=M3" — which still contains "0:=" and does not contain "0:M". The first +// version of this test asserted on those two prefixes and therefore passed on +// exactly the merge it existed to catch. Found in review of #266, not by the +// suite, which is the whole argument for the companion test below. +func TestSearchProjects_FetchesPayloadByRowid(t *testing.T) { + d := openTestDB(t) + ctx := context.Background() + seedCorpus(t, d, []string{"p1", "p2"}) + + idxs := ftsIndexes(explain(t, ctx, d, payloadQuery(placeholders(2)), 1, 2)) + if len(idxs) == 0 { + t.Fatal("the payload fetch does not touch chunks_fts at all") + } + for _, idx := range idxs { + if idx != "0:=" { + t.Errorf(`chunks_fts is not a plain rowid lookup in the payload `+ + `fetch: idxStr %q ("=" is the rowid constraint; an "M" means a `+ + `MATCH crept back in)`, idx) + } + } +} + +// ftsIndexes returns every FTS5 idxStr in a query plan, whole. The planner +// prints it as "... VIRTUAL TABLE INDEX " at the end of the line. +func ftsIndexes(plan string) []string { + var out []string + for _, line := range strings.Split(plan, "\n") { + if _, idx, ok := strings.Cut(line, "VIRTUAL TABLE INDEX "); ok { + out = append(out, strings.TrimSpace(idx)) + } + } + return out +} + +// TestExplainRejectsThePayloadShapes is the mutation check for the test above, +// kept in the tree rather than run by hand: it builds the shapes that test +// exists to reject and asserts it would reject them. +// +// The MATCH case is not hypothetical. The prefix-matching version of the guard +// let it straight through, and nothing in the suite said so. +func TestExplainRejectsThePayloadShapes(t *testing.T) { + d := openTestDB(t) + ctx := context.Background() + seedCorpus(t, d, []string{"p1", "p2"}) + + for _, tc := range []struct { + name string + query string + args []any + }{ + { + name: "the two statements merged back together", + query: ` + SELECT cm.rowid, cm.file_path, cf.content + FROM chunks_meta cm + JOIN chunks_fts cf ON cf.rowid = cm.rowid + WHERE chunks_fts MATCH ? AND cm.rowid IN (?,?)`, + args: []any{`"retry" OR "backoff"`, 1, 2}, + }, + { + name: "a join FTS5 cannot serve by rowid", + query: ` + SELECT cm.rowid, cm.file_path, cf.content + FROM chunks_meta cm + JOIN chunks_fts cf ON cf.rowid + 0 = cm.rowid + WHERE cm.rowid IN (?,?)`, + args: []any{1, 2}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + plan := explain(t, ctx, d, tc.query, tc.args...) + for _, idx := range ftsIndexes(plan) { + if idx != "0:=" { + return + } + } + t.Errorf("this shape reports a plain rowid lookup, so the payload "+ + "guard no longer distinguishes it:\n%s", plan) + }) + } +} + +// TestFetchPayload_SpansTheBatchBoundary covers the rowid IN-list batching. +// +// TestSearchProjects_SpansTheBatchBoundary cannot reach it, and the reason is a +// coincidence of the two constants being equal: that test seeds one hit per +// project, so the rowid list is at most searchProjectsBatch long and the +// payload loop runs exactly once however many projects there are. Production is +// 43 projects x 50 hits = five batches, so without this the path that always +// runs in production would be the one nothing covers. +func TestFetchPayload_SpansTheBatchBoundary(t *testing.T) { + d := openTestDB(t) + ctx := context.Background() + + const n = payloadFetchBatch + 7 + chunks := make([]Chunk, 0, n) + for i := 0; i < n; i++ { + chunks = append(chunks, Chunk{ + Content: "func retryWithBackoff() {}", + FilePath: "a.go", + StartLine: 1 + i*10, EndLine: 5 + i*10, + Language: "go", + }) + } + upsert(t, d, "proj", "a.go", chunks) + + got, err := SearchProjects(ctx, d, []string{"proj"}, "retry", n) + if err != nil { + t.Fatalf("SearchProjects: %v", err) + } + if len(got["proj"]) != n { + t.Errorf("got %d hits, want %d — a payload batch was dropped", + len(got["proj"]), n) + } +} + +// TestCollectHits covers what splitting one statement into two actually +// changed: a chunk can disappear between the ranking scan and the payload +// fetch. Racing a real delete against a live query is not worth building, so +// the seam is tested directly — a payload map with rows deliberately left out +// is exactly the state that race produces. +func TestCollectHits(t *testing.T) { + rows := []rankedRow{{rid: 7, bm: -9}, {rid: 8, bm: -5}, {rid: 9, bm: -1}} + full := map[int64]Hit{ + 7: {FilePath: "a.go"}, 8: {FilePath: "b.go"}, 9: {FilePath: "c.go"}, + } + + t.Run("all present", func(t *testing.T) { + dst := map[string][]Hit{} + collectHits(map[string][]rankedRow{"p": rows}, full, dst) + got := dst["p"] + if len(got) != 3 { + t.Fatalf("got %d hits, want 3", len(got)) + } + for i, want := range []struct { + file string + score float64 + }{{"a.go", 9}, {"b.go", 5}, {"c.go", 1}} { + if got[i].FilePath != want.file || got[i].Score != want.score { + t.Errorf("rank %d: got %s/%v, want %s/%v", + i, got[i].FilePath, got[i].Score, want.file, want.score) + } + } + }) + + t.Run("one row vanished", func(t *testing.T) { + partial := map[int64]Hit{7: full[7], 9: full[9]} + dst := map[string][]Hit{} + collectHits(map[string][]rankedRow{"p": rows}, partial, dst) + got := dst["p"] + if len(got) != 2 { + t.Fatalf("got %d hits, want 2", len(got)) + } + if got[0].FilePath != "a.go" || got[1].FilePath != "c.go" { + t.Errorf("got %s,%s — the surviving rows lost their rank order", + got[0].FilePath, got[1].FilePath) + } + if got[0].Score != 9 || got[1].Score != 1 { + t.Errorf("got scores %v,%v — a dropped row shifted the scores", + got[0].Score, got[1].Score) + } + }) + + t.Run("every row vanished", func(t *testing.T) { + dst := map[string][]Hit{} + collectHits(map[string][]rankedRow{"p": rows}, map[int64]Hit{}, dst) + if _, present := dst["p"]; present { + t.Errorf("a project whose every survivor vanished is present with "+ + "%d hits; this package's contract is that it is absent", + len(dst["p"])) + } + }) +} + +// TestTopHits_MatchesAFullSort is the property test for the bounded heap that +// replaced SQLite's window function. +// +// The scores are drawn from a deliberately tiny set so that most rows tie: +// in a trigram index over real code most hits share a score with another hit, +// which makes the (score, rowid) tiebreak the part most likely to be wrong and +// least likely to be noticed. Rows are offered in a shuffled order, because an +// implementation that quietly depended on arrival order would still pass if +// they arrived sorted. +func TestTopHits_MatchesAFullSort(t *testing.T) { + for _, n := range []int{1, 3, 50} { + for seed := int64(1); seed <= 20; seed++ { + rng := rand.New(rand.NewSource(seed)) + rows := make([]rankedRow, 0, 500) + for i := 0; i < 500; i++ { + rows = append(rows, rankedRow{ + rid: int64(rng.Intn(1 << 20)), + bm: -float64(rng.Intn(8)), + }) + } + seen := map[int64]bool{} + uniq := rows[:0] + for _, r := range rows { + if !seen[r.rid] { + seen[r.rid] = true + uniq = append(uniq, r) + } + } + rows = uniq + + top := &topHits{n: n} + for _, r := range rows { + top.offer(r) + } + got := top.sorted() + + want := append([]rankedRow(nil), rows...) + sort.Slice(want, func(i, j int) bool { return want[i].betterThan(want[j]) }) + if len(want) > n { + want = want[:n] + } + if len(got) != len(want) { + t.Fatalf("n=%d seed=%d: kept %d rows, a full sort keeps %d", + n, seed, len(got), len(want)) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("n=%d seed=%d: rank %d is %+v, a full sort puts %+v there", + n, seed, i, got[i], want[i]) + } + } + } + } +} + +// TestTopHits_ZeroLimitKeepsNothing pins the guard in offer. perProject is +// clamped to a positive number by SearchProjects, so this is about the heap +// being safe on its own terms rather than about a reachable call. +func TestTopHits_ZeroLimitKeepsNothing(t *testing.T) { + top := &topHits{n: 0} + top.offer(rankedRow{rid: 1, bm: -9}) + if got := top.sorted(); len(got) != 0 { + t.Errorf("n=0 kept %d rows", len(got)) } }