RFC - Beta Filter For Disk Search

[dyhyfu]

Summary

Adds beta-biased filtering on the disk search path by introducing a SearchPlan enum that replaces the existing (vector_filter, is_flat_search) parameter pair on searcher.search(). The new enum is hierarchical (SearchPlan { FlatScan, Graph(GraphMode) }), makes invalid combinations unrepresentable, and gives future filter algorithms a single extension point without further growing the public search() signature.

Motivation

• Beta-biased graph search is unavailable on the disk path. The in-memory side has a BetaFilter strategy; the disk side has no equivalent.
• A raw closure can't carry beta. Bolting it on as a third Option<f32> parameter creates meaningless combinations (is_flat_search=true + beta=Some(_)) the type system can't reject.
• No clean integration point for future filter algorithms (MultihopSearch exists in diskann but the disk API has no way to select it).

What's in the RFC

• SearchPlan { FlatScan { filter: Option<Predicate> }, Graph(GraphMode) } + GraphMode { Unfiltered, PostFilter(p), BetaFilter { predicate, beta } }.
• Predicate = Box<dyn Fn(u32) -> bool + Send + Sync> — closure-based.
• Single GraphMode match site in search_strategy() that projects (predicate, beta); downstream DiskAccessor and RerankAndFilter read those fields directly — adding a new GraphMode variant touches one place.
• Invalid combinations (beta on flat scan, beta without a predicate) unrepresentable by construction.
• Zero allocation and zero per-iteration overhead preserved on the no-filter graph path.

Caller migration

Today (vector_filter, is_flat_search)	New plan
(None, false)	SearchPlan::graph()
(None, true)	SearchPlan::flat()
(Some(p), false)	SearchPlan::graph_with(GraphMode::post_filter(p))
(Some(p), true)	SearchPlan::flat_filtered(p)
(new capability)	SearchPlan::graph_with(GraphMode::beta_filter(p, β))

Known follow-ups (deferred)

• Disk's GraphMode::BetaFilter applies bias + hard post-filter; in-memory BetaFilter only biases. Future work to align.

See rfcs/01101-disk-beta-filter.md for the full design.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR adds an RFC proposing a new SearchPlan/GraphMode API for disk search to support beta-biased filtering, replace (vector_filter, is_flat_search) with a single typed parameter, and create an extensible hook for future graph search variants.

Changes:

• Introduces a hierarchical SearchPlan { FlatScan, Graph(GraphMode) } model for disk search configuration.
• Defines Predicate, GraphMode variants (including BetaFilter), and migration examples from the current API.
• Documents intended internal plumbing changes (strategy projection, pq_distances beta application, and post-filtering behavior).

---

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

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.53%. Comparing base (e2dc9a0) to head (9379b39).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1101      +/-   ##
==========================================
+ Coverage   89.46%   90.53%   +1.06%     
==========================================
  Files         482      482              
  Lines       91082    91082              
==========================================
+ Hits        81491    82461     +970     
+ Misses       9591     8621     -970     

Flag	Coverage Δ
miri	90.53% <ø> (+1.06%)	⬆️
unittests	90.49% <ø> (+1.37%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 41 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[suri-kumkaran]

Thanks for working through this — I’m aligned with the design. I do feel we can trim some of the redundant or overly explanatory parts of the RFC and leave a few of the implementation details for the implementation PRs.

[suri-kumkaran]

This is getting close!

[suri-kumkaran]

Aligned on the design. ID convention is a clean fix and the FilterMode<'a> sum type makes the invariants compiler-enforced.

On Predicate<'a> vs Predicate ('static-only): fine either way — lifetime preserves borrow flexibility, 'static-only is simpler. Author's call.

Approving once the inline comments are resolved.

[suri-kumkaran]

There's no flat_search in diskann/src/graph/index.rs (grep confirms — only disk_provider.rs has one). I think this might be a conflation with diskann::flat::FlatIndex::knn_search in diskann/src/flat/index.rs (the module from RFC 00983-flat-search) — that's a separate, parallel abstraction that the disk path doesn't delegate to.

The disk's own flat_search (disk_provider.rs:912) is a standalone implementation, and it invokes the predicate with positional/internal IDs directly:

let mut iter = (0..provider.num_points as u32).filter(vector_filter);

No to_external_id call anywhere in the disk flat_search path (confirmed: grep for to_external_id in disk_provider.rs returns only the trait definition and one unit test). The 0..num_points idiom looks generic but is the positional/internal space by construction — there's no abstraction between integer counting and positional storage. Identity translation masks this today; under any future non-identity provider, flat-scan and graph paths would diverge silently.

So all three sites (flat_search, pq_distances, RerankAndFilter::post_process) are currently incorrect — this RFC needs to fix all three.

Suggested edits:

• Remove the "Sites already correct" bullet (line 269).
• Add flat_search to "Sites updated in the implementation PR" (line 262):

  • flat_search (the disk-local one in disk_provider.rs) — call to_external_id on each id in (0..num_points) before invoking the predicate.

[wuw92]

Both BetaFilter and MultihopSearch take QueryLabelProvider<DP::InternalId> — their predicates run in the internal-id space. The only to_internal_id / to_external_id translations in index.rs happen at the insert/delete API boundary, not on the search path.
We can keep Predicate keyed on internal-id here too.

[dyhyfu]

Checking latest code, flat_search has been moved to disk_provider.rs and is using internal_id as input param. The "ID convension" part will be removed and Predicate will be keyed on internal_id.

[suri-kumkaran]

The motivation for external-keyed predicates was broader than the flat_search audit — that was a symptom, not the core argument. External-keyed is still the right contract because:

• Matches the DataProvider abstraction; doesn't leak the positional internal representation.
• Matches in-memory BetaFilter (keyed on DP::ExternalId via QueryLabelProvider) — closes one corner of the divergence the Future Work item flags.
• Future-proof: non-identity translation (sparse IDs, deletion compaction, u64→u32) stays disk-internal; callers don't break silently.

Practically: how do external clients construct a predicate over internal IDs? They only see external IDs — from search outputs, ingest, the DataProvider contract. Asking them to maintain a bitmap over a space they can't observe pushes implementation details out as caller responsibility, and silently couples every consumer to today's identity-translation invariant.

The fix is small and inlines to a no-op under identity translation. Worth keeping in the RFC.

[wuw92]

DiskProvider doesn't really expose an Internal/External contract today. Both types are pinned to u32, VectorIdType = u32 is a hard where bound on the impl (disk_provider.rs:95-103), I'd suggest dropping the ID-convention change from this RFC.

  impl<Data> DataProvider for DiskProvider<Data>
  where Data: GraphDataType<VectorIdType = u32>,
  {
      type InternalId = u32;
      type ExternalId = u32;

      fn to_internal_id(&self, _: &DefaultContext, gid: &u32) -> Result<u32, _> { Ok(*gid) }
      fn to_external_id(&self, _: &DefaultContext, id: u32)   -> Result<u32, _> { Ok(id) }
  }

For Practical, external clients implements to_internal_id / to_external_id on their provider, so they define the translation and own both sides. The question isn't whether they can observe internal id — it's which side of the boundary is the natural place to do the translation.

For the 1:N domain model (which is what our team has: external = doc_id, internal = vector_id, one doc-id → many vector-ids), "caller side" is a feasible option.

• DataProvider's ExternalId ↔ InternalId is 1:1 by trait signature; a 1:N domain doesn't fit through it.
• So we keep the doc_id ↔ {vector_id} map outside the library, for example in a filtered scenario:
  1. Filtered doc-ids → vector-id bitmap (outside DiskANN).
  2. DiskANN filter-search returns vector-ids.
  3. Vector-ids → doc-ids (outside DiskANN).

[suri-kumkaran]

Two separate things getting mixed up — the outer mapping (domain ↔ vector IDs) and the inner consistency (which ID space DiskANN uses internally).

Outer mapping (1:N): agreed, this lives outside the library. DataProvider::ExternalId ↔ InternalId is 1:1 by signature, so doc-id ↔ {vector-id} has to be maintained caller-side. The flow you describe (filtered doc-ids → vector-id bitmap → DiskANN search → vector-ids → doc-ids) is the only way it can work. No disagreement there.

Inner consistency (the RFC's change): by the time DiskANN receives the predicate, it's already in vector-id space (= external ID). The question is whether DiskANN invokes that predicate with ExternalId or InternalId. Today three sites — RerankAndFilter::post_process, pq_distances, flat_search — pass internal directly. This is a latent bug: it produces wrong results the moment DiskProvider ever uses non-identity translation (e.g., for deletion compaction or sparse IDs added in place to the existing provider — no fork required).

Re: "DiskProvider doesn't expose the distinction today" — true, and that's exactly why the bug is invisible. The trait exists precisely so future in-place changes don't break consumers. Fixing now is essentially free (translation inlines to a no-op under identity). Leaving it means the next person extending DiskProvider has to remember to also audit every predicate site, with no compile-time help.

Want to keep the ID-convention section in this RFC — it pre-existing-bug we surface and fix here. Fix lands in the impl PR.

[dyhyfu]

@microsoft-github-policy-service agree company="Microsoft"