Skip to content

Deduplicate virtual start point edges during disk serialization - #1350

Open
juchen-ms (partychen) wants to merge 1 commit into
microsoft:mainfrom
partychen:fix/virtual-start-serialization
Open

Deduplicate virtual start point edges during disk serialization#1350
juchen-ms (partychen) wants to merge 1 commit into
microsoft:mainfrom
partychen:fix/virtual-start-serialization

Conversation

@partychen

@partychen juchen-ms (partychen) commented Aug 24, 2026

Copy link
Copy Markdown
Contributor
  • Does this PR have a descriptive title that could go in our release notes?
  • Does this PR add any new dependencies? No.
  • Does this PR modify any existing APIs? No.
  • Is the change to the API backwards compatible? Yes.
  • Should this result in any documentation changes? No.

Reference Issues/PRs

N/A

What does this implement/fix? Briefly explain your changes.

The in-memory graph uses a virtual start point ID, while the serialized disk graph must replace it with the actual medoid ID.

Previously, this replacement happened after adjacency-list uniqueness had already been established. If a list contained both the virtual start point and the actual medoid, the remapping produced a duplicate medoid edge. On the medoid's own adjacency list, it could also produce a self-loop.

This PR updates DiskAdaptor to:

  • remap the virtual start point to the actual medoid;
  • preserve the original neighbor order;
  • retain only the first resulting medoid edge;
  • remove self-loops after remapping.

It does not backfill removed entries with arbitrary neighbors, so affected adjacency lists may contain one fewer unique edge.

The regression test covers both a remapping collision and the medoid self-loop case. The expected disk-index fixture is updated for the corrected serialized graph.

Any other comments?

The in-memory adjacency list is already unique before serialization, so only the alias introduced by virtual-start-point remapping needs post-remap deduplication. This keeps the change limited to disk serialization and does not alter graph construction or pruning behavior.

Copilot AI 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 fixes a disk-serialization edge case in diskann-providers where remapping the in-memory “virtual start point” ID to the actual medoid ID could introduce duplicate medoid edges and (on the medoid’s own adjacency list) create a self-loop in the serialized disk graph. The change is scoped to the disk-graph adaptor used during serialization and is validated by a new regression test plus an updated disk-index fixture.

Changes:

  • Update DiskAdaptor::get_adjacency_list to remap the virtual start point to the actual start point while preserving neighbor order, deduplicating the resulting medoid edge (keep first), and removing self-loops after remapping.
  • Add a unit test covering both the remapping collision (duplicate medoid) and medoid self-loop scenarios.
  • Update the expected disk-index fixture (Git LFS pointer) to match the corrected serialized graph output.

Reviewed changes

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

File Description
diskann-providers/src/model/graph/provider/async_/simple_neighbor_provider.rs Adjusts disk-serialization adjacency list remapping to deduplicate the remapped medoid edge and remove self-loops; adds a regression test for both failure modes.
test_data/disk_index_build/truth_sift_learn_R4_L50_disk.index Updates the expected on-disk index fixture pointer to reflect the corrected serialization output.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.53%. Comparing base (860cf47) to head (dc9b2b7).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1350      +/-   ##
==========================================
+ Coverage   91.55%   92.53%   +0.97%     
==========================================
  Files         521      521              
  Lines      100347   100371      +24     
==========================================
+ Hits        91877    92876     +999     
+ Misses       8470     7495     -975     
Flag Coverage Δ
miri 92.53% <100.00%> (+0.97%) ⬆️
unittests 92.49% <100.00%> (+1.25%) ⬆️

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

Files with missing lines Coverage Δ
.../graph/provider/async_/simple_neighbor_provider.rs 94.09% <100.00%> (-2.63%) ⬇️

... and 43 files with indirect coverage changes

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

Sign up for free to 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.

3 participants