Integrate filtering support for ANN #84734
Conversation
jtibshirani
added
>enhancement
:Search/Search
|
Pinging @elastic/es-search (Team:Search) |
|
Hi @jtibshirani, I've created a changelog YAML for you. |
mayya-sharipova
left a comment
mayya-sharipova
left a comment
There was a problem hiding this comment.
@jtibshirani Thank you, great work! Overall LGTM, I've left some small comments.
x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/vectors/40_knn_search.yml
Show resolved
Hide resolved
| this.fieldName = fieldName; | ||
| this.queryVector = queryVector; | ||
| this.numCands = numCands; | ||
| this.filterQueries = List.of(); |
There was a problem hiding this comment.
Should we make the parameter filterQueries also final and ensure filterQueries is never null?
There was a problem hiding this comment.
We actually allow it to be set in the filterQueries(...) method. Since this builder is only meant to be used internally to Elasticsearch, I just chose the setter methods that made our code the cleanest.
There was a problem hiding this comment.
I am wondering, since it is a public class, and filterQueries are public methods can't this class be used by Java HLRC client? if we don't want to make filterQueries final may be just ensure that are not null when we set them? WDYT?
Thanks for other new changes, they LGTM as well.
There was a problem hiding this comment.
I don't think this can be used from the Java HLRC. Since it lives in an xpack module, it's not available by default -- for example, for pinned queries we had to add an extra query builder just for the HLRC (#45779).
I ended up reworking this to just add the filters instead of replacing them. I also added a note to the query builder warning that it's an internal class. Thanks for the ideas.
...lugin/vectors/src/main/java/org/elasticsearch/xpack/vectors/query/KnnVectorQueryBuilder.java
Show resolved
Hide resolved
| can match. The kNN search will return the top `k` documents that also match | ||
| this filter. The value can be a single query or a list of queries. If `filter` | ||
| is not provided, all documents are allowed to match. | ||
|
|
There was a problem hiding this comment.
It would be nice to have some json example as with filter as well.
There was a problem hiding this comment.
Good point, I'll actually add an Examples section at the end of these docs.
|
@elasticmachine run elasticsearch-ci/part-1 |
|
@elasticmachine run elasticsearch-ci/part-2 |
|
Awesome job! Do I understand correctly from the tag in this PR that it will be available in ElasticSearch 8.2? |
|
@coreation yes, that's the plan! |
|
Awesome! Not to press, but merely as information, is there an estimated timeframe for when 8.2 might be released? |
|
We don't usually give out release dates (even estimates), but I can say that we're hard at work on 8.2 now. |
|
Ok thanks for the info @jtibshirani I want to press again, only something I was curious about. Looking forward to the release! |
We implemented this in #84734 but forgot to update these docs.
We implemented this in #84734 but forgot to update these docs.
jtibshirani
added
:Search Relevance/Vectors
5 participants
This PR integrates support for ANN with filtering added in Lucene 9.1. It adds
a new
filtersection to the_knn_searchendpoint, which accepts a query (inthe Elasticsearch query DSL). The value can either be a single query or a list
of queries, which matches the syntax we use for defining filter clauses in a
boolquery.Closes #81788.