Skip to content

VirtualStorageProvider: Make new() private, add new_physical - #764

Merged
Alex Razumov (arrayka) merged 8 commits into
mainfrom
copilot/refactor-virtualstorageprovider-constructor
Feb 11, 2026
Merged

VirtualStorageProvider: Make new() private, add new_physical#764
Alex Razumov (arrayka) merged 8 commits into
mainfrom
copilot/refactor-virtualstorageprovider-constructor

Conversation

CopilotAI commented Feb 11, 2026

Copy link
Copy Markdown
Contributor

Completed Changes

  • Made VirtualStorageProvider::new() fully private (not just doc-hidden)
  • Updated all unit tests in virtual_storage_provider.rs to use new_memory()
  • Updated all usages across diskann-providers, diskann-disk, and diskann-tools to use specific constructors
  • Added VirtualStorageProvider::new_physical to clippy.toml disallowed-methods list
  • Removed all unused imports (MemoryFS, OverlayFS)
  • Made test helper functions generic over filesystem type where needed
  • Fixed doc tests:
    • Added missing use std::io::Write; import to doc examples
    • Removed doc examples that write to physical filesystem (new_physical and new_overlay)
    • Simplified to only show new_memory() usage
    • Removed unnecessary turbofish notation in test code
  • Verified all checks pass:
    • ✅ cargo fmt --all --check
    • ✅ cargo clippy --locked --workspace --all-targets --no-deps
    • ✅ cargo test --doc --workspace
    • ✅ All unit tests pass

All PR review feedback has been addressed.

Original prompt

This section details on the original issue you should resolve

<issue_title>VirtualStorageProvider: encourage use of new_overlay or new_memory</issue_title>
<issue_description>Make VirtualStorageProvider::new private.
Also add VirtualStorageProvider::new_physical<P: AsRefstd::path::Path>(path: P).
Expose only new_overlay(), new_memory(), new_physical().

Examples like these should be replaced with VirtualStorageProvider::new_memory():
let fs = OverlayFS::new(&[MemoryFS::default().into()]);
let storage_provider = VirtualStorageProvider::new(fs);

Original post:

Do you see any value in making VirtualStorageProvider::new much harder to call to encourage use of new_overlay or new_memory? Maybe with like a new_physical and then linting that with clippy?

Originally posted by Mark Hildebrand (@hildebrandmw) in #700</issue_description>

Comments on the Issue (you are Copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

CopilotAIand others added 3 commits February 11, 2026 07:30
Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com>
…new() doc(hidden)
Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com>
Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com>
CopilotAI changed the title [WIP] Make VirtualStorageProvider::new private and expose alternativesVirtualStorageProvider: Hide generic constructor, add new_physicalFeb 11, 2026
Comment threaddiskann-providers/src/storage/virtual_storage_provider.rs Outdated
Comment threaddiskann-providers/src/storage/virtual_storage_provider.rs
Comment threaddiskann-providers/src/storage/virtual_storage_provider.rs
CopilotAIand others added 2 commits February 11, 2026 08:14
…_memory() in diskann-providers
Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com>
…ic constructors, add clippy rule
Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com>
CopilotAI changed the title VirtualStorageProvider: Hide generic constructor, add new_physicalVirtualStorageProvider: Make new() private, add new_physicalFeb 11, 2026
@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Feb 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.99%. Comparing base (87a0757) to head (c3daeb3).
⚠️ Report is 41 commits behind head on main.

Files with missing linesPatch %Lines
...-providers/src/storage/virtual_storage_provider.rs50.00%4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@ Coverage Diff @@## main #764 +/- ##
==========================================
+ Coverage 88.98% 88.99% +0.01% 
==========================================
Files 428 428 Lines 78365 78420 +55 ==========================================
+ Hits 69730 69788 +58 + Misses 8635 8632 -3 
FlagCoverage Δ
miri88.99% <96.00%> (+0.01%)⬆️
unittests88.99% <96.00%> (+0.01%)⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing linesCoverage Δ
diskann-disk/src/build/builder/build.rs94.16% <100.00%> (-0.03%)⬇️
diskann-disk/src/storage/cached_reader.rs99.16% <100.00%> (-0.02%)⬇️
diskann-disk/src/storage/quant/generator.rs92.72% <100.00%> (+0.02%)⬆️
diskann-disk/src/storage/quant/pq/pq_generation.rs93.33% <100.00%> (ø)
...ovider/async_/fast_memory_quant_vector_provider.rs98.50% <100.00%> (ø)
...aph/provider/async_/fast_memory_vector_provider.rs95.12% <100.00%> (ø)
...rs/src/model/graph/provider/async_/inmem/scalar.rs96.02% <100.00%> (ø)
...ph/provider/async_/memory_quant_vector_provider.rs98.38% <100.00%> (ø)
...el/graph/provider/async_/memory_vector_provider.rs98.30% <100.00%> (ø)
.../graph/provider/async_/simple_neighbor_provider.rs92.16% <100.00%> (ø)
... and 12 more

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens the diskann_providers::storage::VirtualStorageProvider API by making the generic new() constructor private and pushing callers toward explicit constructors (new_memory, new_overlay, new_physical) that better communicate intent and reduce accidental real-filesystem usage in tests.

Changes:

  • Made VirtualStorageProvider::new() fully private and updated call sites to use intent-specific constructors.
  • Added VirtualStorageProvider::new_physical(...) for sandboxed physical filesystem usage.
  • Added a Clippy disallowed-methods rule to discourage new_physical() in tests and updated examples/bench code accordingly.

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 6 comments.

Show a summary per file
FileDescription
diskann-tools/src/utils/relative_contrast.rsSwitched tests from VirtualStorageProvider::new(MemoryFS) to new_memory().
diskann-tools/src/utils/build_disk_index.rsSwitched tests to VirtualStorageProvider::new_memory().
diskann-providers/src/utils/storage_utils.rsUpdated tests to use new_memory() instead of constructing MemoryFS directly.
diskann-providers/src/utils/sampling.rsUpdated tests to use new_memory().
diskann-providers/src/utils/medoid.rsUpdated tests to use new_memory() and write via storage_provider.filesystem().
diskann-providers/src/utils/file_util.rsUpdated tests to use new_memory().
diskann-providers/src/storage/virtual_storage_provider.rsMade new() private, added new_physical(), updated doctest examples and unit tests.
diskann-providers/src/storage/sq_storage.rsUpdated tests to use new_memory().
diskann-providers/src/storage/protos/storage.rsUpdated tests to use new_memory().
diskann-providers/src/storage/pq_storage.rsUpdated tests to use new_memory().
diskann-providers/src/storage/index_storage.rsUpdated async test to use new_memory().
diskann-providers/src/model/pq/pq_construction.rsUpdated tests to use new_memory().
diskann-providers/src/model/graph/provider/async_/simple_neighbor_provider.rsUpdated tests to use new_memory().
diskann-providers/src/model/graph/provider/async_/memory_vector_provider.rsUpdated tests to use new_memory().
diskann-providers/src/model/graph/provider/async_/memory_quant_vector_provider.rsUpdated tests to use new_memory().
diskann-providers/src/model/graph/provider/async_/inmem/scalar.rsUpdated tests to use new_memory().
diskann-providers/src/model/graph/provider/async_/fast_memory_vector_provider.rsUpdated tests to use new_memory().
diskann-providers/src/model/graph/provider/async_/fast_memory_quant_vector_provider.rsUpdated tests to use new_memory().
diskann-providers/benches/benchmarks_iai/copy_aligned_data_bench_iai.rsUpdated benchmark setup to use new_physical(...).
diskann-providers/benches/benchmarks/copy_aligned_data_bench.rsUpdated benchmark setup to use new_physical(...).
diskann-disk/src/storage/quant/pq/pq_generation.rsSimplified test storage setup to new_memory() and generalized helper types.
diskann-disk/src/storage/quant/generator.rsUpdated test helpers to use new_memory() and generalized storage provider type params.
diskann-disk/src/storage/cached_reader.rsUpdated tests to use new_memory().
diskann-disk/src/build/builder/build.rsUpdated tests to use new_memory().
clippy.tomlAdded disallowed-methods entry for VirtualStorageProvider::new_physical.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threaddiskann-providers/src/storage/virtual_storage_provider.rs
Comment threaddiskann-providers/src/storage/virtual_storage_provider.rs Outdated
Comment threaddiskann-providers/src/storage/virtual_storage_provider.rs Outdated
Comment threaddiskann-providers/src/storage/virtual_storage_provider.rs Outdated
Comment threadclippy.toml Outdated
Comment threaddiskann-providers/src/storage/virtual_storage_provider.rs Outdated
Comment threaddiskann-disk/src/storage/quant/generator.rs Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Comment threaddiskann-providers/src/storage/virtual_storage_provider.rs
Comment threaddiskann-providers/src/storage/virtual_storage_provider.rs Outdated
Comment threaddiskann-providers/src/storage/virtual_storage_provider.rs Outdated
Comment threaddiskann-providers/src/storage/virtual_storage_provider.rs Outdated
Comment threaddiskann-providers/src/storage/virtual_storage_provider.rs Outdated
…xamples, remove unnecessary turbofish
Co-authored-by: arrayka <1551741+arrayka@users.noreply.github.com>
@arrayka
Alex Razumov (arrayka) merged commit 0a00171 into mainFeb 11, 2026
20 checks passed
@arrayka
Alex Razumov (arrayka) deleted the copilot/refactor-virtualstorageprovider-constructor branch February 11, 2026 22:24
Mark Hildebrand (hildebrandmw) added a commit that referenced this pull request Feb 13, 2026
## What's Changed
### API Breaking Changes
* Remove the `experimental_avx512` feature. by @hildebrandmw in
#732
* Use VirtualStorageProvider::new_overlay(test_data_root()) in tests by
@Copilot in #726
* save and load max_record_size and leaf_page_size for bftrees by
@backurs in #724
* [multi-vector] Verify `Standard` won't overflow in its constructor. by
@hildebrandmw in #757
* VirtualStorageProvider: Make new() private, add new_physical by
@Copilot in #764
* [minmax] Refactor full query by @arkrishn94 in
#770
* Bump diskann-quantization to edition 2024. by @hildebrandmw in
#772
### Additions
* [multi-vector] Enable cloning of `Mat` and friends. by @hildebrandmw
in #759
* adding bftreepaths in mod.rs by @backurs in
#775
* [quantization] Add `as_raw_ptr`. by @hildebrandmw in
#774
### Bug Fixes
* Fix `diskann` compilation without default-features and add CI tests.
by @hildebrandmw in #722
### Docs and Comments
* Updating the benchmark README to use diskann-benchmark by @bryantower
in #709
* Fix doc comment: Windows line endings are \r\n not \n\r by @Copilot in
#717
* Fix spelling errors in streaming API documentation by @Copilot in
#715
* Add performance diagnostic to `diskann-benchmark` by @hildebrandmw in
#744
* Add agents.md onboarding guide for coding agents by @Copilot in
#765
* [doc] Fix lots of little typos in `diskann-wide` by @hildebrandmw in
#771
### Performance
* [diskann-wide] Optimize `load_simd_first` for 8-bit and 16-bit element
types. by @hildebrandmw in #747
### Dependencies
* Bump bytes from 1.11.0 to 1.11.1 by @dependabot[bot] in
#723
* [diskann] Add note on the selection of `PruneKind` in
`graph::config::Builder`. by @hildebrandmw in
#734
* [diskann-providers] Remove the LRU dependency and make `vfs` and
`serde_json` optional. by @hildebrandmw in
#733
### Infrastructure
* Add initial QEMU tests for `diskann-wide`. by @hildebrandmw in
#719
* [CI] Skip coverage for Dependabot. by @hildebrandmw in
#725
* Add miri test coverage to CI workflow by @Copilot in
#729
* [CI] Add minimal ARM checks by @hildebrandmw in
#745
* Enable CodeQL security analysis by @Copilot in
#754
## New Contributors
* @backurs made their first contribution in
#724
* @arkrishn94 made their first contribution in
#770
**Full Changelog**:
0.45.0...0.46.0
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VirtualStorageProvider: encourage use of new_overlay or new_memory

6 participants

@codecov-commenter@metajack@arrayka@hildebrandmw