Skip to content

Does elasticsearch-dsl-py support knn_search? #1679

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

Closed
shandou opened this issue Oct 26, 2023 · 6 comments · Fixed by #1691
Closed

Does elasticsearch-dsl-py support knn_search? #1679

shandou opened this issue Oct 26, 2023 · 6 comments · Fixed by #1691
Assignees
Labels
good first issue Good for new contributors

Comments

@shandou
Copy link

shandou commented Oct 26, 2023

Great to see that this package now supports 8+ versions of Elasticsearch ❤️

However, when perusing the documentation, I am not yet seeing explicit support to knn_search similar to the lower-level python client. Curious about:

  1. Is it indeed the case that knn_search is not yet supported?
  2. If so, is it among the features that may be added in the near future?

Thank you very much!

@pquentin
Copy link
Member

Hello! Glad that you're happy with elasticsearch-dsl-py 8.x.

Note that the page you linked to mentions that knn_search is deprecated in favor of the knn option in the search API. I would suggest supporting that instead, which means adding a knn method to the Search class, like this pull request is doing for collapse: #1649. How does that sound to you?

I may get to it in the future, but for now I'm happy to receive contributions!

@pquentin pquentin added the good first issue Good for new contributors label Oct 27, 2023
@shandou
Copy link
Author

shandou commented Oct 27, 2023

Thank you very much for clarifying! (I completely missed the deprecation note until you have pointed it out)

The collapse implementation is one way for sure! I am very fond of the syntax offered by the Q shortcut and wonder if a knn type could be added to it as well. But unfortunately at this very moment I haven't thought through how exactly the syntax could look like for a Q shortcut that does ANN search.

So I look forward to seeing how things turn out for #1649, and in the meantime, I will think about how this may look like for the Q shortcut and come back with a more specific proposal (and if possible, to also help contribute).

Thanks!

@pquentin
Copy link
Member

@honzakral Any opinion on Search vs Q?

@honzakral
Copy link
Contributor

@pquentin Q is specifically for queries whereas knn is its own thing. For example there is no way to combine it with other Q objects (Q("match", message="hello world") | Q("knn", ...) makes no sense) so I don't think it is a good match.

A .knn() method seems a way to go for sure here.

@shandou
Copy link
Author

shandou commented Oct 28, 2023

Appreciate the discussion! :) One quick question to @honzakral: what about a potential use case of combining lexical matching with knn via an OR condition in the context of lexical-vector hybrid?

@honzakral
Copy link
Contributor

@shandou based on https://www.elastic.co/guide/en/elasticsearch/reference/current/knn-search.html#_combine_approximate_knn_with_other_features I'd say the natural way would be to just define both query and knn:

s = Search(index="...")
s = s.query("match", whatever_field="some query")
s = s.knn(filter=Q("terms", tags=["why", "not", "filter", "knn", "too", ":)"]), ...)

It might not be the simplest expression, but it mirrors exactly the logic of the dsl where different options in the body of the search request are pretty much mapped to methods so if someone is familiar with the JSON API they will be able to anticipate the pattern I hope...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for new contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants