Skip to content

Simplify the DataProvider contract for graph search#1067

Draft
hildebrandmw wants to merge 5 commits into
mainfrom
mhildebr/experiment
Draft

Simplify the DataProvider contract for graph search#1067
hildebrandmw wants to merge 5 commits into
mainfrom
mhildebr/experiment

Conversation

@hildebrandmw
Copy link
Copy Markdown
Contributor

@hildebrandmw hildebrandmw commented May 14, 2026

Executive Summary: What happens if we start simplifying our trait hierarchy and rely on ExpandBeam to do the heavy lifting?

This is controversial and I have no intention (yet) of turning this into a full PR, but it demonstrates the feasibility of something I've come to realize recently. The whole Accessor -> BuildQueryComputer -> ExpandBeam originated from earlier patterns in the library of building up indexing algorithms from tiny pieces, enabling a user to implement a small set of methods and get the whole indexing algorithm for free.

This breaks down, though, when we consider performance: the more information we provide a backend database about the operation we want to perform, the better the database can optimize. This lead to on_elements_unordered (bulk element retrieval, allowing for example prefetching), distances_unordered (bulk retrieval with a concrete distance computations), and finally ExpandBeam. Of these, the latter has proved by far the most useful, being used by diskann-disk and diskann-garnet to specialize the algorithm to their preferred data access pattern.

I've been exploring how to make filtered search more of a first-class citizen to allow us to batch together predicate queries and potentially fuse predicate evaluation with data access. I was halfway through writing a parallel Accessor/Computer/ExpandBeam/Strategy hierarchy for predicate aware search (and struggling with expressing the more complicated type relationships required), when I realized that all we really needed were a few slightly different flavors of ExpandBeam. Instead of building a complex nest of interrelated traits to express these subtly different ExpandBeam methods, why not just require users to implement the necessary ExpandBeam? The required semantics are not that complicated and it gives the implementor full control of the data access.

This PR is an experiment in removing all but the ElementRef GAT of the Accessor trait and instead making ExpandBeam the most relevant method. There's more simplification that can be done: fusing SearchExt with ExpandBeam, getting rid of the pesky AsNeighbor bound from ExpandBeam, simplifying BuildQueryComputer to maybe not even need the PreprocessedDistanceFunction bound. However, this is already a large reduction in code and has a couple extra benefits:

  • The ExpandBeam implementations for inmem are actually more efficient than the previous provided implementations because we can more aggressively evaluate the visited predicate (and merge the neighbor buffer in the prefetch id_buffer to save an allocation on each ExpandBeam call. This is not a significant increase in code because we simply move the old on_elements_unordered.
  • We can drop a bunch of test code from diskann testing the provided implementations that are now no longer needed.
  • If we merge ExpandBeam with SearchExt, then all the relevant bits for search are grouped in a single place instead of spread out for easier discovery.
  • We can shed a bunch of complexity in diskann, with potentially more simplification if we continue down this path.
  • Users need to implement fewer traits, and the implementation of ExpandBeam is perhaps a little more prone to error. This is a distinct negative, but if everything is collected into a single place, it's easier to audit.

Now for some tradeoffs:

  • We don't have vocabulary to implement the flat_search function. I kind of view this as a bonus: the existing flat search relies on an ID iterator (not easy for most backends) and uses an inefficient loop. The call in diskann-disk would likely be much more efficient if it was just done directly on the PQ dataset.
  • This substantially breaks the design in diskann-label-filter. Coarsening the granularity of data access methods means the wrapping done by the label provider is less effective. Again, though, this is probably a feature: since real implementations are moving towards ExpandBeam for performance, the label provider may not be a good approach anyways. We certainly had this experience in Remove the Caching Provider #1052.
  • There is still some friction in test code that is relying on Accessor's get-element. This can be grouped into the larger diskann-providers cleanup.
  • The provided implementation of Fill for diskann::graph::workingset::Map is no longer applicable. Users have to write their own relatively easy Fill method. I don't think this is as big of a problem as it sounds because I don't think any serious implementation uses the provided implementation: it's too slow.

Why is this relevant now? Aside from the filtered search work, #983 is working towards adding flat search and introduces an even finer granularity of traits in the hierarchy, when my gut tells me we should be moving the other direction towards slightly coarser traits. Also, if we have an opportunity to simplify our codebase, I think we should take it.

Why is this possible now? #859 moved one of the last big get_element related operations behind a bulk operation. Surprisingly, that removed all calls to the raw get_element from insert or inplace delete.

@metajack
Copy link
Copy Markdown
Contributor

get_element is basically unused in garnet, except for implementing VEMB (return an vector) and other debugging-style functions.

This sounds like a very promising direction, and I am very supportive.

@hildebrandmw hildebrandmw changed the title [DO NOT MERGE] Delete all the things! Simplify the DataProvider contract for graph search May 15, 2026
hildebrandmw added a commit that referenced this pull request May 16, 2026
Remove `flat_search` from `DiskANNIndex` and the `IdIterator` trait from
`diskann`. Since the only caller was from `diskann-disk`, add a new
`flat_search` inherent method to `DiskIndexSearcher`.

The flat search method is not compatible with the experimental direction
in #1067 and with #983 on the horizon, this is safe to move for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants