Implement federated multi search #625
Conversation
LukasKalbertodt
force-pushed
the
implement-federated-multi-search
branch
2 times, most recently
from
67c4c36 to
4051890
Compare
LukasKalbertodt
force-pushed
the
implement-federated-multi-search
branch
from
4051890 to
93e52d8
Compare
|
""" WalkthroughThe changes introduce federated multi-search support by adding new query and response types, federation-specific options, and related metadata structures. The client gains an asynchronous method to execute federated multi-search queries. Documentation is updated to explain usage and type requirements for federated queries. No changes to error handling or unrelated functionality are made. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant Server
User->>Client: Build MultiSearchQuery
User->>Client: Call with_federation(federation_options)
User->>Client: Call execute() on FederatedMultiSearchQuery
Client->>Server: POST /multi-search with federated query
Server-->>Client: FederatedMultiSearchResponse<T>
Client-->>User: Merged federated search results
Assessment against linked issues
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
LukasKalbertodt
force-pushed
the
implement-federated-multi-search
branch
from
93e52d8 to
9d73cb9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/client.rs (1)
131-144: Reduce duplication by factoring out the common HTTP call
execute_federated_multi_search_queryandexecute_multi_search_queryare identical except for the body/response types. This means we now have two almost-copy-pasted code blocks that have to stay in sync (endpoint path, status code, error handling, etc.).A tiny generic helper would remove this repetition and the chance that one of the two branches silently drifts:
+ async fn post_multi_search<B, R>(&self, body: &B) -> Result<R, Error> + where + Http: HttpClient, + B: Serialize + ?Sized, + R: DeserializeOwned, + { + self.http_client + .request::<(), &B, R>( + &format!("{}/multi-search", &self.host), + Method::Post { body, query: () }, + 200, + ) + .await + } + pub async fn execute_federated_multi_search_query< T: 'static + DeserializeOwned + Send + Sync, >( &self, body: &FederatedMultiSearchQuery<'_, '_, Http>, ) -> Result<FederatedMultiSearchResponse<T>, Error> { - self.http_client - .request::<(), &FederatedMultiSearchQuery<Http>, FederatedMultiSearchResponse<T>>( - &format!("{}/multi-search", &self.host), - Method::Post { body, query: () }, - 200, - ) - .await + self.post_multi_search(body).await }This keeps one single source-of-truth for the HTTP details and scales if more multi-search flavours appear later.
src/search.rs (6)
368-377: Consider documenting federation-specific weight semantics
QueryFederationOptions::weightis the only supported option for now, but its valid range/behaviour (e.g. 0–1? negative allowed?) isn’t obvious from the type alone. A short doc comment and, if applicable,#[serde(skip_serializing_if = "is_zero")]would improve ergonomics.
617-624: Guard against accidental use outside a federated query
with_federation_optionscan be called on anySearchQuery, including ones that will later be executed as single-index searches – the backend will then choke on the unexpectedfederationfield.To make misuse impossible you could:
- Gate the method behind a feature flag on
FederatedMultiSearchQueryonly, or- Make it
pub(crate)and funnel all access throughMultiSearchQuery::with_federation.Either way the public surface stays self-consistent.
666-676: Ownership ergonomics: return&mut selfinwith_federationConsuming
selfforces a temporary variable:let resp = client.multi_search() .with_search_query(q1) .with_search_query(q2) .with_federation(fed_opts) // returns FederatedMultiSearchQuery .execute::<Foo>() .await?;Returning
FederatedMultiSearchQueryby value is fine, but to stay symmetrical with the other builder methods you could implementwith_federation_mut(&mut self, …) -> &mut Selfas sugar.
Not critical, just API nicety.
690-698: Missing#[serde(bound(serialize = \"\"))]on the whole structLike
MultiSearchQuery,FederatedMultiSearchQuerycan end up with an unnecessaryHttp: Serializebound when the derive macro kicks in. Only thequeriesfield is patched – adding the same attribute to the struct ensures consistency:-#[derive(Debug, Serialize, Clone)] +#[derive(Debug, Serialize, Clone)] +#[serde(bound(serialize = ""))] pub struct FederatedMultiSearchQuery …
702-713: Nit: useVec<String>forfacets_by_indexvalues?
facets_by_indexisHashMap<String, Vec<String>>, but facet names are usually&strand never repeated, soHashSetorVecis fine. If order doesn’t matter a set avoids duplicates.Not blocking – purely optional.
716-725: Propagate lifetimes correctly inexecuteThe method returns
Result<FederatedMultiSearchResponse<T>, Error>by borrowingselffor'a, but the response doesn’t referenceself. You can shorten the borrow to&selflike the non-federated variant:- pub async fn execute<T: 'static + DeserializeOwned + Send + Sync>( - &'a self, + pub async fn execute<T: 'static + DeserializeOwned + Send + Sync>( + &self, ) -> Result<FederatedMultiSearchResponse<T>, Error> {No semantic change, but simpler signature.
🛑 Comments failed to post (1)
src/search.rs (1)
69-72:
⚠️ Potential issue
weighted_ranking_scoreis treated as mandatory – will panic on missing dataThe Meilisearch docs currently don’t guarantee that
_federation.weighted_ranking_scoreis always present (the comment below even hints at that).
Deserialising into a non-Option<f32>will make the whole response fail if that single field is omitted in a minor server update.- pub weighted_ranking_score: f32, + /// Score contributed by the federation weighting. + /// Making this optional avoids hard-fail deserialisation if the server omits it. + #[serde(default)] + pub weighted_ranking_score: Option<f32>,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/search.rs around lines 69 to 72, the field weighted_ranking_score is currently defined as a non-optional f32, which will cause a panic if the field is missing in the response. Change the type of weighted_ranking_score to Option<f32> to safely handle cases where this field is absent, preventing deserialization failures and panics.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/search.rs (1)
636-1406: 🛠️ Refactor suggestionConsider adding tests for the federated search functionality.
The code has good test coverage for other search features, but there are no tests for the new federated search functionality. Adding tests would help verify that everything works as expected and provide examples of how to use the new features.
Would you like me to help draft test cases for the federated search functionality?
🧹 Nitpick comments (1)
src/search.rs (1)
751-759: Fix typo in TODO comment and consider field optionality.There's a typo in the comment on line 757 ("TOOD" should be "TODO"). Also, if
weighted_ranking_scoremight be optional as the comment suggests, consider usingOption<f32>for the field type.- // TOOD: not mentioned in the docs, is that optional? - pub weighted_ranking_score: f32, + // TODO: not mentioned in the docs, is that optional? + pub weighted_ranking_score: Option<f32>,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
src/client.rs(2 hunks)src/search.rs(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration-tests
🔇 Additional comments (9)
src/search.rs (9)
69-71: LGTM: Clear field addition for federation metadata.This new field enables storing federation-related information on search results, correctly following the pattern used for other metadata fields with proper serde attribute and documentation.
368-376: LGTM: Federation options field and struct look good.The federation options field is properly marked as crate-internal with appropriate serialization handling, and the QueryFederationOptions struct is well-structured with the needed weight field.
409-409: LGTM: Default initialization for federation_options.Properly initializing the new field as None in the constructor.
617-624: LGTM: Builder method for federation options.This follows the established builder pattern consistently with other methods in the class.
636-646: LGTM: Serialization configuration for MultiSearchQuery.The custom serde bound is correctly used here as explained in the previous review comment.
667-676: LGTM: Transformation method from MultiSearchQuery to FederatedMultiSearchQuery.This provides a clean way to transform a regular multi-search query into a federated one, following the Builder pattern approach.
690-698: LGTM: FederatedMultiSearchQuery struct definition.The structure properly includes all necessary fields with correct serialization configuration.
700-713: LGTM: FederationOptions struct with clear documentation.The struct provides all the necessary options for federation with a helpful link to the documentation.
715-725: LGTM: Execute method implementation for FederatedMultiSearchQuery.This implementation correctly delegates to the client's method for executing federated searches.
| /// Returned by federated multi search. | ||
| #[derive(Debug, Deserialize, Clone)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct FederatedMultiSearchResponse<T> { | ||
| /// Merged results of the query. | ||
| pub hits: Vec<SearchResult<T>>, | ||
|
|
||
| // TODO: are offset, limit and estimated_total_hits really non-optional? In | ||
| // my tests they are always returned, but that's not a proof. | ||
| /// Number of documents skipped. | ||
| pub offset: usize, | ||
| /// Number of results returned. | ||
| pub limit: usize, | ||
| /// Estimated total number of matches. | ||
| pub estimated_total_hits: usize, | ||
|
|
||
| /// Distribution of the given facets. | ||
| pub facet_distribution: Option<HashMap<String, HashMap<String, usize>>>, | ||
| /// facet stats of the numerical facets requested in the `facet` search parameter. | ||
| pub facet_stats: Option<HashMap<String, FacetStats>>, | ||
| /// Processing time of the query. | ||
| pub processing_time_ms: usize, | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address TODO comment about field optionality.
There's an open question about whether offset, limit, and estimated_total_hits fields should be optional. Consider investigating this further to ensure the API is consistent and robust.
🏁 Script executed:
#!/bin/bash
# Check Meilisearch API documentation and actual responses to determine if these fields are always present
# Search the codebase for clues about these fields' optionality in other response structures
echo "Checking other response structures for field optionality patterns:"
grep -r "pub offset:" --include="*.rs" .
grep -r "pub limit:" --include="*.rs" .
grep -r "pub estimated_total_hits:" --include="*.rs" .
# Look for any test files that might verify these fields
echo -e "\nSearching for tests that might verify these fields:"
grep -r "offset" --include="*test*.rs" .
grep -r "limit" --include="*test*.rs" .
grep -r "estimated_total_hits" --include="*test*.rs" .Length of output: 1546
Align optionality of offset, limit, and estimated_total_hits across response structs
We currently see both plain and Option<usize> definitions for these fields in several places—this should be standardized based on the API spec:
• src/documents.rs
– pub offset: u32 vs pub offset: Option<usize>
– pub limit: u32 vs pub limit: Option<usize>
• src/indexes.rs
– same pattern for offset and limit
• src/key.rs
– same pattern for offset and limit
• src/search.rs
– other search structs use pub offset: Option<usize>, pub limit: Option<usize>, and pub estimated_total_hits: Option<usize>
Action items:
- Check the Meilisearch API docs (or real responses) to confirm whether these fields are always returned for federated multi‐search.
- If guaranteed, change them to non‐optional (
usizeoru32) everywhere and remove theTODO. - If they can be omitted, update this struct to use
Option<usize>, remove theTODO, and add/update tests accordingly.
🤖 Prompt for AI Agents
In src/search.rs around lines 727 to 749, the fields offset, limit, and
estimated_total_hits are currently non-optional but there is uncertainty if they
are always present in federated multi-search responses. Investigate the
Meilisearch API documentation or actual API responses to confirm if these fields
are guaranteed. If always present, keep them as non-optional usize and remove
the TODO comment. If they can be omitted, change these fields to Option<usize>,
remove the TODO, and update or add tests to reflect this optionality for
consistency with other search response structs.
Pull Request
Related issue
Fixes #609
What does this PR do?
This PR adds types and methods to use the federated multi search API. There are multiple ways to add this to this library, depending on how strictly one wants to type everything. I decided to:
FederatedMultiSearchResponse, which also required a newClient::execute_federated_multi_search_query. The response of federated searches is just very different from normal multi searches, and I didn't think having an enum returned byexecute_multi_search_querywas a particularly nice design (and it would have been a breaking change).federation: Option<FederationHitInfo>to eachSearchResult(which isNonefor non-federated searches). I don't think it's worth to have a dedicatedFederatedSearchResult.MultiSearchQuery::with_federationwhich returns a newFederatedMultiSearchQuery. One could also changeMultiSearchQueryto be able to represent federated queries, but I had a slight preference for my solution.PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
New Features
Documentation