Skip to content

Conversation

@nnethercott
Copy link
Owner

@nnethercott nnethercott commented Oct 11, 2025

Fixes #105

Separates layer traversal logic from Reader to new struct Visitor. Cancellation is performed by checking an impl Fn()->bool in a hot loop inside Visitor::visit. We also add 2 new methods to the Reader api for degraded search by vector and by item.

Copy link
Collaborator

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

I am wondering if returning a tuple is the best option or if we plan to return other things and therefore returning a struct would be more future-proof? 🤔

@nnethercott
Copy link
Owner Author

returning a struct would be more future-proof

Doing something like below is certainly "nicer", but would require also updating the api's for QueryBuilder.by_vector and QueryBuider.by_item and not just their cancellable variants.

I kinda like how simple the methods are already, but I'm not sure if you guys have a preference at meili.

@dureuill thoughts ?

#[non_exhaustive]
pub struct HannoyResult {
  pub neighbours: Vec<(u32, f32)>,
  pub did_cancel: bool 
}

pub fn by_vector_with_cancellation(
    &self,
    rtxn: &RoTxn,
    vector: &'a [f32],
    cancel_fn: impl Fn() -> bool,
) -> Result<HannoyResult> { todo!() }

Copy link
Collaborator

@Kerollmops Kerollmops left a comment

Choose a reason for hiding this comment

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

Ho! Crap! I thought you were changing the output of the original search methods. Indeed doing that is great as it doesn't impact other users.

However, I was more into proposing something similar to the indexing part where we set the cancellation function with the builder and don't introduce any other API. The search methods are not returning the boolean you introduced. We can directly check if the search took too long at Meilisearch 👀

@dureuill
Copy link

dureuill commented Oct 13, 2025

Hello @nnethercott , thank you for taking this one ☺️

I much prefer the struct version, it is both more readable and more future-proof. I'm fine with the added methods, I don't think we'll need many much more in the future (cancellation is kind of the only support function I can think of)

I'm not exactly sure if I like the non-exhaustive: while I understand it from a semver perspective, I think I prefer the Meilisearch code to break at call sites when a new field is added, which will not be possible with the non-exhaustive AFAIU.

Thanks again, looking forward to this ❤️

@dureuill
Copy link

FYI @nnethercott we took the code of this PR and added it to the branch in #103 which we use in Meilisearch.

@nnethercott nnethercott merged commit d1f30a0 into main Oct 18, 2025
8 checks passed
@nnethercott nnethercott deleted the add-degraded-search branch October 18, 2025 15:40
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.

Support degraded search

4 participants