Skip to content

Disallow explicitly setting search_type with knn search#88504

Merged
jtibshirani merged 2 commits intoelastic:knn-searchfrom
jtibshirani:knn-search-type
Jul 20, 2022
Merged

Disallow explicitly setting search_type with knn search#88504
jtibshirani merged 2 commits intoelastic:knn-searchfrom
jtibshirani:knn-search-type

Conversation

@jtibshirani
Copy link
Copy Markdown
Contributor

@jtibshirani jtibshirani commented Jul 13, 2022

When kNN search is enabled, it automatically uses the dfs_query_then_fetch
search type. The user can no longer control the search type that's used. This
commit throws an error if search_type is explicitly set when knn search is
used.

Addresses #87625.

@jtibshirani jtibshirani added >non-issue :Search/Search Search-related issues that do not fall into other categories v8.4.0 labels Jul 13, 2022
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Jul 13, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Jul 13, 2022

Pinging @elastic/es-search (Team:Search)

@jtibshirani
Copy link
Copy Markdown
Contributor Author

jtibshirani commented Jul 13, 2022

I initially tried a strategy where we check this inside SearchRequest#validate. But it is not possible to tell whether the search type was explicitly set by the user, or just set to the default value. We could update SearchRequest to use Explicit<SearchType>, but this was intrusive -- there are questions over wire serialization, and we need multiple different setters for setting "explicitly" (for user requests) vs. "implicitly" (for internal optimizations).

@jtibshirani jtibshirani mentioned this pull request Jul 13, 2022
Closed
8 tasks
mayya-sharipova
mayya-sharipova reviewed Jul 14, 2022
View reviewed changes
server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java
searchRequest.indicesOptions(IndicesOptions.fromRequest(request, searchRequest.indicesOptions()));

checkRestTotalHits(request, searchRequest);
validateSearchRequest(request, searchRequest);
Copy link
Copy Markdown
Contributor

@mayya-sharipova mayya-sharipova Jul 14, 2022

Choose a reason for hiding this comment

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

If validateSearchRequest already includes checkSearchType why do we need to call it again after validateSearchRequest?

Copy link
Copy Markdown
Contributor Author

@jtibshirani jtibshirani Jul 14, 2022

Choose a reason for hiding this comment

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

Thanks @mayya-sharipova , this was just an oversight. Will fix.

mayya-sharipova
mayya-sharipova approved these changes Jul 14, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

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

@jtibshirani Thanks, this overall LGTM, except the removal of double checking in RestSearchAction. After fixing it, this PR can be merged without further review.

@sethmlarson sethmlarson added the Team:Clients Meta label for clients team label Jul 18, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Jul 18, 2022

Pinging @elastic/clients-team (Team:Clients)

@elasticsearchmachine elasticsearchmachine removed the Team:Clients Meta label for clients team label Jul 20, 2022
@jtibshirani jtibshirani merged commit 27cbf46 into elastic:knn-search Jul 20, 2022
@jtibshirani jtibshirani deleted the knn-search-type branch July 20, 2022 23:20
@jtibshirani jtibshirani added :Search Relevance/Vectors Vector search and removed :Search/Search Search-related issues that do not fall into other categories labels Jul 21, 2022
@jtibshirani jtibshirani mentioned this pull request Jul 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mayya-sharipova mayya-sharipova mayya-sharipova approved these changes

Assignees

No one assigned

Labels

>non-issue :Search Relevance/Vectors Vector search Team:Search Meta label for search team v8.4.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jtibshirani @elasticmachine @mayya-sharipova @sethmlarson @elasticsearchmachine