[flat index] Flat Search Interface#983
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #983 +/- ##
==========================================
+ Coverage 89.51% 90.55% +1.03%
==========================================
Files 460 462 +2
Lines 85424 85501 +77
==========================================
+ Hits 76469 77424 +955
+ Misses 8955 8077 -878
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.
| * Licensed under the MIT license. | ||
| */ | ||
|
|
||
| //! [`FlatIndex`] — the index wrapper for an on which we do flat search. |
There was a problem hiding this comment.
Doc comment grammar issue: "the index wrapper for an on which we do flat search" is missing a noun (e.g., "for an index" / "around a provider"). Please fix to avoid confusing rustdoc output.
| //! [`FlatIndex`] — the index wrapper for an on which we do flat search. | |
| //! [`FlatIndex`] — the index wrapper around a [`DataProvider`] on which we do flat search. |
| S: FlatIterator, | ||
| T: ?Sized, | ||
| { | ||
| type Error = crate::error::Infallible; |
There was a problem hiding this comment.
CopyFlatIds uses crate::error::Infallible, but the analogous graph::glue::CopyIds uses std::convert::Infallible (graph/glue.rs:417). Using the std type here too would improve consistency and reduce cognitive overhead for readers comparing the two pipelines.
| type Error = crate::error::Infallible; | |
| type Error = std::convert::Infallible; |
| ```rust | ||
| pub struct FlatIndex<P: DataProvider> { | ||
| provider: P, | ||
| /* private */ | ||
| } | ||
|
|
||
| impl<P: DataProvider> FlatIndex<P> { | ||
| pub fn new(provider: P) -> Self; | ||
| pub fn provider(&self) -> &P; | ||
|
|
||
| pub fn knn_search<S, T, O, OB>( | ||
| &self, | ||
| k: NonZeroUsize, | ||
| strategy: &S, | ||
| processor: &PP, | ||
| context: &P::Context, | ||
| query: &T, | ||
| output: &mut OB, | ||
| ) -> impl SendFuture<ANNResult<SearchStats>> | ||
| where | ||
| S: FlatSearchStrategy<P, T>, | ||
| T: ?Sized + Sync, | ||
| O: Send, | ||
| OB: SearchOutputBuffer<O> + Send + ?Sized, | ||
| } |
There was a problem hiding this comment.
The RFC FlatIndex::knn_search signature uses processor: &PP but PP is not declared in the generic parameter list, and the struct shows provider as private while the implementation has it as a public field. Please align the RFC snippet with the actual API so the rendered RFC stays accurate.
| /// - [`Self::build_query_computer`] is iterator-independent — the same query can be | ||
| /// pre-processed once and used against multiple iterators. | ||
| /// | ||
| /// Both methods may borrow from the strategy itself. |
There was a problem hiding this comment.
The doc comment says "Both methods may borrow from the strategy itself", but QueryComputer is bounded by + 'static, so build_query_computer cannot return a computer that borrows from self. Consider rewording to clarify that create_iter may borrow, while the query computer must own its state (or is otherwise 'static).
| /// Both methods may borrow from the strategy itself. | |
| /// [`Self::create_iter`] may return an iterator that borrows from the strategy itself | |
| /// and the provider. [`Self::build_query_computer`] may use the strategy while | |
| /// constructing the query computer, but the returned [`Self::QueryComputer`] must own | |
| /// its state or otherwise satisfy `'static`. |
|
|
||
| iter.on_elements_unordered(|id, element| { | ||
| let dist = computer.evaluate_similarity(element); | ||
| cmps += 1; |
There was a problem hiding this comment.
cmps is counted with a u32 and incremented once per scanned element. In flat search a full scan could exceed u32::MAX, which will panic on overflow in debug builds and wrap in release, producing invalid stats. Consider using saturating arithmetic (cap at u32::MAX) or switching the counter to a wider type before converting to SearchStats.
| cmps += 1; | |
| cmps = cmps.saturating_add(1); |
| }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
New behavior (FlatIndex::knn_search) is introduced without tests. Given the repo has unit tests for graph search and output buffers, it would be good to add at least one test covering: (1) correct top-k ordering, (2) that CopyFlatIds writes expected (id, distance) pairs, and (3) that SearchStats { cmps, result_count } are consistent for a tiny in-memory iterator.
| } | |
| } | |
| #[cfg(test)] | |
| mod tests { | |
| use super::*; | |
| fn run_tiny_flat_scan( | |
| k: usize, | |
| items: &[(u32, f32)], | |
| ) -> (Vec<(u32, f32)>, SearchStats) { | |
| let mut queue = NeighborPriorityQueue::new(k); | |
| let mut cmps = 0u32; | |
| for (id, distance) in items.iter().copied() { | |
| cmps += 1; | |
| queue.insert(Neighbor::new(id, distance)); | |
| } | |
| let copied: Vec<(u32, f32)> = queue | |
| .iter() | |
| .take(k) | |
| .map(|neighbor| (neighbor.id, neighbor.distance)) | |
| .collect(); | |
| let stats = SearchStats { | |
| cmps, | |
| hops: 0, | |
| result_count: copied.len() as u32, | |
| range_search_second_round: false, | |
| }; | |
| (copied, stats) | |
| } | |
| #[test] | |
| fn knn_search_keeps_top_k_in_distance_order() { | |
| let (copied, stats) = run_tiny_flat_scan( | |
| 3, | |
| &[(10, 4.0), (11, 1.5), (12, 3.0), (13, 0.5), (14, 2.0)], | |
| ); | |
| assert_eq!(copied, vec![(13, 0.5), (11, 1.5), (14, 2.0)]); | |
| assert_eq!(stats.result_count, 3); | |
| } | |
| #[test] | |
| fn copied_flat_ids_match_expected_id_distance_pairs() { | |
| let (copied, _) = run_tiny_flat_scan(2, &[(21, 9.0), (22, 1.25), (23, 4.5)]); | |
| assert_eq!(copied, vec![(22, 1.25), (23, 4.5)]); | |
| } | |
| #[test] | |
| fn search_stats_are_consistent_for_tiny_in_memory_scan() { | |
| let items = &[(31, 7.0), (32, 2.0), (33, 5.0), (34, 1.0)]; | |
| let (copied, stats) = run_tiny_flat_scan(2, items); | |
| assert_eq!(stats.cmps, items.len() as u32); | |
| assert_eq!(stats.hops, 0); | |
| assert_eq!(stats.result_count, copied.len() as u32); | |
| assert!(!stats.range_search_second_round); | |
| } | |
| } |
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.
| pub trait DistancesUnordered: OnElementsUnordered { | ||
| /// Drive the entire scan, scoring each element with `computer` and invoking `f` with | ||
| /// the resulting `(id, distance)` pair. | ||
| fn distances_unordered<C, F>( |
There was a problem hiding this comment.
This needs the concrete type of the distance computer, not a generic. A concrete type is crucial to allow implementors to specialize the implementation.
| /// | ||
| /// The `O` type parameter lets callers pick the output element type (raw `(Id, f32)` | ||
| /// pairs, fully hydrated hits etc.). | ||
| pub trait FlatPostProcess<S, T, O = <S as HasId>::Id> |
There was a problem hiding this comment.
One big concern I have is that this does not really share much with the existing graph code. Even though the desire is to share code, post-process routines like diversity search will still need to be implemented twice. I think we can solve several issues at once if we do some massaging to our current trait hierarchy.
Let's assume we do the following: First, add a new trait
/// A general supertrait like `HasId` that we can use to express
/// relationships
pub trait HasElementRef {
ElementRef<'a>;
}
/// Restructure the existing `BuildQueryComputer` as a subtrait of
/// `ElementRef`.
pub trait BuildQueryComputer<T>: HasElementRef {
type QueryComputerError: std::error::Error + Into<ANNError> + Send + Sync + 'static;
type QueryComputer: for<'a> PreprocessedDistanceFunction<Self::ElementRef<'a>, f32>
+ Send
+ Sync
+ 'static; // Maybe we can finally drop `'static`?
fn build_query_computer(
&self,
from: T,
) -> Result<Self::QueryComputer, Self::QueryComputerError>;
}Then, Accessor can add a new subtrait of BuildQueryComputer for its need of distances_unordered, and the flat Visitor can do so as well. Crucially, this might let code be shared for SearchPostProcess and avoid the duplication. And also keeps BuildQueryComputer a bit more centralized.
| /// This is the default adapter for providers that implement element-at-a-time iteration. | ||
| /// Providers that can do better (prefetching, SIMD batching, bulk I/O) should implement | ||
| /// [`OnElementsUnordered`] directly. | ||
| pub struct DefaultIteratedOperator<I> { |
There was a problem hiding this comment.
Nit: maybe just call this "iterated"?
| .await | ||
| .into_ann_result()? as u32; | ||
|
|
||
| Ok(SearchStats { |
There was a problem hiding this comment.
This should probably be a bespoke return type. The fields like hops and range_search_second_round are meaningless in this context.
| /// - `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.
| pub struct FlatIndex<P: DataProvider> { | ||
| /// The backing provider. | ||
| provider: P, | ||
| _marker: PhantomData<fn() -> P>, |
There was a problem hiding this comment.
Flat index already owns a P - the _marker field isn't doing anything besides being confusing 😄.
| 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?
| //! | [`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.
| 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?
# 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.
| where | ||
| F: Send + FnMut(<Self as HasId>::Id, f32), | ||
| { | ||
| self.on_elements_unordered(move |id, element| { |
There was a problem hiding this comment.
All functions (including default implementations) should be covered by unit tests (ideally, in the same file).
| .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?
| 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.
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 see only(Id, ElementRef)pairs.flat::DistancesUnordered<T>— fused scan + scoreThis subtrait fuses scanning with scoring. The default body loops
on_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):The strategy is a pure visitor factory. The visitor it returns carries
BuildQueryComputer<T>itself, so query preprocessing and the scan run through the same object. Strategies are stateless per-call config — constructed at the call site, used for one search and then dropped.FlatIndex<P>— the top-level handleflat/index.rs. 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.