-
Notifications
You must be signed in to change notification settings - Fork 188
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
Add searches on multiple segments for dense vector #454
Add searches on multiple segments for dense vector #454
Conversation
With introduction of concurrent search across multiple segments elastic/elasticsearch#98204 there is a need to measure search across multiple segments before force merge. This PR adds this operation.
Results from my M1 Mac laptop
Full results:
|
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.
Thanks Mayya.
A few comments:
- Should we also run the ann recall with multiple segments ?
- Why not running knn-search-10-100 (in addition to 100-1000)?
We should also add a test for the multi-threaded search.
We can (de)activate the setting with:
{
"name": "disable_search_worker_thread",
"tags": ["setup"],
"operation": {
"operation-type": "raw-request",
"method": "PUT",
"path": "/_cluster/settings",
"body": {
"transient": {
"search.worker_threads_enabled" : false
}
},
"include-in-reporting": false
}
}
Can be a follow up though so feel free to ignore if you prefer to keep this PR smaller.
Is it still necessary to test without concurrency? The |
I think we're passed that but I thought that it would be nice to expose this in our benchmarks. I am fine if we don't though since we'll still be able to find regression. |
Sorry for the late follow-up, this completely slipped my radar. @jimczi I've addressed you comments in 0a53e3a, adding Results from the latest run:
|
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.
LGTM
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.
LGTM
With introduction of concurrent search across multiple segments elastic/elasticsearch#98204 there is a need to measure search across multiple segments before force merge.
With introduction of concurrent search across multiple segments elastic/elasticsearch/pull/98204 there is a need to measure search across multiple segments before force merge.
This PR adds this operation.