You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/01/03 15:19:04 UTC
[GitHub] [lucene] mayya-sharipova opened a new pull request #580: LUCENE-10351 Correct knn search failure with deleted docs
mayya-sharipova opened a new pull request #580:
URL: https://github.com/apache/lucene/pull/580
Current when doing knn search on an segment where all documents
with knn field were deleted, we get the following error:
maxSize must be > 0 and < 2147483630; got: 0
java.lang.IllegalArgumentException: maxSize must be > 0 and < 2147483630; got: 0
at __randomizedtesting.SeedInfo.seed([43F1F124D7076A4E:1B860BFCCB9B0BB5]:0)
at org.apache.lucene.util.LongHeap.<init>(LongHeap.java:57)
at org.apache.lucene.util.LongHeap$1.<init>(LongHeap.java:69)
at org.apache.lucene.util.LongHeap.create(LongHeap.java:69)
at org.apache.lucene.util.hnsw.NeighborQueue.<init>(NeighborQueue.java:41)
at org.apache.lucene.util.hnsw.HnswGraph.search(HnswGraph.java:105)#
This patch fixes this error and ensures empty TopDocs are returned when
knn field doesn't have any documents left.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jtibshirani commented on a change in pull request #580: LUCENE-10351 Correct knn search failure with deleted docs
Posted by GitBox <gi...@apache.org>.
jtibshirani commented on a change in pull request #580:
URL: https://github.com/apache/lucene/pull/580#discussion_r777682465
##########
File path: lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90HnswVectorsReader.java
##########
@@ -239,6 +239,9 @@ public TopDocs search(String field, float[] target, int k, Bits acceptDocs) thro
if (fieldEntry == null || fieldEntry.dimension == 0) {
return null;
}
+ if (fieldEntry.size() == 0) {
Review comment:
Thanks, I often forget what the precedent is here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mayya-sharipova merged pull request #580: LUCENE-10351 Correct knn search failure with deleted docs
Posted by GitBox <gi...@apache.org>.
mayya-sharipova merged pull request #580:
URL: https://github.com/apache/lucene/pull/580
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jtibshirani commented on a change in pull request #580: LUCENE-10351 Correct knn search failure with deleted docs
Posted by GitBox <gi...@apache.org>.
jtibshirani commented on a change in pull request #580:
URL: https://github.com/apache/lucene/pull/580#discussion_r777676777
##########
File path: lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90HnswVectorsReader.java
##########
@@ -239,6 +239,9 @@ public TopDocs search(String field, float[] target, int k, Bits acceptDocs) thro
if (fieldEntry == null || fieldEntry.dimension == 0) {
return null;
}
+ if (fieldEntry.size() == 0) {
Review comment:
Would it be simpler to return null here?
##########
File path: lucene/test-framework/src/java/org/apache/lucene/tests/index/BaseKnnVectorsFormatTestCase.java
##########
@@ -559,6 +561,12 @@ public void testDeleteAllVectorDocs() throws Exception {
VectorValues values = getOnlyLeafReader(r).getVectorValues("v");
assertNotNull(values);
assertEquals(0, values.size());
+
+ // assert that knn search doesn't fail on a field with all deleted docs
+ IndexSearcher searcher = newSearcher(r);
Review comment:
Small comment, maybe we can use `LeafReader` directly here. That way this test stays focused on the vectors format instead of `KnnVectorQuery`, which sometimes adds extra logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jpountz commented on a change in pull request #580: LUCENE-10351 Correct knn search failure with deleted docs
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #580:
URL: https://github.com/apache/lucene/pull/580#discussion_r777679243
##########
File path: lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90HnswVectorsReader.java
##########
@@ -239,6 +239,9 @@ public TopDocs search(String field, float[] target, int k, Bits acceptDocs) thro
if (fieldEntry == null || fieldEntry.dimension == 0) {
return null;
}
+ if (fieldEntry.size() == 0) {
Review comment:
In other places, we only return `null` when the field doesn't exist or doesn't have the feature (vectors in this case) enabled. In other cases, we return a non-null value, even when all documents that have a value for the field have been merged away. So I think it would be more consistent to return an empty instance in this case.
(FWIW the above code that checks `if (fieldEntry == null || fieldEntry.dimension == 0)` is dead code since `CodecReader` only forwards the call to the `VectorsReader` if the field exists in `FieldInfos` and has vectors enabled.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mayya-sharipova commented on a change in pull request #580: LUCENE-10351 Correct knn search failure with deleted docs
Posted by GitBox <gi...@apache.org>.
mayya-sharipova commented on a change in pull request #580:
URL: https://github.com/apache/lucene/pull/580#discussion_r778170401
##########
File path: lucene/test-framework/src/java/org/apache/lucene/tests/index/BaseKnnVectorsFormatTestCase.java
##########
@@ -559,6 +561,12 @@ public void testDeleteAllVectorDocs() throws Exception {
VectorValues values = getOnlyLeafReader(r).getVectorValues("v");
assertNotNull(values);
assertEquals(0, values.size());
+
+ // assert that knn search doesn't fail on a field with all deleted docs
+ IndexSearcher searcher = newSearcher(r);
Review comment:
@jtibshirani Thanks for the review. I've adjusted the test in b4dfe087bf4a2f8686c1d90f1f658389b94f3f1f
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mayya-sharipova commented on a change in pull request #580: LUCENE-10351 Correct knn search failure with deleted docs
Posted by GitBox <gi...@apache.org>.
mayya-sharipova commented on a change in pull request #580:
URL: https://github.com/apache/lucene/pull/580#discussion_r778314409
##########
File path: lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90HnswVectorsReader.java
##########
@@ -239,6 +239,9 @@ public TopDocs search(String field, float[] target, int k, Bits acceptDocs) thro
if (fieldEntry == null || fieldEntry.dimension == 0) {
return null;
}
+ if (fieldEntry.size() == 0) {
Review comment:
Thanks Adrien, since the removal of this dead code is already addressed in your PR, I will revert my last commit from this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mayya-sharipova commented on a change in pull request #580: LUCENE-10351 Correct knn search failure with deleted docs
Posted by GitBox <gi...@apache.org>.
mayya-sharipova commented on a change in pull request #580:
URL: https://github.com/apache/lucene/pull/580#discussion_r778185295
##########
File path: lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90HnswVectorsReader.java
##########
@@ -239,6 +239,9 @@ public TopDocs search(String field, float[] target, int k, Bits acceptDocs) thro
if (fieldEntry == null || fieldEntry.dimension == 0) {
return null;
}
+ if (fieldEntry.size() == 0) {
Review comment:
@jpountz Thanks for the review.
> the above code that checks if (fieldEntry == null || fieldEntry.dimension == 0) is dead code
I've removed this dead code in f380e54fea020dd4502d36043d33d09996f72277
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jpountz commented on a change in pull request #580: LUCENE-10351 Correct knn search failure with deleted docs
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #580:
URL: https://github.com/apache/lucene/pull/580#discussion_r778216878
##########
File path: lucene/core/src/java/org/apache/lucene/codecs/lucene90/Lucene90HnswVectorsReader.java
##########
@@ -239,6 +239,9 @@ public TopDocs search(String field, float[] target, int k, Bits acceptDocs) thro
if (fieldEntry == null || fieldEntry.dimension == 0) {
return null;
}
+ if (fieldEntry.size() == 0) {
Review comment:
Thanks Mayya. I opened an issue to clarify these contracts. https://issues.apache.org/jira/browse/LUCENE-10354
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org