script_score query errors on negative scores #53133
Conversation
7.5 and 7.6 had a regression that allowed for script_score queries to have negative scores. We have corrected this regression in elastic#52478. This is an addition to elastic#52478 that adds a test and release notes.
mayya-sharipova
added
the
:Search Relevance/Ranking
|
Pinging @elastic/es-search (:Search/Ranking) |
There was a problem hiding this comment.
@mayya-sharipova thanks for making this update!
For the test, I think it'd be nice to add a case to ScriptScoreQueryTests instead of adding a new yml REST test. The behavior seems pretty straightforward to cover through a unit test, and unit tests tend to be faster to run and easier to debug than integration tests.
|
@jtibshirani Thanks for the review, I have addressed your comments in the second commit. Please continue to review. |
Remove yml test, add unit test
mayya-sharipova
force-pushed
the
script-score-negative-score-7.x
branch
from
1263ec2 to
ed6e118
Compare
| @@ -131,6 +131,14 @@ public void testExplainDefaultNoScore() throws IOException { | |||
| assertThat(explanation.getValue(), equalTo(2.0f)); | |||
| } | |||
|
|
|||
| public void testScriptScoreErrorOnNegativeScore() { | |||
There was a problem hiding this comment.
For me this set of unit tests is quite straightforward + easy to understand. I think yml tests are slower to debug and are most helpful for testing the end-to-end integration. We recently added a set of testing guidelines that I think are really helpful: https://github.com/elastic/elasticsearch/blob/master/TESTING.asciidoc#what-kind-of-tests-should-i-write
There was a problem hiding this comment.
@jtibshirani Thanks a lot, I missed these guidelines, will study them
7.5 and 7.6 had a regression that allowed for
script_score queries to have negative scores.
We have corrected this regression in #52478.
This is an addition to #52478 that adds
a test and release notes.