Skip to content

Commit 999fa5d

Browse files
authored
Simplify expand_beam_accept_only pre-filter predicate (#1192)
#1141 introduced the `FilteredAccessor` trait with two `expand_beam` style methods. This split existed because evaluation of the `HybridPredicate` differed subtly between the "all" and "accept only" paths, but this difference was sufficient to completely tank recall in multi-hop filtered search if rejected IDs were passed to the `PredicateMut`. The two-method solution made this harder, but at the cost of needing to create a new `Accept` type to pass to `Predicate` even before a decision had been made, which is confusing. I think this cleans up that confusion by changing the bound on `expand_beam_accept_only` from ```rust P: HybridPredicate<Accept<Self::Id>> ``` to ```rust P: Predicate<Self::Id> + PredicateMut<Accept<Self::Id>> ``` This makes it clear that raw, unclassified IDs can be used for the non-mutating evaluation while only accepted IDs can be passed to `eval_mut`. The docs have been updated to reflect that the rules of `HybridPredicate` also apply to this pair of traits.
1 parent b5ebac2 commit 999fa5d

2 files changed

Lines changed: 9 additions & 21 deletions

File tree

diskann/src/graph/ext/labeled.rs

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ where
198198
) -> ANNResult<()>
199199
where
200200
Itr: Iterator<Item = Self::Id> + Send,
201-
P: glue::HybridPredicate<Accept<Self::Id>> + Send + Sync,
201+
P: glue::Predicate<Self::Id> + glue::PredicateMut<Accept<Self::Id>> + Send + Sync,
202202
F: FnMut(Accept<Self::Id>, f32) + Send,
203203
{
204204
self.inner
@@ -271,13 +271,11 @@ where
271271

272272
impl<P, I> glue::Predicate<I> for EvalFiltered<'_, P, I>
273273
where
274-
P: glue::Predicate<Accept<I>>,
274+
P: glue::Predicate<I>,
275275
I: VectorId,
276276
{
277277
fn eval(&self, item: &I) -> bool {
278-
// NOTE: Swapping the order here is legal as is passing `Accept` before evaluating
279-
// `is_match`. This is because `self.inner.eval` does not modify state.
280-
self.inner.eval(&Accept::new(*item)) && self.labels.is_match(*item)
278+
self.inner.eval(item) && self.labels.is_match(*item)
281279
}
282280
}
283281

@@ -295,7 +293,7 @@ where
295293

296294
impl<P, I> glue::HybridPredicate<I> for EvalFiltered<'_, P, I>
297295
where
298-
P: glue::HybridPredicate<Accept<I>>,
296+
P: glue::Predicate<I> + glue::PredicateMut<Accept<I>>,
299297
I: VectorId,
300298
{
301299
}

diskann/src/graph/glue.rs

Lines changed: 5 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -454,10 +454,10 @@ pub trait FilteredAccessor: HasId + Send + Sync {
454454
/// Constructing `Accept::new(raw_id)` and passing it to `pred.eval_mut` without
455455
/// having first classified `raw_id` violates this contract.
456456
///
457-
/// Because `pred.eval` is required to be side-effect-free (see [`Predicate`]),
458-
/// implementors are free to call `pred.eval` on any ID — including those that have
459-
/// not yet been classified — to cheaply pre-filter before paying the cost of
460-
/// classification.
457+
/// The [`Predicate`] bound takes unclassified IDs and is side-effect-free, so it can
458+
/// pre-filter IDs before paying the cost of classification. As with [`HybridPredicate`],
459+
/// implementors can assume that [`Predicate`] and [`PredicateMut`] "get along" with
460+
/// respect to `eval(id)` and `eval_mut(Accept::new(id))`.
461461
///
462462
/// See also: [`SearchAccessor::expand_beam`], [`Self::expand_beam_filtered`].
463463
fn expand_beam_accept_only<Itr, P, F>(
@@ -468,7 +468,7 @@ pub trait FilteredAccessor: HasId + Send + Sync {
468468
) -> impl std::future::Future<Output = ANNResult<()>> + Send
469469
where
470470
Itr: Iterator<Item = Self::Id> + Send,
471-
P: HybridPredicate<Accept<Self::Id>> + Send + Sync,
471+
P: Predicate<Self::Id> + PredicateMut<Accept<Self::Id>> + Send + Sync,
472472
F: FnMut(Accept<Self::Id>, f32) + Send;
473473

474474
//////////////////////
@@ -539,15 +539,6 @@ where
539539
}
540540
}
541541

542-
impl<T> Predicate<Accept<T>> for NotInMut<'_, T>
543-
where
544-
T: Eq + std::hash::Hash,
545-
{
546-
fn eval(&self, item: &Accept<T>) -> bool {
547-
self.eval(item.get())
548-
}
549-
}
550-
551542
impl<T> PredicateMut<T> for NotInMut<'_, T>
552543
where
553544
T: Clone + Eq + std::hash::Hash,
@@ -568,7 +559,6 @@ where
568559

569560
/// The interfaces `contains` and `insert` agree with each other.
570561
impl<T> HybridPredicate<T> for NotInMut<'_, T> where T: Clone + Eq + std::hash::Hash {}
571-
impl<T> HybridPredicate<Accept<T>> for NotInMut<'_, T> where T: Clone + Eq + std::hash::Hash {}
572562

573563
/// A search strategy for query objects of type `T`.
574564
///

0 commit comments

Comments
 (0)