-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Scroll queries asking for rescore are considered invalid #32918
Merged
Merged
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
361c7a2
Unit test
not-napoleon e61810b
New validation rule
not-napoleon 9356d7d
Merge branch 'master' into fix/31775
not-napoleon 3aaa33c
changed not to == false
not-napoleon 2983371
Replaced mock with private static no op implementation
not-napoleon ab5468f
Merge branch 'master' into fix/31775
not-napoleon e1f02bf
Merge branch 'master' into fix/31775
not-napoleon 8f39b1d
Merge remote-tracking branch 'origin/master' into fix/31775
not-napoleon 3cc185e
Added documentation for the new behavior
not-napoleon d319989
Merge branch 'master' into fix/31775
not-napoleon 66e76fb
Response to PR feedback
not-napoleon 13bbb1c
fixed unused import
not-napoleon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,9 +26,15 @@ | |
import org.elasticsearch.common.io.stream.BytesStreamOutput; | ||
import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput; | ||
import org.elasticsearch.common.io.stream.StreamInput; | ||
import org.elasticsearch.common.io.stream.StreamOutput; | ||
import org.elasticsearch.common.unit.TimeValue; | ||
import org.elasticsearch.common.util.ArrayUtils; | ||
import org.elasticsearch.common.xcontent.XContentBuilder; | ||
import org.elasticsearch.index.query.QueryRewriteContext; | ||
import org.elasticsearch.index.query.QueryShardContext; | ||
import org.elasticsearch.search.builder.SearchSourceBuilder; | ||
import org.elasticsearch.search.rescore.RescorerBuilder; | ||
import org.elasticsearch.search.rescore.RescoreContext; | ||
|
||
import java.io.IOException; | ||
import java.util.ArrayList; | ||
|
@@ -123,6 +129,17 @@ public void testValidate() throws IOException { | |
assertEquals(1, validationErrors.validationErrors().size()); | ||
assertEquals("[size] cannot be [0] in a scroll context", validationErrors.validationErrors().get(0)); | ||
} | ||
{ | ||
// Rescore is not allowed on scroll requests | ||
SearchRequest searchRequest = createSearchRequest().source(new SearchSourceBuilder()); | ||
searchRequest.source().addRescorer(new NoOpRescorerBuilder()); | ||
searchRequest.requestCache(false); | ||
searchRequest.scroll(new TimeValue(1000)); | ||
ActionRequestValidationException validationErrors = searchRequest.validate(); | ||
assertNotNull(validationErrors); | ||
assertEquals(1, validationErrors.validationErrors().size()); | ||
assertEquals("using [rescore] is not allowed in a scroll context", validationErrors.validationErrors().get(0)); | ||
} | ||
} | ||
|
||
public void testEqualsAndHashcode() throws IOException { | ||
|
@@ -164,4 +181,33 @@ private static SearchRequest copyRequest(SearchRequest searchRequest) throws IOE | |
} | ||
return result; | ||
} | ||
|
||
private static class NoOpRescorerBuilder extends RescorerBuilder<NoOpRescorerBuilder> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had not looked closely now I see why you were using mockito before :) I don't mind if you prefer to go back to it, I am happy either way |
||
NoOpRescorerBuilder() throws IOException { | ||
} | ||
|
||
@Override | ||
public String getWriteableName() { | ||
return "test"; | ||
} | ||
|
||
@Override | ||
public RescorerBuilder<NoOpRescorerBuilder> rewrite(QueryRewriteContext ctx) throws IOException { | ||
return this; | ||
} | ||
|
||
@Override | ||
protected void doWriteTo(StreamOutput out) throws IOException { | ||
} | ||
|
||
@Override | ||
protected void doXContent(XContentBuilder builder, Params params) throws IOException { | ||
} | ||
|
||
@Override | ||
public RescoreContext innerBuildContext(int windowSize, QueryShardContext context) throws IOException { | ||
return null; | ||
} | ||
} | ||
|
||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
maybe replace 6 with previous versions (only this second occurrence)? also when you mention the version can you specify also the minor version that is was deprecated in ?
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've updated this language a bit. I want to be careful here, because according to the original ticket, this was allowed at some time in the 5.x line. It's not clear to me when exactly we stopped supporting rescoring scroll queries.
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.
Makes sense to me!