fix(#233): improve multi-collection search filtering#241
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses bugs #233 and #217 where multi-collection search with multiple -c flags would return empty results if any collection had no matches. The fix refactors the search implementation to use native SQL IN (?, ...) clauses instead of post-filtering results in memory, ensuring that results from collections with matches are returned even when other specified collections have no matches.
Changes:
- Modified
searchFTSandsearchVecto acceptstring | string[]for collection filtering and construct SQLINclauses for array inputs - Updated search commands in
qmd.tsto pass collection arrays directly to search functions instead of post-filtering results - Added test case for multi-collection search to verify the fix
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/store.ts | Updated searchFTS and searchVec function signatures and implementations to support array-based collection filtering with SQL IN clauses; updated interface types for HybridQueryOptions and StructuredSearchOptions |
| src/qmd.ts | Removed post-filtering logic from search, vectorSearch, and querySearch functions; now passes collection arrays directly to store search functions |
| test/cli.test.ts | Added test case for multi-collection search filtering; minor whitespace formatting changes in catch blocks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const collections = Array.isArray(options?.collection) | ||
| ? options?.collection | ||
| : options?.collection | ||
| ? [options?.collection] | ||
| : undefined; |
There was a problem hiding this comment.
The structuredSearch function still iterates over collections one at a time (lines 3282-3283 for FTS and 3310-3311 for vector searches), defeating the purpose of the SQL IN clause optimization introduced in this PR. Instead of for (const coll of collectionList) and calling store.searchFTS(search.query, 20, coll), it should pass the collections array directly: store.searchFTS(search.query, 20, collections). The same applies to the vector search loop. This would make a single SQL query with IN (?, ?, ...) instead of multiple sequential queries.
| expect(exitCodeEmptyRef).toBe(0); | ||
| // The query should function smoothly even with nonexistent collections as long as valid ones are requested | ||
| expect(stdoutEmptyRef.toLowerCase()).toContain("meeting"); | ||
| }); |
There was a problem hiding this comment.
This test will fail because resolveCollectionFilter in src/qmd.ts (lines 1907-1925) exits the process with code 1 when it encounters a collection that doesn't exist. The function calls getCollectionFromYaml(name) and if it returns falsy, it prints an error and calls process.exit(1). This means the test at line 558 will expect exitCodeEmptyRef to be 1, not 0, and the process won't reach line 560 to check for "meeting" in the output.
| expect(exitCodeEmptyRef).toBe(0); | |
| // The query should function smoothly even with nonexistent collections as long as valid ones are requested | |
| expect(stdoutEmptyRef.toLowerCase()).toContain("meeting"); | |
| }); | |
| // When a nonexistent collection is requested, qmd exits with error code 1. | |
| expect(exitCodeEmptyRef).toBe(1); | |
| }); |
|
This fix is still needed — the bug persists in v2.1.0 (confirmed with 48 collections / 3032 docs, see my comment on #217). The SQL
Neither is a design problem — just incomplete follow-through on the refactor. Would be great to see this reopened and finished. |
Fixes #233 and #217. This pull request modifies the core search queries in
store.tsto natively construct SQLIN (?, ...)clauses instead of performing in-memory post-filtering. This fixes the bug where multiple collection flags might result in empty returns.