diff --git a/CHANGELOG.md b/CHANGELOG.md index e8f9800..ba97dd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixes +- Looking a symbol up by name no longer reads the whole graph. Every search made one full pass over all indexed symbols for each word you typed, and a question that named several symbols made two more passes per name — including for a word that matches nothing, which is the common case. The cost therefore grew with the size of the project, and it was paid again on every message when the prompt hook is enabled. These lookups now go through the name index instead. Results are identical; only the time to get them changes, and it no longer grows with the project. + - Naming a file by its path in a `codegraph_explore` query now works reliably: the path is resolved against the index and that file is guaranteed a place at the top of the answer. Previously the path was broken into fragments — bracketed route segments like SvelteKit's `[id]` made this worst — and pieces like `page` or `runs` matched every sibling file, so the file you actually named could be crowded out of the answer entirely. A path that doesn't match any indexed file is now called out instead of silently ignored. - Naming a kebab-case file **without its extension** in a `codegraph_explore` query — `background-image-table` rather than `background-image-table.tsx`, the way import paths and prose spell it — now returns that exact file too. Previously the name was split at the hyphens, and in a kebab-cased frontend those pieces (`background`, `image`, `table`) are among the most common words in the codebase, so look-alike sibling files filled the answer while the named file never appeared. Hyphenated words that don't name an indexed file, like "cross-call" or "non-blocking", are left alone. - Plainly-worded `codegraph_explore` questions now find camelCase code: a query like "auto-scroll to bottom" can reach a function named `scrollFeedToBottom`, because query words are matched against the words inside identifiers, not just whole names. diff --git a/__tests__/name-lookup-index.test.ts b/__tests__/name-lookup-index.test.ts new file mode 100644 index 0000000..dce2f99 --- /dev/null +++ b/__tests__/name-lookup-index.test.ts @@ -0,0 +1,206 @@ +/** + * Exact-name lookups must seek `idx_nodes_lower_name` + * + * `nodes` carries two name indexes and neither one can serve + * `WHERE name = ? COLLATE NOCASE`: + * + * - `idx_nodes_name` is BINARY-collated, so NOCASE equality can't use it; + * - `idx_nodes_lower_name` is an expression index on `lower(name)`, and the + * planner only matches it against the same expression. + * + * So every exact-name lookup written that way degrades to a full table scan. + * The `LIMIT`s on those queries do not save them: SQLite can only stop early + * once it has produced `LIMIT` rows, and the common cases — a query term that + * is not a symbol at all, or a name with only a handful of definitions — never + * reach it and scan the whole table. + * + * These tests read the planner's own verdict rather than a wall-clock number, + * so they are deterministic and fail loudly if a lookup regresses to a scan. + * `lower(name) = lower(?)` (not a JS-side `.toLowerCase()`) is the required + * form — see the folding-parity test at the bottom for why. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import * as fs from 'fs'; +import * as path from 'path'; +import * as os from 'os'; +import { DatabaseConnection } from '../src/db'; +import { QueryBuilder } from '../src/db/queries'; +import { SqliteDatabase } from '../src/db/sqlite-adapter'; +import { Node } from '../src/types'; + +function makeNode(id: string, name: string, filePath = 'src/a.ts'): Node { + return { + id, + kind: 'function', + name, + qualifiedName: name, + filePath, + language: 'typescript', + startLine: 1, + endLine: 2, + startColumn: 0, + endColumn: 0, + updatedAt: Date.now(), + }; +} + +/** Wraps a db so every `prepare()` is recorded, then delegates unchanged. */ +function recordingDb(raw: SqliteDatabase): { db: SqliteDatabase; sqls: string[] } { + const sqls: string[] = []; + const db: SqliteDatabase = { + prepare(sql: string) { + sqls.push(sql); + return raw.prepare(sql); + }, + exec: (sql: string) => raw.exec(sql), + pragma: (str: string, options?: { simple?: boolean }) => raw.pragma(str, options), + transaction: (fn: (...args: any[]) => T) => raw.transaction(fn), + close: () => raw.close(), + get open() { + return raw.open; + }, + }; + return { db, sqls }; +} + +/** SQL that filters `nodes` on whole-name equality, in either spelling. */ +function exactNameLookups(sqls: string[]): string[] { + return sqls.filter( + (s) => + /\bFROM\s+nodes\b/i.test(s) && + (/\bname\s*(COLLATE\s+NOCASE\s*)?=\s*\?(\s*COLLATE\s+NOCASE)?/i.test(s) || + /\blower\(name\)\s*=/i.test(s)) + ); +} + +/** The planner's access path for the `nodes` table in a statement. */ +function nodesAccessPath(raw: SqliteDatabase, sql: string): string { + const args = new Array((sql.match(/\?/g) ?? []).length).fill('x'); + const rows = raw.prepare(`EXPLAIN QUERY PLAN ${sql}`).all(...args) as { detail: string }[]; + const detail = rows.map((r) => r.detail).find((d) => /\bnodes\b/.test(d)); + return detail ?? rows.map((r) => r.detail).join(' | '); +} + +describe('exact-name lookups seek idx_nodes_lower_name', () => { + let dir: string; + let conn: DatabaseConnection; + let raw: SqliteDatabase; + + beforeAll(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'name-lookup-index-')); + conn = DatabaseConnection.initialize(path.join(dir, 'test.db')); + raw = conn.getDb(); + const seed = new QueryBuilder(raw); + + // A corpus wide enough that a scan and a seek can't accidentally agree on + // ordering, with `handleRequest` deliberately rare (2 nodes) — the shape + // the LIMITs never short-circuit on. + const nodes: Node[] = []; + for (let i = 0; i < 300; i++) { + nodes.push(makeNode(`filler-${i}`, `filler${i}Symbol`, `src/pkg${i % 7}/f${i}.ts`)); + } + nodes.push(makeNode('hr-1', 'handleRequest', 'src/server/router.ts')); + nodes.push(makeNode('hr-2', 'HandleRequest', 'src/server/legacy.ts')); + for (const n of nodes) seed.insertNode(n); + }); + + afterAll(() => { + conn.close(); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('searchNodes issues its exact-name supplement as an index seek', () => { + const { db, sqls } = recordingDb(raw); + const q = new QueryBuilder(db); + + const results = q.searchNodes('handleRequest'); + expect(results.length).toBeGreaterThan(0); + + const lookups = exactNameLookups(sqls); + // Guard against a vacuous pass: the supplement must actually have run. + expect(lookups.length).toBeGreaterThan(0); + + for (const sql of lookups) { + expect(nodesAccessPath(raw, sql)).toMatch(/SEARCH nodes USING .*idx_nodes_lower_name/); + } + }); + + it('findNodesByExactName issues both of its passes as index seeks', () => { + const { db, sqls } = recordingDb(raw); + const q = new QueryBuilder(db); + + const results = q.findNodesByExactName(['handleRequest']); + expect(results.length).toBeGreaterThan(0); + + const lookups = exactNameLookups(sqls); + // Two passes: the file_path probe and the row fetch. + expect(lookups.length).toBeGreaterThanOrEqual(2); + + for (const sql of lookups) { + expect(nodesAccessPath(raw, sql)).toMatch(/SEARCH nodes USING .*idx_nodes_lower_name/); + } + }); + + it('getNodesByLowerName seeks the index and does not depend on the caller lowering', () => { + const { db, sqls } = recordingDb(raw); + const q = new QueryBuilder(db); + + // Previously this took an already-lowered string on trust: anything with an + // uppercase letter in it silently returned nothing. + expect(q.getNodesByLowerName('handlerequest').map((n) => n.id).sort()).toEqual([ + 'hr-1', + 'hr-2', + ]); + expect(q.getNodesByLowerName('HandleRequest').map((n) => n.id).sort()).toEqual([ + 'hr-1', + 'hr-2', + ]); + expect(q.getNodesByLowerName('HANDLEREQUEST').map((n) => n.id).sort()).toEqual([ + 'hr-1', + 'hr-2', + ]); + + const lookups = exactNameLookups(sqls); + expect(lookups.length).toBeGreaterThan(0); + for (const sql of lookups) { + expect(nodesAccessPath(raw, sql)).toMatch(/SEARCH nodes USING .*idx_nodes_lower_name/); + } + }); + + it('still matches case-insensitively across both call sites', () => { + const q = new QueryBuilder(raw); + + const exact = q.findNodesByExactName(['HANDLEREQUEST']); + expect(exact.map((r) => r.node.id).sort()).toEqual(['hr-1', 'hr-2']); + + const searched = q.searchNodes('HandleRequest'); + const ids = new Set(searched.map((r) => r.node.id)); + expect(ids.has('hr-1')).toBe(true); + expect(ids.has('hr-2')).toBe(true); + }); + + it('folds exactly what COLLATE NOCASE folded — ASCII only', () => { + // SQLite's NOCASE and its `lower()` are both ASCII-only. JavaScript's + // `.toLowerCase()` is not, so lowering the parameter in JS and comparing + // against `lower(name)` would silently stop matching non-ASCII names that + // NOCASE used to match. `lower(?)` keeps both sides on SQLite's rules. + const probe = new QueryBuilder(raw); + probe.insertNode(makeNode('uni-1', 'Ünïcode', 'src/i18n/a.ts')); + + const found = probe.findNodesByExactName(['Ünïcode']); + expect(found.map((r) => r.node.id)).toContain('uni-1'); + + // The mixed-ASCII half still folds, as NOCASE did. + probe.insertNode(makeNode('uni-2', 'Ünïcodeloader', 'src/i18n/b.ts')); + const folded = probe.findNodesByExactName(['ÜnïcodeLOADER']); + expect(folded.map((r) => r.node.id)).toContain('uni-2'); + + // Same rule for the fuzzy-match lookup. Note what this does NOT claim: a + // caller that lowers in JavaScript first still hands over `ünïcode`, which + // is not what SQLite's `lower()` makes of `Ünïcode`, so the gap stays open + // on that side. + expect(probe.getNodesByLowerName('Ünïcode').map((n) => n.id)).toContain('uni-1'); + expect(probe.getNodesByLowerName('ÜnïcodeLOADER').map((n) => n.id)).toContain('uni-2'); + }); +}); diff --git a/src/db/queries.ts b/src/db/queries.ts index 2b8bc53..788c022 100644 --- a/src/db/queries.ts +++ b/src/db/queries.ts @@ -1168,15 +1168,26 @@ export class QueryBuilder { } /** - * Get nodes by lowercase name match (uses idx_nodes_lower_name expression index) + * Get nodes by name, case-insensitively (seeks the idx_nodes_lower_name + * expression index). + * + * The parameter is lowered in SQL rather than trusted to arrive lowered, so + * the lookup means the same thing whatever casing a caller hands it. Written + * as a bare `lower(name) = ?` it silently returned nothing for any input + * carrying an uppercase letter, and — because SQLite's `lower()` folds ASCII + * only while JavaScript's `.toLowerCase()` folds Unicode — a caller that + * pre-lowered in JavaScript could not match a non-ASCII name at all. + * + * Note this hardens the query, not its one caller: `matchFuzzy` still lowers + * in JavaScript before calling, so the non-ASCII gap remains open there. */ - getNodesByLowerName(lowerName: string): Node[] { + getNodesByLowerName(name: string): Node[] { if (!this.stmts.getNodesByLowerName) { this.stmts.getNodesByLowerName = this.db.prepare( - 'SELECT * FROM nodes WHERE lower(name) = ?' + 'SELECT * FROM nodes WHERE lower(name) = lower(?)' ); } - const rows = this.stmts.getNodesByLowerName.all(lowerName) as NodeRow[]; + const rows = this.stmts.getNodesByLowerName.all(name) as NodeRow[]; return rows.map(rowToNode); } @@ -1242,12 +1253,25 @@ export class QueryBuilder { // pushing them past the FTS fetch limit before post-hoc scoring can help. // Use the max BM25 score as the base so the nameMatchBonus (exact=30 vs // prefix=20) actually differentiates them after rescoring. + // + // Whole-name equality MUST be written as `lower(name) = lower(?)` so it + // seeks `idx_nodes_lower_name`. The equivalent `name = ? COLLATE NOCASE` + // matches no index — `idx_nodes_name` is BINARY-collated and the expression + // index only matches the same expression — and degrades to a full table + // scan. The `LIMIT 20` does not rescue it: SQLite can only stop early once + // it has produced 20 rows, and this runs once per query term, most of which + // name nothing in the corpus. Measured per term on an unmatched term: + // 0.08ms on gin (2.5k nodes), 0.39ms on excalidraw (11k), 2.4ms on django + // (62k) — and growing with the corpus, where the seek is flat at ~0.002ms. + // Lowering the parameter in SQL rather than in JS is deliberate: SQLite's + // `lower()` and NOCASE both fold ASCII only, while JS `.toLowerCase()` + // folds Unicode, which would silently stop matching non-ASCII names. if (results.length > 0 && query) { const existingIds = new Set(results.map(r => r.node.id)); const maxFtsScore = Math.max(...results.map(r => r.score)); const terms = query.split(/\s+/).filter(t => t.length >= 2); for (const term of terms) { - let sql = 'SELECT * FROM nodes WHERE name = ? COLLATE NOCASE'; + let sql = 'SELECT * FROM nodes WHERE lower(name) = lower(?)'; const params: (string | number)[] = [term]; if (kinds && kinds.length > 0) { sql += ` AND kind IN (${kinds.map(() => '?').join(',')})`; @@ -1547,9 +1571,16 @@ export class QueryBuilder { // Pass 2: Query each name, boosting results that co-locate with distinctive symbols. // Pass 1: Find files containing each queried name, identify distinctive names + // + // Both passes spell whole-name equality as `lower(name) = lower(?)` so they + // seek `idx_nodes_lower_name` — see the note in `searchNodes` for why the + // `name = ? COLLATE NOCASE` form full-scans instead. This path is the one + // that hurts most: it runs both passes for every symbol extracted from the + // query, and extraction is generous, so most of those names are absent from + // the corpus and never reach either LIMIT. const nameToFiles = new Map>(); for (const name of names) { - let sql = 'SELECT DISTINCT file_path FROM nodes WHERE name COLLATE NOCASE = ?'; + let sql = 'SELECT DISTINCT file_path FROM nodes WHERE lower(name) = lower(?)'; const params: (string | number)[] = [name]; if (kinds && kinds.length > 0) { sql += ` AND kind IN (${kinds.map(() => '?').join(',')})`; @@ -1577,7 +1608,7 @@ export class QueryBuilder { let sql = ` SELECT nodes.*, 1.0 as score FROM nodes - WHERE name COLLATE NOCASE = ? + WHERE lower(name) = lower(?) `; const params: (string | number)[] = [name];