[flat index] Flat Search Interface#983
Conversation
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (89.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #983 +/- ##
==========================================
- Coverage 89.51% 89.47% -0.05%
==========================================
Files 460 467 +7
Lines 85424 86145 +721
==========================================
+ Hits 76469 77074 +605
- Misses 8955 9071 +116
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces an RFC plus an initial “flat” (sequential scan) search surface in diskann, analogous to the existing graph/random-access search pipeline built around DataProvider/Accessor.
Changes:
- Added an RFC describing the flat iterator/strategy/index abstraction and trade-offs.
- Added a new
diskann::flatmodule withFlatIterator,FlatSearchStrategy,FlatIndex::knn_search, andFlatPostProcess(+CopyFlatIds). - Exported the new
flatmodule from the crate root.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| rfcs/00983-flat-search.md | RFC describing the design for sequential (“flat”) index search APIs. |
| diskann/src/lib.rs | Exposes the new flat module publicly. |
| diskann/src/flat/mod.rs | New module root + re-exports for the flat search surface. |
| diskann/src/flat/iterator.rs | Defines the async lending iterator primitive FlatIterator. |
| diskann/src/flat/strategy.rs | Defines FlatSearchStrategy to create per-query iterators and query computers. |
| diskann/src/flat/index.rs | Implements FlatIndex and the brute-force knn_search scan algorithm. |
| diskann/src/flat/post_process.rs | Defines FlatPostProcess and a basic CopyFlatIds post-processor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hildebrandmw
left a comment
There was a problem hiding this comment.
Thanks Aditya. Left a few general comments with some ideas on how we might improve our code sharing. In general, I'm not a fan of prefixing everything with Flat. We already have the flat module so flat::SearchStrategy reads fine to me as opposed to flat::FlatSearchStrategy, which is a little redundant.
| /// - `context`: per-request context threaded through to the provider. | ||
| /// - `query`: the query. | ||
| /// - `output`: caller-owned output buffer. | ||
| pub fn knn_search<S, T, O, OB, PP>( |
There was a problem hiding this comment.
We recently went through a whole thing of adding the Search trait to the graph index to avoid the proliferation of search methods on the index. We should probably do the same here.
There was a problem hiding this comment.
Ack, makes sense.
| Self: 'a; | ||
|
|
||
| /// The error type yielded by [`Self::next`]. | ||
| type Error: StandardError; |
There was a problem hiding this comment.
On error types, maybe consider ToRanked instead of the Into<ANNError> that StandardError implies? That said, the visitor pattern means the implementation can swallow non-critical errors on its own. So maybe not needed, for the visitor case. But on the iterator case, a ToRanked might be a good idea.
| //! family. It is designed for backends whose natural access pattern is a one-pass scan over | ||
| //! their data — for example append-only buffered stores, on-disk shards streamed via I/O, | ||
| //! or any provider where random access is significantly more expensive than sequential. | ||
| //! |
There was a problem hiding this comment.
This is a nice description comparing traits that enable algorithms based on random access vs sequential scans. Could this be in a higher level directory, either in providers.rs file or in diskann/src/agents.md
|
|
||
|
|
||
| ### The glue: `FlatSearchStrategy` | ||
|
|
There was a problem hiding this comment.
Since we are introducing substantial new machinery, can we think about whether we can reuse some of this for IVF/SPANN type of indices that rely on clustering and then scanning entire data in specific clusters to support queries.
I can imagine that the OnElementsUnordered and DistancesUnordered could be adapted to the scope of a cluster.
And SearchStrategies for IVF would need to be added.
Even if we dont have a fully fleshed our proposal for clustering based indices, it would be ideal to document how the abstractions can be reused or adapted in the near future and avoid another set of abstractions for clustering based indices
| 5. Return search stats. | ||
|
|
||
| Other algorithms (filtered, range, diverse) can be added later as additional methods on | ||
| `FlatIndex`. |
There was a problem hiding this comment.
How would we support predicates on flat index?
There was a problem hiding this comment.
it's a good point - type-1 bitmap filters can be supported by extending the scan trait (DistancesUnordered) with a typed predicate applied before computing distances. This can then be wired through at the top-level search API depending on whether we're doing brute-force or filtered search.
| //! | [`crate::provider::Accessor`] | [`FlatIterator`] | | ||
| //! | [`crate::graph::glue::SearchStrategy`] | [`FlatSearchStrategy`] | | ||
| //! | [`crate::graph::glue::SearchPostProcess`] | [`FlatPostProcess`] | | ||
| //! | [`crate::graph::Search`] | [`FlatIndex::knn_search`] | |
There was a problem hiding this comment.
This table is very useful.
Could this description be upleveled to diskann/src/agents.md or readme.
| //! # Hot loop | ||
| //! | ||
| //! Algorithms drive the scan via [`FlatIterator::next`] (lending iterator) or override | ||
| //! [`FlatIterator::on_elements_unordered`] when batching/prefetching wins. The default |
There was a problem hiding this comment.
Is this paragraph supposed to be here. Seems a bit out of place.
There was a problem hiding this comment.
Agreed, I need to clean up.
| provider::HasId, | ||
| }; | ||
|
|
||
| /// Post-process the survivor candidates produced by a flat search and |
There was a problem hiding this comment.
Is the intention to support filters with post_process? If so where is the clause?
There was a problem hiding this comment.
Post-processing for flat is now common with diskann::graph::glue::SearchPostProcess. So filter will have to live in the object implementing this trait like in the graph case - e.g. inline_beta_filter. Or it can live in the Visitor<'_> itself.
# Conflicts: # diskann/src/graph/glue.rs
| //! | ||
| //! The module mirrors the layering used by graph search: | ||
| //! | ||
| //! | Graph (random access) | Flat (sequential) | Shared? | |
There was a problem hiding this comment.
Consider adding Responsibility column and provide one-sentence description for each layer.
| pub trait DistancesUnordered<T>: OnElementsUnordered + BuildQueryComputer<T> { | ||
| /// Drive the entire scan, scoring each element with `computer` and invoking `f` with | ||
| /// the resulting `(id, distance)` pair. | ||
| fn distances_unordered<F>( |
There was a problem hiding this comment.
The algorithm only needs DistancesUnordered. Making it a supertrait of OnElementsUnordered forces all distance-capable providers to also expose raw element streaming, which leaks a lower-level capability and may block implementations that can compute distances without exposing element refs.
Could we decouple the traits and provide a blanket impl of DistancesUnordered for OnElementsUnordered + BuildQueryComputer instead:
Make DistancesUnordered independent, and provide a separate adapter/blanket impl when OnElementsUnordered is available. Conceptually:
trait DistancesUnordered<T> { fn distances_unordered(...) ... }(no supertrait)impl<T, S> DistancesUnordered<T> for S where S: OnElementsUnordered + BuildQueryComputer<T> { ... }
That keeps the algorithm-facing surface minimal, preserves encapsulation, and still keeps the convenience default behavior for types that do implement OnElementsUnordered.
There was a problem hiding this comment.
- I'm a little confused; how would we define
DistancedUnorderedwithout theBuildQueryComputertrait bound?Specifically, the computer signature in the definition ofdistances_unordered. If possible, can you flesh this out a bit? - On your suggestion, if we implement the default implementation on any
S : OnElementsUnordered + BuildQueryComputerthen we won't allow a consumer to specialize the implementation for theirSno?
I'm not sure I'm following why we might want to have implementations of DistancesUnordered that don't implement OnElementsUnordered :)
| .into_ann_result()?; | ||
|
|
||
| let computer = | ||
| BuildQueryComputer::build_query_computer(&visitor, query).into_ann_result()?; |
There was a problem hiding this comment.
Could you please provide an example of why visitor is needed to build a query computer?
There was a problem hiding this comment.
Couple reasons -
visitorimplementsDistancesUnorderedso it is not unreasonable it should know how to construct the correct type to apply the streaming distance computation.- Making the visitor implement
DistancesUnorderedhas the advantage of being symmetric to the graph case: where thesearch_accessorimplementsExpandBeamwhich must implementBuildQueryComputer<T>. This symmetry helps a bit with sharing the post-process traitSearchPostProcessfor both graph and flat search - since trait bounds are identical for both paths.
| type Id = u32; | ||
| } | ||
|
|
||
| impl provider::HasElementRef for Accessor<'_> { |
There was a problem hiding this comment.
As an option, consider extracting this prerequisite change into a separate PR to keep this one smaller and easier to review.
There was a problem hiding this comment.
Yeah, I'm open to separating this change to a pre-cursor PR.
My only worry is that, since this refactor is specific to enabling the new flat search trait structure I'm proposing here, I don't want to rush the refactor in an earlier PR only to change the flat search trait architecture.
| /// brute-force ground-truth oracle. | ||
| #[derive(Debug, Clone)] | ||
| pub(crate) struct KnnOracleRun { | ||
| /// Top-`k` `(id, distance)` pairs in canonical `(distance asc, id asc)` order. |
There was a problem hiding this comment.
Minor: shouldn't id be in the first position: (distance asc, id asc).
| /// many concurrent searches on a multi-threaded runtime, each producing the | ||
| /// correct top-k independently. | ||
| #[tokio::test(flavor = "multi_thread", worker_threads = 4)] | ||
| async fn knn_search() { |
There was a problem hiding this comment.
I would name the test something like this: flat_index_supports_multi_threading
| self.items.is_empty() | ||
| } | ||
|
|
||
| /// Distance metric the provider was constructed with. |
There was a problem hiding this comment.
Why does data provider (which responsibility is to provide data vectors) care about the distance metric?
Why is data provider defining the allowed distance metric?
| } | ||
|
|
||
| /// Snapshot of the per-provider counters. | ||
| pub fn metrics(&self) -> Metrics { |
There was a problem hiding this comment.
Minor: Metrics is a little bit confusing with Metric. Consider renaming Metrics to Counters to avoid confusion.
This PR introduces a trait interface and a light index to support brute-force search for providers that can be used as/are a flat-index. There is an associated RFC that walks through the interface and associated implementation in
diskannas a newflatmodule.Rendered RFC link.
Motivation
The repo has no first-class surface for brute-force search today. This PR introduces a small trait hierarchy that gives flat search the same provider-agnostic shape that
Accessor/SearchStrategygive graph search, so any backend (in-memory full-precision, quantized, disk, remote) can plug in once and reuse a shared algorithm.This allows implementations of algorithms - diverse search, knn search, range search, pre-filtered search - to live in the repo and let consumers only worry about defining the the data is accessed / provided; just like we do for graph search.
Important refactor
We start with an important refactor of
BuildQueryComputerand its associatedElementRef<'a>that it acts on indiskann::providers.HasElementRef- a new minimal shape traitThis zero-method traits was extracted so the streaming flat traits and the random-access
Accessorcan share their associated types without one inheriting from the other:Accessor: HasId + HasElementRef, and the flat traits below depend on the same two pieces independently.BuildQueryComputer<T>- shared query preprocessingWe split the trait in
diskann::providerto into:build_query_computermethod and theQueryComputerassociated type, and,DistancesUnorderedthat has thedistances_unorderedmethod.This split allows both graph and flat indexes to require building of a computer without dragging in random access.
Flat search - core components
The following trait and its subtrait are the core traits that define how a brute-force scan over the index is implemented.
OnElementsUnordered— the core scanImplemented in
flat/iterator.rs, this is a single (async) method that drives the entire scan via callback. Implementations choose iteration order, prefetching, and bulk reads. Algorithms only see(Id, ElementRef)pairs.flat::DistancesUnordered<T>— fused scan + scoreThis subtrait of
OnElementsUnorderedfuses scanning with scoring - it takes in aQueryComputerin addition to the closure. The default body loopson_elements_unorderedand callsevaluate_similarityon each element. Backends that can fuse retrieval with scoring can override it.The computer type comes from the implementor's own
BuildQueryComputer<T>impl, so the visitor produces the computer that drives its own scan.flat::SearchStrategy<P, T>— the glueImplemented in
flat/strategy.rs. Mirrorsgraph::glue::SearchStrategy(disambiguated by module path) and simply creates a concrete type that implements the scan:Strategies are stateless per-call config — constructed at the call site, used for one search and then dropped.
FlatIndex<P>— the top-level handleImplemented in
flat/index.rs, this is a thin'staticwrapper around aDataProvider. The search is implemented as:BuildQueryComputer::build_query_computer),visitor.distances_unordered(&computer, ...)through aNeighborPriorityQueue,graph::glue::SearchPostProcessto write into the output buffer.Note the
SearchPostProcessbound: the same trait used by graph search!FlatIterator+Iterated<I>— opt-in convenienceFor backends that naturally expose element-at-a-time access,
FlatIteratoris a lending async iterator with a singlenext().Iterated<I>wraps anyFlatIteratorand provides theOnElementsUnorderedimpl (andDistancesUnorderedby inheritance) by looping overnext()and reborrowing each element.A backend opts in at exactly the right layer: bulk-friendly backends implement
OnElementsUnordereddirectly; element-at-a-time backends implementFlatIteratorand useIteratedfor the rest.