Make flat search visitors query-aware - #1359
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR redesigns DiskANN’s flat (sequential) k-NN search API so flat-scan visitors become query-aware, shifting query preprocessing/distance computation into the visitor and exposing the search algorithm as a free flat::knn_search entry point (removing the thin FlatIndex wrapper).
Changes:
- Reworks the flat-search traits so
DistancesUnorderedvisitors own per-query state, andSearchStrategyconstructs a visitor using the query. - Moves the brute-force flat k-NN algorithm to
flat::knn_search(provider, ...)and updates tests accordingly. - Updates benchmark integration to call
flat::knn_searchdirectly and to use query-aware visitors.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| diskann/src/flat/test/provider.rs | Updates the test visitor/strategy to be query-aware by storing a query distance computer in the visitor. |
| diskann/src/flat/test/harness.rs | Switches the reusable harness from FlatIndex::knn_search to the free flat::knn_search function. |
| diskann/src/flat/test/cases/flat_knn_search.rs | Updates the baseline-cached regression sweep to use a shared provider (no FlatIndex). |
| diskann/src/flat/strategy.rs | Refactors core flat-search traits to make visitors query-aware and simplifies the DistancesUnordered contract. |
| diskann/src/flat/mod.rs | Updates module exports/docs to expose knn_search as the flat-search entry point. |
| diskann/src/flat/index.rs | Removes FlatIndex and implements knn_search as a borrowed-provider free function; updates flat-search tests. |
| diskann-benchmark/src/flat/search.rs | Migrates benchmark backend from FlatIndex::knn_search to flat::knn_search and updates the benchmark visitor/strategy accordingly. |
Suppressed comments (1)
diskann/src/flat/test/cases/flat_knn_search.rs:90
- This doc comment still refers to a shared
index, but the test now shares only aprovider(theFlatIndexwrapper was removed). Update the wording so it matches the new API.
/// Run `knn_search` + brute-force oracle against a *shared* `index`, assert the
/// cross-row invariants, and produce the baseline row. The per-row provider metrics
/// captured into the baseline are the *delta* observed during this row, which keeps
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1359 +/- ##
==========================================
- Coverage 91.55% 91.54% -0.02%
==========================================
Files 521 521
Lines 100371 100302 -69
==========================================
- Hits 91899 91823 -76
- Misses 8472 8479 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Aditya Krishnan (arkrishn94)
left a comment
There was a problem hiding this comment.
Thanks Junkui, I like the simplification here. Only left some minor comments, but apart from that happy to approve.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Mark Hildebrand (hildebrandmw)
left a comment
There was a problem hiding this comment.
Thanks! I've been wanting to move the graph index to a free-function style interface (taking the SearchAccessor/PruneAccessors directly) for some time now, though that will have to happen after the inmem2 transition is completely.
One thing to consider is if it makes sense here to take the Visitor directly rather than the Strategy + Provider + Query combination.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Reference Issues/PRs
Prerequisite API refactor requested during review of #1341.
What does this implement/fix? Briefly explain your changes.
Makes flat search visitors query-aware, moves the search algorithm to the free
flat::knn_searchentry point, and removes the unnecessaryFlatIndexwrapper. It also updates the generic flat tests, test providers, benchmark integration, and API rustdoc for the redesigned public API.The redesigned interface has several benefits:
DistancesUnorderednow emits(id, distance)pairs directly. Backends can fuse scanning and distance computation instead of exposing every stored element through a commonElementRefand externalQueryComputerabstraction.ElementRef,QueryComputer, andQueryComputerErrorassociated types, the visitor GAT, and their HRTB/lifetime constraints.flat::knn_search(&provider, ...)function borrows the provider directly, removing a stateless ownership wrapper and making shared providers and concurrent searches more natural.Any other comments?
This intentionally changes the existing public flat-search API and is not backwards compatible. The trade-off is a breaking migration for current callers in exchange for an interface that can naturally represent query-aware, streaming, and quantized backends. #1341 will remain open and be rebased onto
mainafter this prerequisite merges.