Make Neighbor less incorrect - #1273
Open
hildebrandmw wants to merge 3 commits into
Open
Conversation
added 2 commits
July 23, 2026 17:32
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors diskann::neighbor::Neighbor into a tuple-like aggregate with private fields and removes its problematic Ord/Eq semantics, replacing them with explicit comparator functions in neighbor::ord so algorithms can choose the intended ordering (and make NaN behavior easier to document).
Changes:
- Simplifies
Neighbor(privateid/distance, accessors, tuple conversion) and moves ordering toneighbor::ord::*comparators. - Updates search, postprocess, and test code across crates to use
Neighbor::id(),Neighbor::distance(),Neighbor::as_tuple(), andneighbor::ordcomparators for sorting. - Improves test diagnostics by using
assert_eq_verbose!/VerboseEqin several updated test assertions.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| diskann/src/test/cmp.rs | Minor update in verbose_eq! macro expansion formatting. |
| diskann/src/neighbor/queue.rs | Updates fixed-capacity fast-path to compare by distance() instead of relying on Neighbor ordering. |
| diskann/src/neighbor/mod.rs | Core refactor: tuple-like Neighbor, accessors, VerboseEq (tests), new ord comparator module, and updated tests. |
| diskann/src/graph/test/provider.rs | Updates deletion filtering to use n.id() accessor. |
| diskann/src/graph/test/cases/range_search.rs | Updates range-search test helpers and baselines to use accessors/as_tuple(). |
| diskann/src/graph/test/cases/paged_search.rs | Updates paged-search invariants/baselines to use accessors/as_tuple(). |
| diskann/src/graph/test/cases/multihop.rs | Updates multihop tests to use id() accessor. |
| diskann/src/graph/search/record.rs | Updates visited-id extraction and recall accounting to use id() accessor. |
| diskann/src/graph/search/range_search.rs | Updates range search logic/tests to use distance()/id() accessors. |
| diskann/src/graph/search/paged.rs | Updates start-point filtering to use id() accessor. |
| diskann/src/graph/search/multihop_filter_search.rs | Switches sorts to neighbor::ord::fast_distance and updates ID extraction via id(). |
| diskann/src/graph/search/inline_filter_search.rs | Switches matched-results sorting to neighbor::ord::fast_distance and updates ID extraction via id(). |
| diskann/src/graph/internal/sorted_neighbors.rs | Replaces sort_unstable/select_nth_unstable ordering with neighbor::ord::fast_distance to keep behavior explicit. |
| diskann/src/graph/index.rs | Updates various ID extraction points and replaces sort_unstable() with sort_unstable_by(neighbor::ord::fast_distance). |
| diskann/src/graph/glue.rs | Uses Neighbor::as_tuple() for SearchOutputBuffer::extend and updates start-point filtering via id(). |
| diskann/src/flat/test/harness.rs | Removes bespoke neighbor sorting logic and uses neighbor::ord::fast_distance_total; updates tuple mapping/filtering via accessors. |
| diskann-tools/src/utils/ground_truth.rs | Replaces struct-literal neighbor construction and field access with Neighbor::new/accessors. |
| diskann-providers/src/test_utils/search_utils.rs | Updates sorting to neighbor::ord comparators and switches distance/id reads to accessors. |
| diskann-providers/src/model/graph/provider/async_/postprocess.rs | Updates deletion filtering and output extension to use accessors/as_tuple(). |
| diskann-providers/src/model/graph/provider/async_/inmem/full_precision.rs | Updates deletion filtering, vector reads, and extraction of ids/distances to use accessors. |
| diskann-providers/src/index/wrapped_async.rs | Updates assertions to use id()/distance() accessors. |
| diskann-providers/src/index/diskann_async.rs | Updates test sorting to neighbor::ord and replaces direct field mutation/access with accessor-based rebuilding. |
| diskann-label-filter/src/inline_beta_search/inline_beta_filter.rs | Updates attribute lookup to use id() accessor and avoids reconstructing neighbors unnecessarily. |
| diskann-garnet/src/provider.rs | Updates external ID mapping and push logic to use id()/distance() accessors. |
| diskann-disk/src/search/provider/disk_provider.rs | Updates candidate-id extraction to use id() accessor. |
| diskann-bftree/src/provider.rs | Updates vector reads and test assertions to use id() accessor. |
| diskann-benchmark/src/exhaustive/algos.rs | Updates output ID fill to use id() accessor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1273 +/- ##
==========================================
- Coverage 91.50% 90.47% -1.03%
==========================================
Files 498 497 -1
Lines 95522 95548 +26
==========================================
- Hits 87407 86447 -960
- Misses 8115 9101 +986
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
harsha-simhadri
approved these changes
Jul 27, 2026
| /// | ||
| /// An exceedingly common algorithmic operation is to sort [`Neighbor`]s according to some | ||
| /// protocol. Usually, this is by distance (from smallest to largest) ignoring the ID entirely, | ||
| /// but certain situations call for different orderings. This raises two problems: |
Contributor
There was a problem hiding this comment.
could we capture these other orderings currently used here. Would nice to have a reference
Contributor
Author
There was a problem hiding this comment.
The idea behind this PR is to move the orderings into the ord submodule, which then serves as kind of an "automatic" collection of the various orderings. As a bonus, the functions names like fast_distance are pretty easy to grep, making it easier to locate where these are being used. Is the ask here to also record when other recordings are used?
JordanMaples
approved these changes
Jul 27, 2026
JordanMaples
approved these changes
Jul 27, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
diskann/src/neighbor/mod.rs:177
Ordering::then(...)eagerly evaluates the ID comparison even when distances differ. Usingthen_withavoids unnecessaryOrd::cmpwork in this hot comparator.
fast_distance(x, y).then(x.id().cmp(y.id()))
diskann/src/neighbor/mod.rs:155
- This doc comment says behavior is unspecified for NaNs, but
fast_distance_totaldelegates tofast_distanceand is used as asort_bycomparator. After makingfast_distancetotal (e.g., viatotal_cmp), this should be updated to avoid misleading users.
/// This is a fast, semi-approximate method whose behavior is unspecified when either
/// distance is [`f32::NAN`].
| macro_rules! verbose_eq { | ||
| ($struct:path { $($fields:ident),+ $(,)? }) => { | ||
| impl $crate::test::cmp::VerboseEq for $struct { | ||
| impl $crate::test::cmp::VerboseEq for $struct { |
|
|
||
| // Imports | ||
| use std::{cmp::Ordering, fmt::Debug}; | ||
| use std::fmt::Debug; |
Comment on lines
+128
to
150
| /// This is a fast, semi-approximate method whose behavior is unspecified when either | ||
| /// distance is [`f32::NAN`]. | ||
| /// | ||
| /// ```rust | ||
| /// use diskann::neighbor::{Neighbor, ord::fast_distance}; | ||
| /// | ||
| /// let x = Neighbor::new(10, 5.0); | ||
| /// let y = Neighbor::new(11, 4.0); | ||
| /// | ||
| /// assert!(fast_distance(&x, &y).is_gt()); | ||
| /// assert!(fast_distance(&y, &x).is_lt()); | ||
| /// assert!(fast_distance(&x, &x).is_eq()); | ||
| /// | ||
| /// let z = Neighbor::new(12, f32::NAN); | ||
| /// | ||
| /// // The following line can return any `Ordering`. | ||
| /// // neighbor::ord::fast_distance(&z, &z); | ||
| /// ``` | ||
| pub fn fast_distance<I>(x: &Neighbor<I>, y: &Neighbor<I>) -> std::cmp::Ordering { | ||
| x.distance() | ||
| .partial_cmp(&y.distance()) | ||
| .unwrap_or(std::cmp::Ordering::Equal) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Our current
Neighbortype has several inter-related issues:It structurally requires
Eqfor the ID type, even though this is only needed when comparing equality. This makes it impossible to construct aNeighborcontaining arbitrary external ID types and is the reasonSearchOutputBufferhas to take tuples instead ofNeighbors. This leads to duplicated sorting logic all over the code base.This structural requirement is preventing some aspects of ID handling cleanup.
Its members are public.
Its implementation of
Ordis not consistent with itsPartialEqmethod.Its implementation of
Ordis explicitly called out as invalid.This PR attempts to address many of these issues. Mainly, it turns
Neighborinto a pretty simple tuple-like type with the various ordering methods implemented as free functions inneighbors::ord. Algorithms that need to sortNeighborscan pick and choose the correctord::*methods needed, which in turn are quitegreppable. Theseord::*methods do not quite patch the issue of NaNs showing up in as distances, but at least we have a better way of documenting our requirements.