Add algorithm and benchmark for filtered range search#1228
Add algorithm and benchmark for filtered range search#1228magdalendobson wants to merge 60 commits into
Conversation
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces filtered range search into the diskann search stack and wires it into diskann-benchmark, along with new groundtruth artifacts and integration tests. It also tightens unfiltered range-search behavior to better respect max_returned and adds regression tests to prevent future violations.
Changes:
- Added
FilteredRangesearch type todiskann(new implementation + exports) and created dedicated integration tests/baselines. - Extended
diskann-benchmark/diskann-benchmark-coreto accept a new"filtered-range"search phase and run end-to-end integration tests. - Added new (LFS-backed) range-search groundtruth files and test inputs.
Reviewed changes
Copilot reviewed 22 out of 32 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test_data/yfcc/groundtruth_rad_75000.rangeres | Adds YFCC range-search groundtruth pointer (LFS). |
| test_data/yfcc/groundtruth_rad_110000_filtered.rangeres | Adds YFCC filtered-range groundtruth pointer (LFS). |
| test_data/disk_index_search/groundtruth_rad_125000_filtered.rangeres | Adds filtered-range groundtruth pointer (LFS) for benchmark integration test data. |
| diskann/test/generated/graph/test/cases/range_search/max_results_respected_means_no_second_round.json | New baseline for range-search max_returned behavior. |
| diskann/test/generated/graph/test/cases/range_search/max_results_respected_and_second_round_triggered.json | New baseline for second-round triggering when max_returned allows it. |
| diskann/test/generated/graph/test/cases/filtered_range_search/two_round_search.json | New filtered-range baseline. |
| diskann/test/generated/graph/test/cases/filtered_range_search/max_results_respected_means_no_second_round.json | New filtered-range baseline. |
| diskann/test/generated/graph/test/cases/filtered_range_search/max_results_respected_and_second_round_triggered.json | New filtered-range baseline. |
| diskann/test/generated/graph/test/cases/filtered_range_search/inner_radius_filtering.json | New filtered-range baseline for inner radius behavior. |
| diskann/test/generated/graph/test/cases/filtered_range_search/divisible_by_four_filter_second_round_triggered.json | New filtered-range baseline for selective filter + second round. |
| diskann/test/generated/graph/test/cases/filtered_range_search/divisible_by_four_filter_no_second_round_from_max_results.json | New filtered-range baseline for selective filter + max_returned interaction. |
| diskann/test/generated/graph/test/cases/filtered_range_search/divisible_by_four_filter_no_second_round_from_l_search.json | New filtered-range baseline for selective filter + larger starting_l. |
| diskann/test/generated/graph/test/cases/filtered_range_search/basic_range_search.json | New filtered-range baseline for basic search. |
| diskann/src/graph/test/cases/range_search.rs | Exposes shared helpers/baseline struct and adds new max_returned regression tests. |
| diskann/src/graph/test/cases/mod.rs | Registers new filtered-range test module. |
| diskann/src/graph/test/cases/filtered_range_search.rs | Adds integration tests and baselines for filtered range search. |
| diskann/src/graph/search/range_search.rs | Ensures second round + frontier expansion honor max_returned. |
| diskann/src/graph/search/mod.rs | Adds filtered-range module and re-exports FilteredRange. |
| diskann/src/graph/search/inline_filter_search.rs | Makes internal return type/function pub(crate) for reuse by filtered-range search. |
| diskann/src/graph/search/filtered_range_search.rs | Implements the new FilteredRange search type and its internal second-round expansion. |
| diskann/src/graph/glue.rs | Adds PartialEq/Eq derives for Decision<T>. |
| diskann-benchmark/src/main.rs | Adds benchmark integration tests for range and filtered-range JSON examples. |
| diskann-benchmark/src/inputs/graph_index.rs | Adds "filtered-range" search phase schema + validation. |
| diskann-benchmark/src/index/search/range.rs | Generalizes range runner to support both range and filtered-range backends via parameter mapping. |
| diskann-benchmark/src/index/search/plugins.rs | Adds filtered-range plugin entry. |
| diskann-benchmark/src/index/result.rs | Adds RangeSearchResults::new_filtered constructor for filtered-range summaries. |
| diskann-benchmark/src/index/inmem/spherical.rs | Wires filtered-range plugin into spherical in-mem benchmark path. |
| diskann-benchmark/src/index/benchmarks.rs | Wires filtered-range plugin into benchmark registry + job runner. |
| diskann-benchmark/example/graph-index-filter-range.json | Adds example job config for "filtered-range". |
| diskann-benchmark-core/src/search/mod.rs | Documents filtered-range support. |
| diskann-benchmark-core/src/search/graph/mod.rs | Exposes new filtered_range module. |
| diskann-benchmark-core/src/search/graph/filtered_range.rs | Adds benchmark-core searcher + aggregation plumbing for filtered-range. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| InternalSearchStats { | ||
| cmps, | ||
| hops: hops + range_stats.hops, | ||
| range_search_second_round: true, | ||
| }, |
| //! Tests for filtered range search using an always-true filter. | ||
| //! | ||
| //! These cover the filtered-range cases directly and validate the filtered-range | ||
| //! behavior against its own baselines. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1228 +/- ##
==========================================
+ Coverage 89.64% 90.36% +0.71%
==========================================
Files 503 510 +7
Lines 95761 97751 +1990
==========================================
+ Hits 85845 88330 +2485
+ Misses 9916 9421 -495
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hildebrandmw
left a comment
There was a problem hiding this comment.
Thanks Magdalen. I've left a number of comments throughout the PR, but they all fall into a few general themes.
- This PR duplicates a lot of logic when that duplication should be abstracted instead. The worst offender is probably
DistanceFiltered. - We need to move away from messing around with the internal of scratch space (
in_range,range_frontier,visitedetc) - that can quickly become very brittle. - The logic in the filtered search implementation can be tightened.
- We should start tagging baselines with more verbose descriptions of what the test is covering and what the corner cases being exercised are.
- Please keep to high level trait objects in
diskann-benchmarkinstead of generics. The former is significantly better for compile times.
In addition, is the intention of this algorithm for research, or is there a production ask for it? I ask because maintaining such algorithms/code is not free. If this is meant for research, please put it behind an experimental feature flag to avoid committing us to it long term.
|
|
||
| #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] | ||
| pub(super) struct RangeSearchBaseline { | ||
| pub(super) grid_size: usize, |
There was a problem hiding this comment.
Request: can you add a "description" field to describe what's being tests and what the expected results are? Something like this.
We want to help future developers a little.
|
|
||
| let stats = if scratch.in_range.len() | ||
| >= ((self.starting_l() as f32) * self.initial_slack()) as usize | ||
| && scratch.in_range.len() < self.max_returned().unwrap_or(usize::MAX) |
There was a problem hiding this comment.
Can this bug fix be split out into its own little PR?
There was a problem hiding this comment.
Done, I split this and the new tests out to #1253
| merged | ||
| .entry(neighbor.id) | ||
| .and_modify(|best: &mut Neighbor<_>| { | ||
| if neighbor.distance < best.distance { |
There was a problem hiding this comment.
What's happening here? Why are we expecting repeats for IDs and why is picking the best the right choice?
There was a problem hiding this comment.
Overwriting the distance was an error and I've gotten rid of it now. We are expecting repeats because we are merging the best elements found so far with all the filter-satisfying elements found so far, which might have overlap.
| in_range.sort_unstable_by(|left, right| { | ||
| left.distance | ||
| .total_cmp(&right.distance) | ||
| .then_with(|| left.id.cmp(&right.id)) |
There was a problem hiding this comment.
Don't Neighbors already sort themselves?
There was a problem hiding this comment.
No, because scratch.best is merged with the predicate-satisfying points, which are not in sorted order.
| average_precision, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Please add tests like all the other entries in search/graph/
| } | ||
|
|
||
| /// Create range search with full options. | ||
| #[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
We should probably listen to clippy here. This is a lot of arguments, and while it mirrors Range - I don't think that's sufficient justification. There are several problems:
- There's no real documentation about how these are expected to affect the algorithm.
- Callers of
FilteredRange::with_optionsjust have a sea of values, which can be difficult to remember. - Any changes to the parameters requires a breaking change for all callers.
It would be way better to use a builder interface for these. As a side-benefit, the same builder could be used for both Range and FilteredRange.
There was a problem hiding this comment.
Thanks, I set up a builder
| PP: glue::SearchPostProcess<S::SearchAccessor, T, O> + Send + Sync, | ||
| OB: SearchOutputBuffer<O> + Send + ?Sized, | ||
| { | ||
| async move { |
There was a problem hiding this comment.
This and filtered_range_search_internal need at least one clean up pass, if not more. The intermediate storage states are messy and often redundant.
Several things I notices:
- The computation
self.range() * self.range_slack()happens often, especially within loops. Should this not be a pre-computed value? - We may be able to skip
mergedentirely with a vec + sort + dedup - I don't know what's happening with
scratch.in_range. It's overwritten inscratchsimply to pass it as an argument tofiltered_range_search_internal(this could be completely avoided and likely save an allocation). Further, it is mutated infiltered_range_search_internalbut that mutation is never observed. So why is it being pushed to? - The initialization of
scratch.range_frontieris sketchy - it's really relying on the pooled variable being cleared properly to prevent garbage. - Many repetitions of the pattern
self.max_returned().unwrap_or(usize::MAX), which presumably is related to the bug fix inrange_search. Perhaps this should be a dedicated helper or done just once?
Basically, there are a lot of little things and implicit coupling that could do with a solid scrutinization. Much of this is inherited from range search, but many of my complaints also hold for range search; newer code should be held to a higher standard.
There was a problem hiding this comment.
Thanks, I believe I have fixed most of these now. I have:
- Removed computation
self.range() * self.range_slack()in several places. In some cases it was being used mistakenly, because we don't test forrange_slack > 1.0sufficiently (at all). I will follow up with another PR with tests for this value. - We now use a vec + sort + dedup to gather all points within the radius
- I got rid of use of
scratch.in_rangeand now directly modify onlyscratch.visitedandscratch.range_frontier, which I now properly clear and then initialize - pattern
self.max_returned().unwrap_or(usize::MAX)is now called just twice, once in each search function
| /// | ||
| /// Both variants carry the item `T` since rejected items are useful for graph navigation. | ||
| #[derive(Debug, Clone, Copy)] | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] |
There was a problem hiding this comment.
Why are PartialEq and Eq needed? The code compiles fine without them.
There was a problem hiding this comment.
I think this was leftover from an intermediate state. Removed now.
| let (filtered_stats, filtered_results) = | ||
| run_filtered_range_search(&index, query.as_slice(), filtered_range, &filter); | ||
|
|
||
| let baseline = RangeSearchBaseline { |
There was a problem hiding this comment.
There's probably a better way to construct these baselines. A constructor taking the stats object and the Range could automatically fill in a lot of the fields (and we could capture all the fields of `Range~ rather then just like 3).
There was a problem hiding this comment.
Thanks for the suggestion, I added a constructor
| pub(crate) fn run<I>( | ||
| runner: &dyn Range<I>, | ||
| pub(crate) fn run<I, R, F>( | ||
| runner: &R, |
There was a problem hiding this comment.
Why did this change from a &dyn Range to a true generic? The former is quite important to keep compile times down.
Presumably it's because there's a different in the parameters being passed. Please find an alternate solution that isn't as regressive on compile times.
There was a problem hiding this comment.
I found an alternative way to do this, so the generic isn't used anymore
While working on #1228, I noticed that there were some issues with our current implementation of range search and its testing. The main issue with testing is that there were no tests ensuring the `max_results` parameter was respected. I have added two tests that ensure this now. The main code had several issues with how `max_results` was handled: 1. The `max_results` parameter was allowed to be less than the initial L_search. This is a conceptual issue because the user expects `max_results` to stop the search from continuing for too long, and the compute used in the initial search will always be controlled by `initial_search_l`. 2. A `max_results` check was not enforced before deciding to continue to the second round search. This meant that if the max results was reached via the initial search, it might not be respected. 3. The second round search was not terminated when `max_results` was reached, meaning it would continue to perform unnecessary work. This PR fixes these issues by adding additional checks of `max_results` at the correct points in the code. --------- Co-authored-by: Magdalen Manohar <mmanohar@microsoft.com>
fff67b6 to
e30697f
Compare
This PR adds an algorithm for filtered range search to the DiskANN repository. Details of the algorithm, along with the experiments supporting it, are at this Wiki page.
This PR:
diskann, along with integration tests.diskann-benchmark, along with an integration test.test_data.Some enhancements/fixes to range search along the way:
test_datafor yfcc.