Refactor disk index build structure

[Wei Wu (wuw92)]

Why

This is a follow-up to #1254. Removing checkpoint and continuation support eliminated the workflow state that previously justified much of the disk-build indirection, allowing the remaining build path to be simplified and its module ownership clarified.

Module changes

1. Rename core.rs to merged_index.rs; the renamed module retains the merged-build implementation and RAM estimation.
2. Move shared builder fixtures and end-to-end disk-build tests from the renamed module to tests.rs.
3. Move IndexBuildStrategy and determine_build_strategy from the renamed module to build.rs, where the one-shot versus merged decision is made.
4. Move the single in-memory graph construction pipeline from build.rs to inmem_builder.rs.

build.rs
├── one-shot ────────────────> inmem_builder.rs
└── merged ──> merged_index.rs ──> inmem_builder.rs

Compatibility

This change makes builder::quantizer and builder::tokio private, including the previously public BuildQuantizer, BuildQuantizer::train, and create_runtime APIs. These are implementation details of diskann-disk rather than supported downstream extension points. Downstream crates should use the public disk-index build APIs and manage their own Tokio runtime when needed.

Tests

• test_build_from_iter_one_shot_with_metric verifies one-shot builds for L2, inner-product, and cosine distance.
• test_build_multi_sector_per_node exercises the disk layout and search path when one node spans multiple 512-byte sectors.
• test_build_from_iter_one_shot_with_associated_data verifies that vectors, neighbors, and associated data are persisted correctly without intermediate in-memory index files.
• test_build_from_iter_merged_index forces the low-memory merged-build path and verifies search results and the configured maximum degree.
• test_build_quantization_type_failure_cases verifies that unsupported build quantization settings are rejected with the expected error.
• test_disk_index_builder covers one-shot and merged builds with full precision, 1-bit scalar quantization, and product quantization, then validates persisted PQ data and search results.
• test_disk_minmax_index_builder covers one-shot and merged builds for MinMax vector data and validates the resulting search behavior.

[Copilot]

Pull request overview

This PR refactors the diskann-disk disk index build path to clarify module ownership: merged-index building logic is separated from top-level orchestration, and the in-memory single-graph build pipeline is consolidated behind an internal entry point, while keeping behavior unchanged.

Changes:

• Extract merged-index build implementation (partition/shard build/merge/cleanup) into a dedicated merged_index module.
• Move the in-memory build pipeline into inmem_builder and call it from the top-level disk builder.
• Centralize builder test fixtures under build::builder::tests and update test imports accordingly.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
diskann-disk/src/search/provider/disk_provider.rs	Updates test fixture import path to the new builder test module location.
diskann-disk/src/build/builder/tests.rs	Introduces centralized builder test fixtures and helpers under build::builder::tests.
diskann-disk/src/build/builder/quantizer.rs	Updates documentation wording for build quantizer types.
diskann-disk/src/build/builder/mod.rs	Adjusts module layout (adds merged_index, exposes tests as pub(crate) under cfg(test)).
diskann-disk/src/build/builder/merged_index.rs	New module containing merged-index build implementation and RAM estimation tests.
diskann-disk/src/build/builder/inmem_builder.rs	Consolidates the one-shot in-memory build pipeline behind an internal entry point.
diskann-disk/src/build/builder/build.rs	Keeps top-level orchestration and policy; wires in merged_index + inmem_builder entry points.
diskann-disk/src/build/builder/core.rs	Deletes the previous combined “core” module after refactor.

---

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

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

[Harsha Simhadri (harsha-simhadri)]

could you please briefly list the tests in this PR and their intent in the PR description? thanks

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

[Codecov Comments Bot (codecov-commenter)]

Codecov Report

❌ Patch coverage is 93.17697% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.66%. Comparing base (b7cf3b0) to head (9b1de20).

Files with missing lines	Patch %	Lines
diskann-disk/src/build/builder/inmem_builder.rs	88.72%	15 Missing ⚠️
diskann-disk/src/build/builder/merged_index.rs	97.08%	9 Missing ⚠️
diskann-disk/src/build/builder/build.rs	68.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1271      +/-   ##
==========================================
- Coverage   90.68%   90.66%   -0.03%     
==========================================
  Files         515      516       +1     
  Lines       99230    98856     -374     
==========================================
- Hits        89987    89625     -362     
+ Misses       9243     9231      -12     

Flag	Coverage Δ
miri	90.66% <93.17%> (-0.03%)	⬇️
unittests	90.33% <93.17%> (-0.03%)	⬇️

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

Files with missing lines	Coverage Δ
diskann-disk/src/build/builder/quantizer.rs	92.85% <100.00%> (ø)
diskann-disk/src/build/builder/tokio.rs	88.70% <100.00%> (ø)
diskann-disk/src/search/provider/disk_provider.rs	94.00% <ø> (ø)
diskann-disk/src/build/builder/build.rs	87.76% <68.00%> (-2.40%)	⬇️
diskann-disk/src/build/builder/merged_index.rs	97.08% <97.08%> (ø)
diskann-disk/src/build/builder/inmem_builder.rs	87.12% <88.72%> (+1.40%)	⬆️

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Wei Wu (wuw92)]

could you please briefly list the tests in this PR and their intent in the PR description? thanks

I’ve updated the PR description with a brief list of the covered build scenarios and the intent of each test.

[juchen-ms (partychen)]

Before this change, builder::quantizer and builder::tokio were public modules, and downstream crates could use at least:

• diskann_disk::build::builder::quantizer::BuildQuantizer
• BuildQuantizer::train
• diskann_disk::build::builder::tokio::create_runtime

Making both modules private, together with reducing those items to pub(super), causes existing downstream code using those paths to stop compiling. This is more than an internal module reorganization, although the PR is otherwise presented as a behavior-preserving structural refactor.

If these APIs are intentionally being removed, could the PR description call out the breaking API change? Otherwise, should the old public paths be preserved or deprecated for a compatibility window?

[Wei Wu (wuw92)]

Before this change, builder::quantizer and builder::tokio were public modules, and downstream crates could use at least:

• diskann_disk::build::builder::quantizer::BuildQuantizer
• BuildQuantizer::train
• diskann_disk::build::builder::tokio::create_runtime

Making both modules private, together with reducing those items to pub(super), causes existing downstream code using those paths to stop compiling. This is more than an internal module reorganization, although the PR is otherwise presented as a behavior-preserving structural refactor.

If these APIs are intentionally being removed, could the PR description call out the breaking API change? Otherwise, should the old public paths be preserved or deprecated for a compatibility window?

Thanks for calling this out. The visibility reduction is intentional. BuildQuantizer, BuildQuantizer::train, and the builder-specific create_runtime helper are implementation details used to orchestrate diskann-disk builds; they are not intended as downstream extension points.
I checked the current internal and M365 sources and found no remaining uses of these APIs. I will update the PR description to explicitly call out the breaking visibility changes.

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (2)

diskann-disk/src/build/builder/tests.rs:18

• VectorRepr is imported here but not used anywhere in this test module, which can trigger unused_imports warnings under -D warnings / clippy-all-targets. Drop it from the import list.

    use diskann::{
        graph::config,
        utils::{IntoUsize, VectorRepr, ONE},
        ANNResult,
    };

diskann-disk/src/build/builder/tests.rs:491

• The return value from search_internal is an ANNResult<...>, but it’s currently ignored. If the search errors, the test can keep going with default-filled output buffers and produce misleading passes/failures. Propagate the error (or unwrap) so failures are visible.

            _ = search_engine.search_internal(
                query_data,
                top_k,
                search_l,
                None, // beam_width

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (2)

diskann-disk/src/build/builder/merged_index.rs:412

• Same issue as above: the final node neighbor list is converted into a new Vec before writing. Writing the u32 slice directly avoids an allocation on the hot path.

        let bytes = final_nbrs
            .iter()
            .take(nnbrs as usize)
            .flat_map(|x| x.to_le_bytes())
            .collect::<Vec<u8>>();
        merged_vamana_cached_writer.write(&bytes)?;

diskann-disk/src/build/builder/merged_index.rs:367

• This allocates a fresh Vec for every node when serializing neighbors (via collect::<Vec>()), which can add significant CPU/memory overhead in large merged builds. You can write the existing u32 neighbor slice directly as bytes (bytemuck) to avoid the per-node allocation.

                let bytes = final_nbrs
                    .iter()
                    .take(nnbrs as usize)
                    .flat_map(|x| x.to_le_bytes())
                    .collect::<Vec<u8>>();
                merged_vamana_cached_writer.write(&bytes)?;

[juchen-ms (partychen)]

I will close #1306 to avoid maintaining two competing refactors. My feedback here is not a request to adopt its exact layout. My main concern is that a structural refactor should make the domain model visible from the module boundaries: the disk build orchestrates PQ construction, Vamana construction, and disk layout, while Vamana construction has one-shot and merged strategies.

I would like the naming and ownership in this PR to reflect that model more directly. The enum-dispatch suggestion can be handled separately; the module boundaries, strategy ownership, and overloaded build_inmem_index terminology are the main changes I am requesting.

[juchen-ms (partychen)]

The name build_inmem_index is now used at two different abstraction levels. DiskIndexBuilder::build_inmem_index selects either the one-shot or merged strategy, while this imported helper builds and persists one complete Vamana graph (for either the full dataset or one shard). The outer operation is not necessarily in-memory, and the inner helper is specifically the one-shot leaf operation.

Could we distinguish these responsibilities explicitly - for example, build_vamana_index for the outer phase and build_one_shot_vamana or build_vamana_graph for the leaf helper? That would make the call graph understandable without relying on the historical _mem.index terminology.

[juchen-ms (partychen)]

With core removed, I expected the new module boundaries to expose the build model more directly, but these sibling names still use different concepts: inmem_builder describes memory residency/an implementation detail, while merged_index describes a build strategy or output. Their contents are not symmetric either: the former owns quantizer dispatch plus the complete graph-construction pipeline, while the latter owns the merged-build workflow.

Could we align the ownership and naming around the actual domain - Vamana construction with one-shot and merged strategies - for example through private vamana::{one_shot, merged} modules or an equivalent layout? The exact number of files is not important; the goal is for the module names to state which algorithm and responsibility they own.

[juchen-ms (partychen)]

IndexBuildStrategy sounds like it controls the complete disk-index build, but it only selects how the Vamana graph is constructed. Its current placement also leaves the decision split across modules: build.rs owns the strategy, while the RAM estimate it requires remains in merged_index.rs, even though that estimate is not specific to the merged path.

Could the strategy and its estimate be named and owned together with the Vamana builders (for example, VamanaBuildStrategy)? That would leave build.rs focused on the top-level PQ, Vamana, and disk-layout orchestration.

[juchen-ms (partychen)]

Now that this facade is private, it only erases the closed set of implementations selected by BuildQuantizer (FP, SQ, and PQ). That requires boxed futures and dynamic dispatch for operations such as every vector insertion, while also hiding the supported variants from the type structure.

Would an explicit enum over those variants make this pipeline easier to follow and avoid the per-operation boxing? I see this as a useful simplification, but it need not block the module and naming cleanup if it is better handled as a follow-up.