-
Notifications
You must be signed in to change notification settings - Fork 3.8k
enhance: [2.6][cp]support search_by_pk for BM25 #47012
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.6
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liliu-z The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary of ChangesHello @liliu-z, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the search capabilities by introducing support for Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
[ci-v2-notice] To rerun ci-v2 checks, comment with:
If you have any questions or requests, please contact @zhikunyao. |
|
[INFO] PR Label Summary by Default
[WARNING] Milestone not set
You can set milestone by commenting: Use /refresh-label to update related check and label manually |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request enhances Milvus by adding support for search by primary key (PK) for BM25, a feature that was previously unsupported. The changes correctly identify when a search request is for a BM25 field and modify the query to fetch the underlying text data instead of vector data. The Python integration tests have been updated to validate this new functionality. The implementation is solid, and I have one suggestion to improve its robustness.
| if len(bm25Function.InputFieldNames) == 0 { | ||
| return merr.WrapErrParameterInvalidMsg("BM25 function has no input field") | ||
| } | ||
| fieldToFetch = bm25Function.InputFieldNames[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current code checks if bm25Function.InputFieldNames is empty, but it doesn't handle the case where there might be more than one input field. While a BM25 function is expected to have a single text input, it would be more robust to enforce this constraint here to prevent unexpected behavior if a misconfigured function is used. I suggest checking for exactly one input field.
| if len(bm25Function.InputFieldNames) == 0 { | |
| return merr.WrapErrParameterInvalidMsg("BM25 function has no input field") | |
| } | |
| fieldToFetch = bm25Function.InputFieldNames[0] | |
| if len(bm25Function.InputFieldNames) != 1 { | |
| return merr.WrapErrParameterInvalidMsg(fmt.Sprintf("BM25 function must have exactly one input field, but has %d", len(bm25Function.InputFieldNames))) | |
| } | |
| fieldToFetch = bm25Function.InputFieldNames[0] |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.6 #47012 +/- ##
==========================================
+ Coverage 74.51% 76.73% +2.22%
==========================================
Files 1395 1920 +525
Lines 217975 300343 +82368
==========================================
+ Hits 162415 230461 +68046
- Misses 48127 62451 +14324
+ Partials 7433 7431 -2
🚀 New features to boost your workflow:
|
|
/ci-rerun-code-check |
|
/ci-rerun-e2e-default |
issue: milvus-io#39157 Signed-off-by: Li Liu <[email protected]>
875ec82 to
678a8f9
Compare
|
[INFO] PR Label Summary by Default
[WARNING] Milestone not set
You can set milestone by commenting: Use /refresh-label to update related check and label manually |
issue: #39157
pr: #46937