feat(query): make hybrid_search ranker optional with RRF default#107
feat(query): make hybrid_search ranker optional with RRF default#107marlon-costa-dc wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: marlon-costa-dc The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@marlon-costa-dc Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco. |
|
@marlon-costa-dc Please associate the related issue to the body of your Pull Request. (eg. “issue: #187”) |
| /// | ||
| /// let ranker = WeightedRanker::new(vec![0.7, 0.3]); | ||
| /// let results = client.hybrid_search("my_collection", vec![req1, req2], Box::new(ranker), None).await?; | ||
| /// let results = client.hybrid_search("my_collection", vec![req1, req2], Some(Box::new(ranker)), None).await?; |
There was a problem hiding this comment.
The new doc example at L1417 moves req1 and req2 into vec![req1, req2], then L1420 reuses them in a second call. AnnSearchRequest derives Clone but the example never calls .clone(), so this demonstrates use-after-move to users. Either split into independent examples or clone the requests before the second call.
| }; | ||
|
|
||
| let ids: Vec<String> = ids.into_iter().map(|x| x.into()).collect(); | ||
| let ids: Vec<String> = ids.into_iter().collect(); |
There was a problem hiding this comment.
PR simplified this line from .map(|x| x.into()).collect() to .into_iter().collect(), but ids is already Vec<String>, so the collect is still a pointless identity reallocation. Since the PR intentionally touched this line to simplify it, the simplification should be completed by deleting the line entirely.
| /// let options = SearchOptions::new().ranker(Box::new(WeightedRanker::new(vec![0.7, 0.3]))); | ||
| /// let results = client.hybrid_search("my_collection", vec![req1, req2], None, Some(options)).await?; | ||
| /// ``` | ||
| pub async fn hybrid_search<S>( |
There was a problem hiding this comment.
The third parameter of hybrid_search() changed from Box<dyn BaseRanker> to Option<Box<dyn BaseRanker>>. All existing external callers must wrap their argument with Some(...) or their code will fail to compile. While 0.1.0 SemVer allows breaking changes and the error is caught at compile time, this should be explicitly declared in release notes so downstream users are aware.
| where | ||
| S: Into<String>, | ||
| { | ||
| let options = options.unwrap_or_default(); |
There was a problem hiding this comment.
The new priority logic ranker.or(options.ranker).unwrap_or_else(|| RrfRanker(60.0)) at L1432-1435 introduces three distinct branches (explicit parameter, options.ranker, default RRF) but none are covered by unit or integration tests. This core public API behavior is vulnerable to accidental regression during future refactoring.
yhmo
force-pushed
the
feat/optional-hybrid-search-ranker
branch
from
6af654b to
2d4b618
Compare
yhmo
force-pushed
the
feat/optional-hybrid-search-ranker
branch
from
2d4b618 to
a068d69
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the Client::hybrid_search API to make the ranker parameter optional, simplifying the common usage by defaulting to Reciprocal Rank Fusion (RRF) when no ranker is explicitly provided.
Changes:
- Changed
hybrid_searchto acceptranker: Option<Box<dyn BaseRanker>>and select an effective ranker from (param → options.ranker → defaultRrfRanker(k=60.0)). - Added
SearchOptions::ranker(...)to allow configuring the ranker via options instead of always passing it tohybrid_search. - Performed a few small refactors/cleanups (derive
DefaultforQueryOptions, minor idiomatic Rust simplifications, and slice-based params helpers).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let options = options.unwrap_or_default(); | ||
| let effective_ranker = ranker | ||
| .or(options.ranker) |
Summary
rankerparameter inhybrid_searchanOption<Box<dyn BaseRanker>>.Noneis provided, it falls back tooptions.rankeror defaults toRrfRankerwithk=60.0.