-
Notifications
You must be signed in to change notification settings - Fork 3k
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
enhance: simplify the structure of search_params #38251
Conversation
@smellthemoon E2e jenkins job failed, comment |
@smellthemoon go-sdk check failed, comment |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #38251 +/- ##
==========================================
- Coverage 81.07% 81.06% -0.02%
==========================================
Files 1372 1372
Lines 191508 191558 +50
==========================================
+ Hits 155274 155291 +17
- Misses 30738 30765 +27
- Partials 5496 5502 +6
|
67d43b3
to
cd0f327
Compare
@smellthemoon E2e jenkins job failed, comment |
@smellthemoon go-sdk check failed, comment |
cd0f327
to
6f83f5c
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: smellthemoon The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
6f83f5c
to
e805bec
Compare
Signed-off-by: lixinguo <[email protected]>
e805bec
to
e9ec324
Compare
@smellthemoon go-sdk check failed, comment |
rerun go-sdk |
pageRetainOrderKey = "page_retain_order" | ||
) | ||
|
||
var ParamsKeyList = []string{ |
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.
unnecessary?
return merr.WrapErrParameterInvalidMsg(fmt.Sprintf("parameter(%s) has the wrong type, expect bool type", key)) | ||
} | ||
if v != value { | ||
return merr.WrapErrParameterInvalidMsg(fmt.Sprintf("inconsistent parameter(%s), search_param(%t),search_param.params(%t)", key, v, value)) |
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.
ambiguous
nprobeKey = "nprobe" | ||
maxEmptyResultBuckets = "max_empty_result_buckets" | ||
reorderKKey = "reorder_k" | ||
searchListKey = "search_list" |
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.
Knowhere’s param need to be transparent to MIlvus @foxspy Plz take a look
// before 2.5.1, all key in ParamsKeyList will be write in search_params.params | ||
// after 2.5.1, allow user to write all this key in search_params | ||
// SearchParams in planpb.QueryInfo is the params set passed to segcore | ||
// so if you want to use the params in segcore/knowhere, remember to add the new params into ParamsKeyList after 2.5.1 |
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.
Need add the index parameter here? This doesn't seem reasonable. It will introduce new compatibility logic. Can we just write all of params without adding a whitelist?
#37972