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

Knn Search support #28

Merged
merged 5 commits into from
Sep 26, 2024
Merged

Conversation

hkulekci
Copy link

@hkulekci hkulekci commented Oct 7, 2023

Solves #27.

return $output;
}

return [];
Copy link
Author

Choose a reason for hiding this comment

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

Elasticsearch started supporting multiple Knn query with version 8.7. So, to be able to support the versions between 8.7 and 8.4, I just put these if controls. It will return the Knn query itself if the container has just one Knn query. The container has multiple Knn query, it will return an array of Knn queries.

@@ -50,7 +50,7 @@ jobs:

services:
elasticsearch:
image: elasticsearch:8.0.0
image: elasticsearch:8.4.0
Copy link
Author

Choose a reason for hiding this comment

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

KNN support is moved from _knn_search to _search endpoint in version 8.4.0, so for this reason, I change the elasticsearch version of Github CI as 8.4.0. Knn query support will be provided for elasticsearch 8.4.0 and above. :|

@hkulekci
Copy link
Author

Ping @alexander-schranz

@alexander-schranz
Copy link
Member

I did not have time to have deeper look at it just by your comment in #27. I did see:

$knn->setFilter($matchAll);

If I'm looking at the existing API I would expect here addFilter:

$knn->addFilter($matchAll);

But else I think the params make sense.

@hkulekci
Copy link
Author

After giving it some thought, I believe we should stick with setFilter as there is no support for multiple filters for Knn search. Instead, we can set a filter as a bool query. Using addFilter can be misleading as it may give the impression that we can add multiple filters for Knn. However, the filter field of Knn only accepts one query. And enrichment comes with bool query as well.

@hkulekci
Copy link
Author

Ping @alexander-schranz

@hkulekci
Copy link
Author

hkulekci commented Nov 11, 2023

Hey @alexander-schranz, could you find a chance to look into this PR? I hope we can find a chance to merge this.

Thanks.

@hkulekci
Copy link
Author

@alexander-schranz I've made some additional improvements to implement a solution for KNN and Sparse Vector search as the query. I hope we can find a way to merge this PR into the library.

hkulekci#6

@alexander-schranz alexander-schranz merged commit d61ea65 into handcraftedinthealps:8.x Sep 26, 2024
8 checks passed
@alexander-schranz
Copy link
Member

Released as 8.2.0. Thx @hkulekci

@hkulekci hkulekci deleted the knn-support branch September 26, 2024 10:04
@hkulekci
Copy link
Author

hkulekci commented Sep 26, 2024

Oh, really! Thanks for merging! I need to prepare another PR as a next step. Will ping you again.

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

Successfully merging this pull request may close these issues.

2 participants