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

Fail query when a sort is provided in conjunction with rescorers #26510

Merged
merged 3 commits into from Sep 7, 2017

Conversation

Projects
None yet
5 participants
@jimczi
Copy link
Member

commented Sep 5, 2017

This change fixes a regression introduced in 6 that removes the skipping of the rescore phase
when a sort other than _score is used.
This commit also adds an assert that checks if the topdocs are sorted by _score after the rescoring.
This is the responsibility of the rescorer to make sure that topdocs are sorted after rescore so we
just check that this condition is met in the rescore phase.

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2017

What if the sort includes the score? I'm wondering whether we should fail requests that both provide a sort and a rescore element?

@jimczi

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2017

I'm wondering whether we should fail requests that both provide a sort and a rescore element?

Probably better than just skipping the rescore phase even though this is documented:
https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-rescore.html
I'll update the PR.

What if the sort includes the score?

I think we should fail if there is a sort criteria other than _score. Rescorers need to resort the topdocs and if they do it wrong this can break the merge sort in the coordinating node. With custom rescorers I think it's good to keep it simple, rescorers work only when sort is not provided and fail otherwise ?

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2017

What if the sort includes the score?

I imagine that is pretty broken right now. I wonder if it'd be ok to not allow rescore if the sort isn't just on score. Like you suggest, failing the whole request rather than skipping the rescore.

jimczi added some commits Sep 5, 2017

Rescorer phase should be skipped when a sort is provided
This change fixes a regression introduced in 6 that removes the skipping of the rescore phase
when a sort other than _score is used.
This commit also adds an assert that checks if the topdocs are sorted by _score after the rescoring.
This is the responsibility of the rescorer to make sure that topdocs are sorted after rescore so we
just check that this condition is met in the rescore phase.
failed the request when a sort is provided in conjunction with rescor…
…e instead of just skipping the rescore phase
@jpountz

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2017

I wonder if it'd be ok to not allow rescore if the sort isn't just on score.

+1 to that approach

@jimczi jimczi force-pushed the jimczi:rescore_sort_by_score_only branch to 321b055 Sep 7, 2017

@jimczi jimczi changed the title Rescorer phase should be skipped when a sort is provided Fail query when a sort is provided in conjunction with rescorers Sep 7, 2017

@jimczi

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2017

Thanks @jpountz and @nik9000
I updated the title of the PR and pushed a change that fails the request when a sort is provided in conjunction with a rescorer.

@jpountz

jpountz approved these changes Sep 7, 2017

@jimczi jimczi merged commit abe83c4 into elastic:master Sep 7, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

@jimczi jimczi deleted the jimczi:rescore_sort_by_score_only branch Sep 7, 2017

jimczi added a commit that referenced this pull request Sep 7, 2017

Fail query when a sort is provided in conjunction with rescorers (#26510
)

This change fixes a regression introduced in 6 that removes the skipping of the rescore phase
when a sort other than _score is used.
We now fail the request when a sort is provided in conjunction with rescore instead of just skipping the rescore phase
This commit also adds an assert that checks if the topdocs are sorted by _score after the rescoring.
This is the responsibility of the rescorer to make sure that topdocs are sorted after rescore so we
just check that this condition is met in the rescore phase.

jimczi added a commit that referenced this pull request Sep 7, 2017

Fail query when a sort is provided in conjunction with rescorers (#26510
)

This change fixes a regression introduced in 6 that removes the skipping of the rescore phase
when a sort other than _score is used.
We now fail the request when a sort is provided in conjunction with rescore instead of just skipping the rescore phase
This commit also adds an assert that checks if the topdocs are sorted by _score after the rescoring.
This is the responsibility of the rescorer to make sure that topdocs are sorted after rescore so we
just check that this condition is met in the rescore phase.

@colings86 colings86 added v6.0.0-rc1 and removed v6.0.0 labels Sep 22, 2017

@lcawl lcawl removed the v6.1.0 label Dec 12, 2017

@cbuescher cbuescher referenced this pull request Jun 27, 2018

Closed

Rescorer is ignored #31429

@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.