Add MultiInsertStrategy implementations for BfTreeProvider#949
Add MultiInsertStrategy implementations for BfTreeProvider#949hildebrandmw wants to merge 1 commit intomainfrom
MultiInsertStrategy implementations for BfTreeProvider#949Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds missing MultiInsertStrategy implementations for BfTreeProvider to restore internal test/usage paths that broke after #859.
Changes:
- Introduce a
pq::Overlaytype alias forHybridMapworking-set overlays. - Implement
MultiInsertStrategyforBfTreeProviderin bothFullPrecisionandHybridmodes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| diskann-providers/src/model/graph/provider/async_/distances.rs | Adds an overlay type alias used as the seed for multi-insert flows with PQ/hybrid distances. |
| diskann-providers/src/model/graph/provider/async_/bf_tree/provider.rs | Adds MultiInsertStrategy impls so batch insert paths can build the appropriate working set/overlay for BfTreeProvider. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #949 +/- ##
==========================================
- Coverage 89.32% 89.31% -0.01%
==========================================
Files 447 447
Lines 83891 83891
==========================================
- Hits 74934 74931 -3
- Misses 8957 8960 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| std::future::ready(Ok(overlay)) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
are there tests already to cover this code path? if not, please add.
There was a problem hiding this comment.
I called this out in the PR description: there are no meaningful tests for BfTreeProvider. The only tests were code in our internal Python package (which I broke with the multi-insert change and this is fixing 😢). Writing tests here would involve standing up the entire test ecosystem for the BfTreeProvider, which I really don't think is a good use of time at the moment. In any case, this particular change is 95% checked by the compiler.
These implementations were missed in #859 which broke some internal test code. No tests are added since the internal code that broke was largely serving as the tests, and we don't seem to have any current tests for
BfTreeProvideranyways.