Disallow kNN searches with index alias filters #79654
Conversation
This PR throws an exception for kNN searches on filetered aliases. We don't allow kNN searches on filtered aliases as currently filters are applied only after kNN searches are done, which may lead to returning less than k results. In the future, we want to apply filters while doing a kNN search. Once implemented, we will allow kNN searches on filtered aliases. Relates to elastic#78473
mayya-sharipova
added
the
:Search/Search
|
Pinging @elastic/es-search (Team:Search) |
...plugin/vectors/src/main/java/org/elasticsearch/xpack/vectors/action/RestKnnSearchAction.java
Outdated
Show resolved
Hide resolved
| @@ -699,6 +707,9 @@ private void executeSearch(SearchTask task, SearchTimeProvider timeProvider, Sea | |||
| searchService.getResponseCollectorService(), nodeSearchCounts); | |||
| final Set<String> indicesAndAliases = indexNameExpressionResolver.resolveExpressions(clusterState, searchRequest.indices()); | |||
| aliasFilter = buildPerIndexAliasFilter(clusterState, indicesAndAliases, indices, remoteAliasMap); | |||
| if (searchRequest.indicesOptions().forbidFilteredAliases()) { | |||
There was a problem hiding this comment.
Since this is being introduced as a general option, maybe we should check it at a lower layer, like in IndicesService#buildAliasFilter. The other indices options apply across endpoints, not just to search.
There was a problem hiding this comment.
Thanks for the feedback. I've addressed it in bd484b1, but I like the previous approach more – keeping it only in TransportSearchAction, as we consider indicesOptions().forbidFilteredAliases() only a temporary measure, and want to remove it later. WDYT?
There was a problem hiding this comment.
I like the current approach but don't feel strongly. It'd be great to hear the data management team's opinion here.
- Move all request building logic to KnnSearchRequestBuilder - Move a check for allowing filtered aliases to IndicesService
mayya-sharipova
added
the
:Data Management/Indices APIs
elasticmachine
added
the
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
|
@jtibshirani Thanks for the review. I've addressed your feedback! |
|
@elastic/es-data-management We wanted to check if you are ok with us introducing a new |
|
Chatted with @elastic/es-data-management, and they suggested that introducing an extra Indices options just for this use case seems to be excessive. Since we haven't found any other elegant solution, @jtibshirani and me decided to close this PR and instead to update documentation with a note that doing _knn_search on filtered aliases is not recommended, as filter is applied post knn search, and hence the number of results returned can be less < k, and 0 in the worst case. |
This PR throws an exception for kNN searches on filtered aliases.
We don't allow kNN searches on filtered aliases as currently filters are
applied only after kNN searches are done, which may lead to returning
less than k results.
In the future, we want to apply filters while doing a kNN search.
Once implemented, we will allow kNN searches on filtered aliases.
Relates to #78473