Relax the 'static bound on DistanceComputer.#1007
Open
hildebrandmw wants to merge 3 commits intomainfrom
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to relax lifetime requirements around pruning distance computation so DistanceComputer implementations can borrow from their associated provider/accessor instead of being forced to be 'static.
Changes:
- Removes the
+'staticbound fromBuildDistanceComputer::DistanceComputer. - Refactors
PruneStrategy::DistanceComputerinto a lifetime-parameterized associated type (GAT) and wires it throughPruneAccessor. - Updates in-repo
PruneStrategyimplementations to the new associated type shape.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| diskann/src/provider.rs | Drops the 'static bound on BuildDistanceComputer::DistanceComputer. |
| diskann/src/graph/glue.rs | Changes PruneStrategy::DistanceComputer to a GAT and connects it to PruneAccessor. |
| diskann/src/graph/test/provider.rs | Updates the test prune strategy impl to the new DistanceComputer<'a> associated type. |
| diskann-providers/src/model/graph/provider/async_/inmem/test.rs | Updates prune strategy test impl to new DistanceComputer<'a> associated type. |
| diskann-providers/src/model/graph/provider/async_/inmem/spherical.rs | Updates prune strategy impl to new DistanceComputer<'a> associated type. |
| diskann-providers/src/model/graph/provider/async_/inmem/scalar.rs | Updates prune strategy impl to new DistanceComputer<'a> associated type. |
| diskann-providers/src/model/graph/provider/async_/inmem/product.rs | Updates prune strategy impls to new DistanceComputer<'a> associated type. |
| diskann-providers/src/model/graph/provider/async_/inmem/full_precision.rs | Updates prune strategy impl to new DistanceComputer<'a> associated type. |
| diskann-providers/src/model/graph/provider/async_/caching/provider.rs | Propagates the new DistanceComputer<'a> through the caching prune strategy wrapper. |
| diskann-providers/src/model/graph/provider/async_/bf_tree/provider.rs | Updates bf-tree prune strategy impls to new DistanceComputer<'a> associated type. |
| diskann-garnet/src/provider.rs | Updates Garnet prune strategy impl to new DistanceComputer<'a> associated type. |
Comments suppressed due to low confidence (1)
diskann/src/graph/glue.rs:622
PruneStrategy::DistanceComputer<'computer>still has a+ 'staticbound. That prevents the associatedDistanceComputerfrom borrowing from the provider/accessor, which is the main goal of this PR (and it also conflicts with the PR description). Consider removing the'staticbound here (and ensuring any call sites that truly need'staticadd it locally instead).
type DistanceComputer<'computer>: for<'a, 'b, 'c, 'd> DistanceFunction<
<Self::PruneAccessor<'a> as Accessor>::ElementRef<'b>,
<Self::PruneAccessor<'c> as Accessor>::ElementRef<'d>,
f32,
> + Send
+ Sync
+ 'static;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1007 +/- ##
=======================================
Coverage 89.49% 89.49%
=======================================
Files 448 448
Lines 84118 84118
=======================================
Hits 75282 75282
Misses 8836 8836
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
May 2, 2026
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.
What
Remove the
'staticbound onBuildDistanceComputer::DistanceComputerand allow theDistanceComputerassociated type ofPruneStrategyto have a lifetime.Why
This bound is not really needed. Distance computers are almost exclusively used during prune loops, so there's no need for the
'staticbound. Relaxing that bound allowsDistanceComputerimplementations to borrow from the associated provider. This is helpful for PQ since this allowsDistanceComputers to borrow the central pivot table rather than requiring anArcor some other reference counting mechanism.Upgrade Guide
Since all current
DistanceComputersare'static, the only change needed is to add a lifetime to theDistanceComputergeneric associated type of allPruneStrategyimplementations.