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% 92.53% +0.97%
==========================================
Files 521 521
Lines 100371 100313 -58
==========================================
+ Hits 91899 92822 +923
+ Misses 8472 7491 -981
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>
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.