[GitHub] [lucene-solr] mikemccand commented on a change in pull request #2239: LUCENE-9695: don't merge deleted vectors

[GitBox <gi...@apache.org>]

mikemccand commented on a change in pull request #2239:
URL: https://github.com/apache/lucene-solr/pull/2239#discussion_r563217009



##########
File path: lucene/core/src/test/org/apache/lucene/index/TestVectorValues.java
##########
@@ -748,29 +751,107 @@ public void testRandom() throws Exception {
             assertEquals(dimension, v.length);
             String idString = ctx.reader().document(docId).getField("id").stringValue();
             int id = Integer.parseInt(idString);
-            assertArrayEquals(idString, values[id], v, 0);
-            ++valueCount;
+            if (ctx.reader().getLiveDocs() == null || ctx.reader().getLiveDocs().get(docId)) {
+              assertArrayEquals(idString, values[id], v, 0);
+              ++valueCount;
+            } else {
+              assertNull(values[id]);
+            }
           }
         }
         assertEquals(numValues, valueCount);
-        assertEquals(numValues, totalSize);
+        assertEquals(numValues, totalSize - numDeletes);
+      }
+    }
+  }
+
+  /**
+   * Index random vectors, sometimes skipping documents, sometimes deleting a document, sometimes
+   * merging, sometimes sorting the index, and verify that the expected values can be read back
+   * consistently.
+   */
+  public void testRandom2() throws Exception {

Review comment:
       Yay!

##########
File path: lucene/core/src/test/org/apache/lucene/index/TestKnnGraph.java
##########
@@ -124,21 +126,84 @@ public void testMerge() throws Exception {
     }
   }
 
-  private void dumpGraph(KnnGraphValues values, int size) throws IOException {
+  /**
+   * Verify that we get the *same* graph by indexing one segment as we do by indexing two segments
+   * and merging.
+   */
+  public void testMergeProducesSameGraph() throws Exception {

Review comment:
       Nice test :)

##########
File path: lucene/core/src/java/org/apache/lucene/codecs/VectorWriter.java
##########
@@ -194,6 +197,7 @@ public int nextDoc() throws IOException {
       current = docIdMerger.next();
       if (current == null) {
         docId = NO_MORE_DOCS;
+        size = ord;

Review comment:
       Maybe also add a comment here so we understand the sneakiness in context?




----------------------------------------------------------------
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.

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