Add rescore knn vector test coverage #122801
Conversation
…quantized codecs, check doc ordering
…tor-query-test-coverage
carlosdelest
added
>test
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
| for (int i = 1; i < docs.length; i++) { | ||
| assert docs[i - 1] < docs[i] : "doc ids are not in order: " + Arrays.toString(docs); | ||
| } | ||
| } |
There was a problem hiding this comment.
it sounds like this is redundant if we have appropriate test coverage? I was also wondering if it may be worth changing the two first arguments into a ScoreDoc[] given that's how stuff comes in, and perhaps unifying the sorting here. I realize though that this is a copy of a Lucene class and the change I am suggesting will make it diverge from its original source.
There was a problem hiding this comment.
it sounds like this is redundant if we have appropriate test coverage?
My thinking was to provide a way to understand a test failure in an easier way in case someone provided a non-sorted array, instead of going through all the investigations that you had to do 😓
I'm happy with removing the assertion in case you think it's unnecessary, but I think it helps to understand what the preconditions for this constructor are.
I was also wondering if it may be worth changing the two first arguments into a ScoreDoc[] given that's how stuff comes in, and perhaps unifying the sorting here. I realize though that this is a copy of a Lucene class and the change I am suggesting will make it diverge from its original source.
I think that's a good idea. I will give it a try.
I realize though that this is a copy of a Lucene class and the change I am suggesting will make it diverge from its original source.
It already diverges a bit in terms of making it easier to create - as long as it's on the constructor stuff I think we should be good for doing the change.
I'll give it a go and come back for feedback.
There was a problem hiding this comment.
We should have a test for RescoreKnnVectorQuery that indexes a bunch of random vectors, searches with a random vector and asserts the rewrite is a KnnScoreDocQuery with the appropriately ordered values.
It seems we are almost there in RescoreKnnVectorQueryTests, but maybe add some assertions there. Maybe via package private methods?
There was a problem hiding this comment.
it may be worth changing the two first arguments into a ScoreDoc[] given that's how stuff comes in, and perhaps unifying the sorting here
@javanna I gave it a try in a073f43 - I like it more, it simplifies how clients create this query plus we enforce the invariant in the constructor itself 💯
We should have a test for RescoreKnnVectorQuery that indexes a bunch of random vectors, searches with a random vector and asserts the rewrite is a KnnScoreDocQuery with the appropriately ordered values.
@benwtrent I think the change in a073f43 makes it unnecessary. We're already checking via random insertions in the test. Do you think we need to add something else to make sure this doesn't bite us again?
There was a problem hiding this comment.
doing the sort in the ctor is fine and as long as we have tests that fill fail if somebody removes that sort, I am happy.
There was a problem hiding this comment.
RescoreKnnVectorQueryIT add those tests. I checked by removing the sort that Luca added back in #122653 that this was caught by the newly added tests.
There was a problem hiding this comment.
I like it. It also allows to share some code between the two consumers. Perhaps make it clear in the javadocs that this is no longer a straight copy of its lucene sibling. Thanks!
There was a problem hiding this comment.
Perhaps make it clear in the javadocs that this is no longer a straight copy of its lucene sibling.
👍 I've clarified that in ee464fe
…ys on the clients
…tor-query-test-coverage
|
@elasticsearchmachine update branch |
💔 Backport failed
You can use sqren/backport to manually backport by running |
(cherry picked from commit f5e2a92)
(cherry picked from commit f5e2a92) # Conflicts: # server/src/main/java/org/elasticsearch/search/vectors/RescoreKnnVectorQuery.java # server/src/test/java/org/elasticsearch/search/vectors/RescoreKnnVectorQueryTests.java
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
(cherry picked from commit f5e2a92) # Conflicts: # server/src/main/java/org/elasticsearch/search/vectors/RescoreKnnVectorQuery.java # server/src/test/java/org/elasticsearch/search/vectors/RescoreKnnVectorQueryTests.java
Closes #122723
#122653 fixed a scoring bug for knn rescore. This bug should have been caught earlier, as scores were not being sorted when retrieved from knn search.
This PR adds an integration test for kNN section, kNN query and kNN retriever to double check that documents added to multiple shards, in random order, can be successfully rescored.