Repository navigation
Remove DataProvider::to_external_id - #1541
Merged
Mark Hildebrand (hildebrandmw) merged 1 commit intoOct 29, 2026
Merged
Conversation
Mark Hildebrand (hildebrandmw)
requested review from
a team
and
a balanced review from Copilot
October 29, 2026 19:31
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
There are no active review issues for the removal of the unused DataProvider::to_external_id trait method.
No open issues 🎉
What changed in this PR
Removes the unused DataProvider::to_external_id trait method, leaving internal-to-external ID translation to provider-specific post-processing. This simplifies the core interface and avoids requiring a slow fallback for future epoch-protected translation in diskann-inmem.
Changes:
- Remove the trait method and its implementations across providers.
- Preserve Garnet’s translation logic as a private inherent method.
- Remove test assertions for the deleted API.
| File | Description |
|---|---|
diskann/src/provider.rs |
Remove to_external_id from the core trait. |
diskann/src/graph/test/provider.rs |
Remove the test provider’s implementation and related assertions. |
diskann/src/flat/test/provider.rs |
Remove the test provider’s implementation. |
diskann-providers/src/model/graph/provider/async_/inmem/provider.rs |
Remove identity translation and its test assertion. |
diskann-label-filter/src/encoded_attribute_provider/document_provider.rs |
Remove forwarding of the deleted method. |
diskann-inmem/src/provider.rs |
Remove single-ID reverse translation and related assertions. |
diskann-garnet/src/provider.rs |
Move reverse translation into an inherent method. |
diskann-disk/src/search/provider/disk_provider.rs |
Remove identity translation and its test assertion. |
diskann-bftree/src/provider.rs |
Remove identity translation and its test assertion. |
Review effort: Balanced
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1541 +/- ##
==========================================
+ Coverage 91.01% 91.02% +0.01%
==========================================
Files 602 602
Lines 124144 124080 -64
==========================================
- Hits 112994 112949 -45
+ Misses 11150 11131 -19
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Jack Moffitt (metajack)
approved these changes
Oct 29, 2026
Harsha Simhadri (harsha-simhadri)
approved these changes
Oct 29, 2026
Mark Hildebrand (hildebrandmw)
deleted the
mhildebr/remove-to-external-id
branch
October 29, 2026 21:07
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.
DataProvider::to_external_idis apparently not used anywhere in the core algorithm. There are currently two users of internal to external ID translation:diskann-garnert: Just uses it for post-processing and works just fine withto_external_idas an inherent method onGarnetProvider.diskann-inmem: Does do ID translation, but already uses a more batch-friendly version during its post-processing.Removing this (outside of removing code that doesn't apparently have a need) helps the
diskann-inmemwork as we don't need to provide an extremely slow fallback if ID translation is moved behind the epoch protected boundary.