-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Support search_type
in Rank Evaluation API
#48542
Conversation
Adding support for the `search_type` request parameter to the Ranking Evaluation API since this parameter can impact the ranking and the metric score and should be choosen in the same way when evaluating the search as later in the real search. Closes elastic#48503
Pinging @elastic/es-search (:Search/Ranking) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one question.
@@ -39,12 +39,9 @@ | |||
import org.elasticsearch.tasks.Task; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't seem to be for this pr ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I pulled this out to #48523 and didn't delete it here.
* The a string representation search type to execute, defaults to {@link SearchType#DEFAULT}. Can be one of | ||
* "dfs_query_then_fetch"/"dfsQueryThenFetch", "dfs_query_and_fetch"/"dfsQueryAndFetch", "query_then_fetch"/"queryThenFetch", and | ||
* "query_and_fetch"/"queryAndFetch". | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really needed ? It should be enough to use the SearchType
enum ? If not, then the comment should be changed since the types are restricted to dfs_query_then_fetch
and query_then_fetch
.
@jimczi thanks for the review, I pushed some changes that should adress your comments. |
@elasticmachine update branch |
// */ | ||
// public void searchType(String searchType) { | ||
// searchType(SearchType.fromString(searchType)); | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to remove ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doh!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@elasticmachine run elasticsearch-ci/2 |
Adding support for the `search_type` request parameter to the Ranking Evaluation API since this parameter can impact the ranking and the metric score and should be choosen in the same way when evaluating the search as later in the real search. Closes elastic#48503
Adding support for the `search_type` request parameter to the Ranking Evaluation API since this parameter can impact the ranking and the metric score and should be choosen in the same way when evaluating the search as later in the real search. Closes #48503
Adding support for the
search_type
request parameter to the Ranking EvaluationAPI since this parameter can impact the ranking and the metric score and should
be choosen in the same way when evaluating the search as later in the real
search.
Closes #48503