Restore the ability to have Owned computers. - #1382
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new Shared unit test contains an invalid pointer comparison (shared.as_ptr() vs Arc::as_ptr()), which will not type-check and should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR reintroduces reference-counted ownership for PQ distance/query computers by adding a Shared<'a, T> container (borrowed &T or owned Arc<T>) and threading it through the PQ distance stack and relevant providers/benchmarks, restoring compatibility with code paths that still require Arc<FixedChunkPQTable>.
Changes:
- Add
Shared<'a, T>(Arc-backed “owned” + borrowed ref) topq::distanceand update PQ distance/query computer APIs to accept it. - Update PQ distance implementations (L2/IP/Cosine/Dynamic/Multi) and async providers to pass
Sharedinstead of raw&FixedChunkPQTable. - Adjust benchmark code to construct
Sharedwhen creating PQ query computers.
File summaries
| File | Description |
|---|---|
| diskann-providers/src/model/pq/distance/multi.rs | Switch MultiTable to store Shared PQ tables and update tests/callers accordingly. |
| diskann-providers/src/model/pq/distance/mod.rs | Introduce the new Shared<'a, T> container and add unit tests. |
| diskann-providers/src/model/pq/distance/l2.rs | Update L2 PQ query computer to hold Shared parent table. |
| diskann-providers/src/model/pq/distance/innerproduct.rs | Update inner product PQ query computer to hold Shared parent table. |
| diskann-providers/src/model/pq/distance/dynamic.rs | Update dynamic PQ query/distance computer constructors and vtable calls to use Shared. |
| diskann-providers/src/model/pq/distance/cosine.rs | Update cosine PQ query computer to hold Shared parent table. |
| diskann-providers/src/model/graph/provider/async_/memory_quant_vector_provider.rs | Wrap PQ table with Shared::Ref when creating computers. |
| diskann-providers/src/model/graph/provider/async_/fast_memory_quant_vector_provider.rs | Wrap PQ table with Shared::Ref when creating computers. |
| diskann-providers/src/model/graph/provider/async_/experimental/multi_pq_async.rs | Update multi-PQ table construction to use Shared::Ref for schemas. |
| diskann-benchmark/src/exhaustive/product.rs | Construct Shared when creating PQ query computers in benchmarks. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1382 +/- ##
==========================================
- Coverage 91.55% 91.43% -0.12%
==========================================
Files 521 522 +1
Lines 100302 100766 +464
==========================================
+ Hits 91828 92132 +304
- Misses 8474 8634 +160
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
# Breaking Changes since 0.57.0 • Restored support for reference-counted (`Arc`) PQ distance tables via a new internal `Shared<'a, FixedChunkPQTable>` enum, unblocking code paths that still depend on owned/shared tables ahead of a full PQ replacement. (#1382) ## What's Changed * Restore the ability to have `Owned` computers. by @hildebrandmw in #1382 **Full Changelog**: v0.57.0...v0.58.0 Co-authored-by: Mark Hildebrand <mhildebrand@microsoft.com>
Turns out #1247 was a little premature - we still have some code that is dependent on using
Arc<FixedChunkPQTable>. Until a full replacement for PQ is in place, this is a stop-gap to enabled reference counted computers.The main idea is to introduce
as the container for the
FixedChunkPQTable. Types that need to useArccan useShared<'static, T>. Existing code can continue to useShared::Ref.