perf(search): seek the name index for exact-name lookups (#1542)
`nodes` carries two name indexes and neither can serve
`WHERE name = ? COLLATE NOCASE`: `idx_nodes_name` is BINARY-collated, and
`idx_nodes_lower_name` is an expression index the planner only matches against
the same expression. All three whole-name lookups in the query layer were
written that way, so each one degraded to a full table scan
(`EXPLAIN QUERY PLAN` reports `SCAN nodes`).
The LIMITs on those queries do not rescue them. SQLite can only stop early once
it has produced LIMIT rows, and the two dominant cases never get there: a query
word that names no symbol at all, and a name with only a handful of definitions.
`searchNodes` runs its supplement once per query term; `findNodesByExactName`
runs two passes per symbol extracted from the question, and extraction is
generous, so a plainly-worded question issues a dozen full scans.
Written as `lower(name) = lower(?)` the same predicate seeks
`idx_nodes_lower_name`. Measured on four indexed repositories, baseline vs fix
in one process (the only difference being how the predicate is spelled):
query "how does the retry backoff work" findNodesByExactName searchNodes
gin (2.5k nodes) 1.27ms -> 0.18ms 3.1 -> 2.6ms
Alamofire (4.5k nodes) 2.39ms -> 0.22ms 4.9 -> 4.0ms
excalidraw (11k nodes) 10.54ms -> 0.17ms 10.4 -> 5.8ms
django (62k nodes) 49.91ms -> 0.17ms 27.6 -> 4.9ms
The seek is flat across all four; the scan grows with the corpus. A one-word
query into `searchNodes` on django is unchanged (~20ms) because a single term's
scan is not what dominates it there.
Lowering the parameter in SQL rather than in JavaScript is deliberate. SQLite's
`lower()` and NOCASE both fold ASCII only, while JavaScript's `.toLowerCase()`
folds Unicode; comparing a JS-lowered parameter against `lower(name)` would
silently stop matching non-ASCII identifiers that NOCASE used to match.
`getNodesByLowerName` is spelled the same way for the same reason. It already
sought the index, but as a bare `lower(name) = ?` it took a pre-lowered
parameter on trust: any input carrying an uppercase letter returned nothing at
all. This is behaviour-neutral for its one caller — `matchFuzzy` lowers in
JavaScript before calling, and `lower()` over an already-lowered string is a
no-op, verified over the ASCII and non-ASCII cases alike. It closes the trap for
the next caller; the non-ASCII gap on the `matchFuzzy` side is a resolution
change and is deliberately not bundled here.
Result sets are unchanged, including which rows the LIMITs keep: entries under
one key in the expression index are ordered by rowid, the same order a table
scan produces. Verified over 14,400 lookups (top-400 names of the four
corpora, probed as stored / upper / lower, against all three call sites) with
zero differences, and end-to-end above with identical result ids.
Tests assert the planner's verdict rather than a wall-clock number, so they are
deterministic: they intercept the SQL each call site prepares and require an
index seek, with a guard that the lookups actually ran. Reverting any call site
turns them red.
Co-authored-by: Colby McHenry <me@colbymchenry.com>
This commit is contained in:
+38
-7
@@ -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<string, Set<string>>();
|
||||
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];
|
||||
|
||||
|
||||
Reference in New Issue
Block a user