From 44711b8effd06736d8b5e184a8643e6b192a4384 Mon Sep 17 00:00:00 2001 From: Maurice Morfaw Date: Mon, 17 Aug 2026 12:13:59 +1000 Subject: [PATCH 1/2] refactor: take searchByIndex and custom-index search options as an object (#2165) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit searchByIndex had grown to eight positional parameters, five optional, so callers padded with undefined or stopped early and inherited defaults they never chose. The same shape reached HierarchicalNavigableSmallWorld.search(), which is the contract an index implemented outside this repo has to satisfy — every future capability there would be another positional argument and a breaking change. The four required leading parameters stay positional on both; the optional tail becomes a named options object: searchByIndex(condition, txn, reverse, Table, { allowFullScan, filtered, context, minResults }) customIndex.search(condition, context, { filter, minResults }) No behaviour change. The plain-JS benchmark caller is updated too, since the TypeScript build cannot flag it. Co-Authored-By: Claude Fable 5 --- benchmarks/hnsw-search.js | 2 +- resources/Table.ts | 2 +- .../HierarchicalNavigableSmallWorld.ts | 30 ++++--- resources/search.ts | 78 ++++++++++--------- .../resources/searchByIndexOptions.test.js | 53 +++++++++++++ unitTests/resources/vectorIndex.test.js | 15 ++-- 6 files changed, 123 insertions(+), 57 deletions(-) create mode 100644 unitTests/resources/searchByIndexOptions.test.js diff --git a/benchmarks/hnsw-search.js b/benchmarks/hnsw-search.js index 0b416aec8d..7fa0177f94 100644 --- a/benchmarks/hnsw-search.js +++ b/benchmarks/hnsw-search.js @@ -223,7 +223,7 @@ for (const p of SELECTIVITIES) { // predicate-aware traversal const t0 = performance.now(); - const pred = hnsw.search({ target: query, comparator: 'sort', descending: false }, {}, filter); + const pred = hnsw.search({ target: query, comparator: 'sort', descending: false }, {}, { filter }); predNs += (performance.now() - t0) * 1e6; const predIdx = new Set(pred.slice(0, k).map((r) => idxOf(r.key))); predRecall += overlap(gt, predIdx) / k; diff --git a/resources/Table.ts b/resources/Table.ts index c96c7e2849..59063a1b84 100644 --- a/resources/Table.ts +++ b/resources/Table.ts @@ -4962,7 +4962,7 @@ export function makeTable(options) { txnForContext(context).getReadTxn(), false, relatedTable, - false + { allowFullScan: false } ) as any ).map((entry) => { if (entry && entry.key !== undefined) return entry; diff --git a/resources/indexes/HierarchicalNavigableSmallWorld.ts b/resources/indexes/HierarchicalNavigableSmallWorld.ts index f028ff07df..4c12f45fac 100644 --- a/resources/indexes/HierarchicalNavigableSmallWorld.ts +++ b/resources/indexes/HierarchicalNavigableSmallWorld.ts @@ -961,12 +961,17 @@ export class HierarchicalNavigableSmallWorld { * This the main entry from Harper's query functionality, where we actually search for an ordered list of nearest * neighbors, using the provided sort/order definition object and performing the multi-layer skip-list search. * This returns an iterable of the nearest neighbors to the provided target vector, with nearest ordered first. + * + * This is also the contract an index implemented outside this repo has to satisfy, so everything + * optional is named: a future capability (a paging cursor, a deadline, a recall target) is a new + * field on `options` rather than a positional argument that breaks every existing implementation. * @param target * @param value * @param descending * @param distance * @param comparator * @param context + * @param options */ search( { @@ -987,16 +992,21 @@ export class HierarchicalNavigableSmallWorld { filterExpansion?: number; }, context: any, - // Predicate-aware traversal (#1241). When provided, only nodes for which `filter(primaryKey)` - // returns true are admitted to the result list at layer 0; routing is unaffected. Composed by - // search.ts from companion AND conditions and caller-supplied vector/row filters. Must be - // synchronous and side-effect free. JS-API only (never from a REST query string). - filter?: (primaryKey: Id) => boolean, - // offset + limit for a bounded query. A layer-0 search returns at most `ef` candidates, so a - // query asking for more rows than that used to come back short with no error — capped at 512 - // (AUTO_EF_MAX) however large the limit was. Raising ef to cover the request keeps `limit` - // meaningful; the caller pays for what it asked for. - minResults?: number + { + filter, + minResults, + }: { + // Predicate-aware traversal (#1241). When provided, only nodes for which `filter(primaryKey)` + // returns true are admitted to the result list at layer 0; routing is unaffected. Composed by + // search.ts from companion AND conditions and caller-supplied vector/row filters. Must be + // synchronous and side-effect free. JS-API only (never from a REST query string). + filter?: (primaryKey: Id) => boolean; + // offset + limit for a bounded query. A layer-0 search returns at most `ef` candidates, so a + // query asking for more rows than that used to come back short with no error — capped at 512 + // (AUTO_EF_MAX) however large the limit was. Raising ef to cover the request keeps `limit` + // meaningful; the caller pays for what it asked for. + minResults?: number; + } = {} ) { let limit: number | undefined; // only set for threshold comparators; 0 is a valid threshold (e.g. dotProduct) let limitInclusive = false; // true for `le`, false for `lt` diff --git a/resources/search.ts b/resources/search.ts index c1f7dcafdf..dcda46cf61 100644 --- a/resources/search.ts +++ b/resources/search.ts @@ -140,16 +140,12 @@ export function executeConditions( filtered // recordAccess intentionally omitted: guards run once, at the top level (see above). ); - return searchByIndex( - condition, - txn, - condition.descending || request.reverse === true, - table, - request.allowFullScan, + return searchByIndex(condition, txn, condition.descending || request.reverse === true, table, { + allowFullScan: request.allowFullScan, filtered, context, - request.limit !== undefined ? (request.offset || 0) + request.limit : undefined - ); + minResults: request.limit !== undefined ? (request.offset || 0) + request.limit : undefined, + }); } function mapConditionsToFilters(conditions, intersection, estimatedIncomingCount) { return conditions @@ -233,26 +229,34 @@ function composeRecordFilter(recordFilters, table, context): (primaryKey: Id) => } /** - * Search for records or keys, based on the search condition, using an index if available + * Search for records or keys, based on the search condition, using an index if available. The four + * leading parameters are required at every call site; everything optional is named, so a new + * capability can be added without shifting positions at callers that don't use it. * @param searchCondition * @param transaction * @param reverse * @param Table - * @param allowFullScan - * @param filtered + * @param options */ export function searchByIndex( searchCondition: DirectCondition, transaction: any, reverse: boolean, Table: any, - allowFullScan?: boolean, - filtered?: any, - context?: any, - // How many rows the query will ultimately consume (offset + limit), when it is bounded. An - // approximate index returns a fixed-size candidate list, so without this a query asking for more - // rows than that list holds silently gets a short result set. Only custom indexes read it. - minResults?: number + { + allowFullScan, + filtered, + context, + minResults, + }: { + allowFullScan?: boolean; + filtered?: any; + context?: any; + // How many rows the query will ultimately consume (offset + limit), when it is bounded. An + // approximate index returns a fixed-size candidate list, so without this a query asking for more + // rows than that list holds silently gets a short result set. Only custom indexes read it. + minResults?: number; + } = {} ): AsyncIterable { let attribute_name = searchCondition[0] ?? searchCondition.attribute; let value = searchCondition[1] ?? searchCondition.value; @@ -284,8 +288,7 @@ export function searchByIndex( transaction, reverse, relatedTable, - allowFullScan, - joined + { allowFullScan, filtered: joined } ); if (attribute.relationship.to) { // this is one-to-many or many-to-many, so we need to track the filtering of related entries that match @@ -302,8 +305,7 @@ export function searchByIndex( transaction, reverse, Table, - allowFullScan, - joined + { allowFullScan, filtered: joined } ); }; if (attribute.elements) { @@ -510,21 +512,23 @@ export function searchByIndex( // exploring until it has enough MATCHING results, rather than post-filtering an under-filled // candidate set. Only indexes that opt in (filteredSearch) receive it; others post-filter as before. const recordFilter = index.customIndex.filteredSearch ? searchCondition.recordFilter : undefined; - const loaded = index.customIndex.search(searchCondition, context, recordFilter, minResults).map((entry) => { - // if the custom index returns an entry with metadata, merge it with the loaded entry - if (typeof entry === 'object' && entry) { - const { key, ...otherProps } = entry; - if (key == null) return SKIP; // primaryKey missing from HNSW node — skip rather than crash - const loadedEntry = Table.primaryStore.getEntry(key, { - transaction: context && Table._readTxnForContext(context), - }); - if (!loadedEntry) return SKIP; // record was deleted/expired or not yet visible - freezeRecord(loadedEntry?.value); - recordRead(loadedEntry); - return { ...otherProps, ...loadedEntry }; - } - return entry; - }); + const loaded = index.customIndex + .search(searchCondition, context, { filter: recordFilter, minResults }) + .map((entry) => { + // if the custom index returns an entry with metadata, merge it with the loaded entry + if (typeof entry === 'object' && entry) { + const { key, ...otherProps } = entry; + if (key == null) return SKIP; // primaryKey missing from HNSW node — skip rather than crash + const loadedEntry = Table.primaryStore.getEntry(key, { + transaction: context && Table._readTxnForContext(context), + }); + if (!loadedEntry) return SKIP; // record was deleted/expired or not yet visible + freezeRecord(loadedEntry?.value); + recordRead(loadedEntry); + return { ...otherProps, ...loadedEntry }; + } + return entry; + }); if (index.customIndex.rescoreResults) { const rescored = index.customIndex.rescoreResults(loaded, searchCondition, comparator, attribute_name); if (rescored != null) return rescored as any; diff --git a/unitTests/resources/searchByIndexOptions.test.js b/unitTests/resources/searchByIndexOptions.test.js new file mode 100644 index 0000000000..e1a97baa26 --- /dev/null +++ b/unitTests/resources/searchByIndexOptions.test.js @@ -0,0 +1,53 @@ +const assert = require('node:assert'); +const { setupTestDBPath } = require('../testUtils'); +const { table } = require('#src/resources/databases'); +const { searchByIndex } = require('#src/resources/search'); +const { setMainIsWorker } = require('#js/server/threads/manageThreads'); + +// searchByIndex takes its optional tail as a named options object (#2165). These assert the options +// are actually read off that object: a caller that reverts to positional arguments would hand +// `false` in where the options belong, and destructuring it yields `allowFullScan === undefined` — +// silently turning a rejected full scan into a permitted one. +describe('searchByIndex options object (#2165)', () => { + if (process.env.HARPER_STORAGE_ENGINE === 'lmdb') return; + let T; + + before(async () => { + setupTestDBPath(); + setMainIsWorker(true); + T = table({ + table: 'SearchByIndexOptionsTest', + database: 'test', + attributes: [ + { name: 'id', isPrimaryKey: true }, + { name: 'indexedName', indexed: true }, + { name: 'unindexedName' }, + ], + }); + await T.put(1, { indexedName: 'a', unindexedName: 'x' }); + await T.put(2, { indexedName: 'b', unindexedName: 'y' }); + }); + + after(() => { + T.dropTable(); + }); + + it('rejects an unindexed attribute when allowFullScan is false in the options object', () => { + assert.throws( + () => + searchByIndex({ attribute: 'unindexedName', value: 'x' }, undefined, false, T, { + allowFullScan: false, + }), + /not indexed/, + 'allowFullScan must be read from the named option' + ); + }); + + it('keeps the permissive defaults when the options object is omitted entirely', async () => { + const ids = []; + for await (const entry of searchByIndex({ attribute: 'unindexedName', value: 'x' }, undefined, false, T)) { + ids.push(entry?.key ?? entry); + } + assert.deepStrictEqual(ids, [1], 'omitting options must full-scan, not inherit allowFullScan: false'); + }); +}); diff --git a/unitTests/resources/vectorIndex.test.js b/unitTests/resources/vectorIndex.test.js index ff029b714a..c7b6f1ca5e 100644 --- a/unitTests/resources/vectorIndex.test.js +++ b/unitTests/resources/vectorIndex.test.js @@ -809,8 +809,7 @@ describe('HNSW limit above the resolved search ef', () => { customIndex.search( { target: [1, 0, 0], comparator: 'sort' }, { transaction: undefined }, - () => true, - 100_000 // a limit far above anything the index would choose for itself + { filter: () => true, minResults: 100_000 } // a limit far above anything the index would choose for itself ); } finally { customIndex.searchLayer = originalSearchLayer; @@ -1959,7 +1958,7 @@ describeUnlessLmdbFilter('HNSW predicate-aware traversal (#1241)', () => { const results = hnsw.search( { target: [0], comparator: 'sort', descending: false }, { transaction: undefined }, - even + { filter: even } ); assert(results.length > 0, 'expected matching results'); assert( @@ -1978,7 +1977,7 @@ describeUnlessLmdbFilter('HNSW predicate-aware traversal (#1241)', () => { const results = hnsw.search( { target: [0], comparator: 'sort', descending: false }, { transaction: undefined }, - everyTenth + { filter: everyTenth } ); assert( results.every((r) => Number(r.key) % 10 === 0), @@ -1998,7 +1997,7 @@ describeUnlessLmdbFilter('HNSW predicate-aware traversal (#1241)', () => { const results = hnsw.search( { target: [0], comparator: 'sort', descending: false, ef: 10, filterExpansion: 2 }, { transaction: undefined }, - matchesNothing + { filter: matchesNothing } ); assert.strictEqual(results.length, 0, 'no matches yields an empty result, not an error'); assert(results.nodesVisited > 0, 'some nodes were visited'); @@ -2011,7 +2010,7 @@ describeUnlessLmdbFilter('HNSW predicate-aware traversal (#1241)', () => { const results = hnsw.search( { target: [0], comparator: 'sort', descending: false }, { transaction: undefined }, - even + { filter: even } ); assert.strictEqual(typeof results.nodesVisited, 'number'); assert.strictEqual(typeof results.filterEvaluations, 'number'); @@ -2027,7 +2026,7 @@ describeUnlessLmdbFilter('HNSW predicate-aware traversal (#1241)', () => { const results = hnsw.search( { target: [0], comparator: 'sort', descending: false }, { transaction: undefined }, - even + { filter: even } ); assert(!results.some((r) => r.key === 10 || r.key === 20), 'deleted nodes must not be returned'); assert( @@ -2042,7 +2041,7 @@ describeUnlessLmdbFilter('HNSW predicate-aware traversal (#1241)', () => { const results = hnsw.search( { target: [0], comparator: 'sort', descending: false }, { transaction: undefined }, - even + { filter: even } ); assert( results.every((r) => Number(r.key) % 2 === 0), From ab882571e4b050f7afda417773cd97a9d632a6a3 Mon Sep 17 00:00:00 2001 From: Maurice Morfaw Date: Mon, 17 Aug 2026 13:48:46 +1000 Subject: [PATCH 2/2] docs: update the custom-index search contract to the options-object shape (#2165) resources/DESIGN.md still described HierarchicalNavigableSmallWorld.search as (cond, ctx, filter). The third argument is now an options object. Co-Authored-By: Claude Fable 5 --- resources/DESIGN.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resources/DESIGN.md b/resources/DESIGN.md index fffde552d6..ed46fd9f43 100644 --- a/resources/DESIGN.md +++ b/resources/DESIGN.md @@ -93,7 +93,7 @@ One giant `makeTable()` factory that returns a `TableResource extends Resource` | How does a query opt out of a read snapshot? | Pass `snapshot: false` on the search request (e.g. `get_analytics`). `Table.ts → search` calls `txn.useReadTxn(snapshot === false)`; on RocksDB `DatabaseTransaction.getReadTxn` then builds the read txn with `{ disableSnapshot: true }` so a long scan reads latest without pinning a snapshot. No-op on LMDB (`LMDBTransaction.useReadTxn`). | | How does a URL path map to a Resource? | `Resources.ts → getMatch` (exact/prefix fast path) then `matchParamRoute` (parameterised routes); see "Path routing" below | | How does HNSW keep the graph connected on delete? | `indexes/HierarchicalNavigableSmallWorld.ts → index()` delete path: zero-degree orphans reindexed via `needsReindexing`; severed multi-node islands detected and reconnected by `repairSeveredNeighbors` (#1712) | -| How is a filter applied _during_ a vector search? | Predicate-aware traversal (#1241): `search.ts → executeConditions` composes companion AND conditions with request `vectorFilter` / `rowFilter` predicates into one `(primaryKey) => boolean` (`composeRecordFilter`) and passes it to `HierarchicalNavigableSmallWorld.search(cond, ctx, filter)`. The filter gates result admission at layer 0 only (routing ignores it, ACORN-style); a visit budget (`filterExpansion`) bounds the under-filled/selective case. Very selective _condition_ filters are instead diverted to the exact brute-force path by the query planner's `estimateCountAsSort` ordering. | +| How is a filter applied _during_ a vector search? | Predicate-aware traversal (#1241): `search.ts → executeConditions` composes companion AND conditions with request `vectorFilter` / `rowFilter` predicates into one `(primaryKey) => boolean` (`composeRecordFilter`) and passes it to `HierarchicalNavigableSmallWorld.search(cond, ctx, { filter })`. The filter gates result admission at layer 0 only (routing ignores it, ACORN-style); a visit budget (`filterExpansion`) bounds the under-filled/selective case. Very selective _condition_ filters are instead diverted to the exact brute-force path by the query planner's `estimateCountAsSort` ordering. | | How does post-ordering resolve vector distances safely? | Each comparator owns its `Sort`, passes it directly to the custom-index resolver, and caches distances by that immutable per-query sort object. | | How is application row filtering applied? | Authorization admission happens in the resource operation before query work. The legacy `allow*` hook, when armed by the protocol, is evaluated once with its historical receiver semantics; overriding it never changes its scope. An operation override may add indexed conditions and/or attach the JavaScript-only synchronous `target.rowFilter(record, context)`. `Table.search` composes it with query filters and rechecks the final materialized cache/source record. `SubscriptionRequest.rowFilter` covers full-row events; `eventFilter(event, context)` explicitly handles tombstones/messages/raw events. Prefer indexed conditions because an opaque predicate may inspect every admitted candidate and `limit` applies after filtering. |