-
Notifications
You must be signed in to change notification settings - Fork 801
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 a knn
method to elasticsearch_dsl.search.Search
#1691
Conversation
knn
method to elasticsearch_dsl.search.Search
knn
method to elasticsearch_dsl.search.Search
5833e00
to
cc87f50
Compare
This looks good but I did not test locally yet. However, can you please add docs? Unfortunately, the docstring is not enough, you need to edit |
cc87f50
to
4cf3745
Compare
4cf3745
to
e0ecdc0
Compare
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.
Looks great! I only have a small nit.
elasticsearch_dsl/search.py
Outdated
def knn( | ||
self, | ||
field, | ||
k, | ||
num_candidates, | ||
query_vector=None, | ||
query_vector_builder=None, | ||
filter=None, | ||
similarity=None, |
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.
Looks like boost
is missing. It's specified here: https://github.com/elastic/elasticsearch-specification/blob/05c39a9dc46e107e4baaf082a6b56aae1ef5901c/specification/_types/Knn.ts#L26-L43. Documented here: https://www.elastic.co/guide/en/elasticsearch/reference/current/knn-search.html#_search_multiple_knn_fields. But weirdly missing fom https://www.elastic.co/guide/en/elasticsearch/reference/current/search-search.html#search-api-knn.
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.
@pquentin I wonder why is boost
omitted from our documentation. But in any case, I have added it now.
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! LGTM.
We need an update for this change since this PR has arrived at ElasticSearch v8.12.0 I'm trying to use "knn" inside the "query" object. But I'm getting the following error:
How could I add a new Knn class in the accepted ones? Something like # elasticsearch-dsl/query.py
class Knn(Query):
name = "knn" |
@israellias I have added #1770 to track this request. Thanks! |
Thanks @miguelgrinberg queryset = queryset.update_from_dict(
{
"query": {
"knn": {
"field": "embedding",
"query_vector": res.data[0].embedding,
"filter": filter_for_knn,
"num_candidates": 30,
}
},
}
) But I'm noticing that update_from_dict still calls Q which results in doing the same as queryset.query("knn", field="embedding", query_vector=res.data[0].embedding, filter=filter_for_knn, num_candidates=30) and that is throwing the error since the class is simply not listed |
Fixes #1679
This PR adds the
knn
option for k-nearest neighbor vector search.