Skip to content
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

Added the similarity parameter to the KnnQuery type #2261

Merged
merged 4 commits into from
Sep 27, 2023

Conversation

pstrsr
Copy link
Contributor

@pstrsr pstrsr commented Aug 31, 2023

Fix for: #2149
and: elastic/elasticsearch-java#643
and: elastic/elasticsearch-java#654

Simplified version of: #2224, so that people can use vector search in their Java services.

@mshameti
Copy link

mshameti commented Sep 3, 2023

It might make sense to just hold off or adjust based on the API changes that will stem from:
elastic/elasticsearch#98916

@pstrsr
Copy link
Contributor Author

pstrsr commented Sep 4, 2023

I guess that makes sense.

It is always possible to not use the offical elastic clients in the meantime as a workaround, so it is not super critical I suppose.

@mshameti
Copy link

mshameti commented Sep 5, 2023

In JS, you can still use the official elastic client of the =>8.8.0 version which is when similarity parameter was launched https://www.elastic.co/guide/en/elasticsearch/reference/8.8/knn-search.html.

you'd just need to //@ts-ignore if you're using TS or amend the types with a .d.ts file.

@pstrsr
Copy link
Contributor Author

pstrsr commented Sep 5, 2023

We are sadly using Java for our query services, so that is not possible. But until the feature is fully supported by Elastic, we can use the rest client.

Maybe a hint in the documentation that the vector search is not stable would be nice.

The python client also works at the moment, if that helps anyone. We have used it for some prototypes with semantic search.

@bijela-gora bijela-gora mentioned this pull request Sep 20, 2023
3 tasks
@@ -38,6 +38,8 @@ export interface KnnQuery {
boost?: float
/** Filters for the kNN search query */
filter?: QueryContainer | QueryContainer[]
/** The minimum similarity for a vector to be considered a match */
similarity?: double

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
similarity?: double
similarity?: float

According to the docs it should be float.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you, changed it

@bijela-gora
Copy link

@mshameti @pstrsr hello

Can we also make k and num_candidates to have integer parameters in this MR?

@bijela-gora
Copy link

@mshameti hello, you wrote

It might make sense to just hold off

It may also make sense to align the capabilities of client libraries with those of the HTTP API. What do you think?

@bijela-gora
Copy link

@JoshMock hello

Just a kind reminder that we are one click away from improving the API.

@JoshMock JoshMock merged commit 0fb6d8e into elastic:main Sep 27, 2023
5 checks passed
github-actions bot pushed a commit that referenced this pull request Sep 27, 2023
Co-authored-by: Josh Mock <joshua.mock@elastic.co>
(cherry picked from commit 0fb6d8e)
github-actions bot pushed a commit that referenced this pull request Sep 27, 2023
Co-authored-by: Josh Mock <joshua.mock@elastic.co>
(cherry picked from commit 0fb6d8e)
github-actions bot pushed a commit that referenced this pull request Sep 27, 2023
Co-authored-by: Josh Mock <joshua.mock@elastic.co>
(cherry picked from commit 0fb6d8e)
@JoshMock
Copy link
Member

Thanks for the reminder @bijela-gora. I've merged and backported this to 8.8, 8.9 and 8.10 branches so we should expect this to be fixed in each client in the next patch release for any of those branches, or at the very least 8.11.0.

@bijela-gora
Copy link

@JoshMock thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants