[GitHub] [lucene] jpountz commented on a change in pull request #616: LUCENE-9573 Add Vectors to TestBackwardsCompatibility

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

jpountz commented on a change in pull request #616:
URL: https://github.com/apache/lucene/pull/616#discussion_r791819311



##########
File path: lucene/backward-codecs/src/test/org/apache/lucene/backward_index/TestBackwardsCompatibility.java
##########
@@ -1278,6 +1294,37 @@ public void searchIndex(Directory dir, String oldName, int minIndexMajorVersion)
         34,
         searcher.getIndexReader());
 
+    // test vector values and KNN search
+    if (minIndexMajorVersion >= KNN_VECTOR_MIN_SUPPORTED_VERSION) {
+      // test vector values
+      int cnt = 0;
+      for (LeafReaderContext ctx : reader.leaves()) {
+        VectorValues values = ctx.reader().getVectorValues(KNN_VECTOR_FIELD);
+        if (values != null) {
+          assertEquals(KNN_VECTOR_FIELD_TYPE.vectorDimension(), values.dimension());
+          for (int doc = values.nextDoc(); doc != NO_MORE_DOCS; doc = values.nextDoc()) {
+            float[] expectedVector = {0.1f, 0.1f, 0.1f + cnt};
+            assertArrayEquals(
+                "vectors do not match for doc=" + cnt, expectedVector, values.vectorValue(), 1e-4f);

Review comment:
       we could set the error to 0 since there's no rounding involved?

##########
File path: lucene/backward-codecs/src/test/org/apache/lucene/backward_index/TestBackwardsCompatibility.java
##########
@@ -1325,22 +1382,44 @@ public void changeIndexWithAdds(Random random, Directory dir, Version nameVersio
 
     reader = DirectoryReader.open(dir);
     searcher = newSearcher(reader);
+    // make sure searching sees right # hits fot term search
     hits = searcher.search(new TermQuery(new Term("content", "aaa")), 1000).scoreDocs;
     assertEquals("wrong number of hits", 44, hits.length);
     d = searcher.doc(hits[0].doc);
     doTestHits(hits, 44, searcher.getIndexReader());
     assertEquals("wrong first document", "0", d.get("id"));
+
+    // make sure searching sees right # hits for KNN search
+    if (nameVersion.major >= KNN_VECTOR_MIN_SUPPORTED_VERSION) {
+      float[] queryVector = {0.1f, 0.1f, 0.1f};
+      hits =
+          searcher.search(new KnnVectorQuery(KNN_VECTOR_FIELD, queryVector, 1000), 1000).scoreDocs;
+      assertEquals("wrong number of hits", 44, hits.length);
+      d = searcher.doc(hits[0].doc);
+      assertEquals("wrong first document", "0", d.get("id"));
+    }
     reader.close();
   }
 
-  public void changeIndexNoAdds(Random random, Directory dir) throws IOException {
-    // make sure searching sees right # hits
+  public void changeIndexNoAdds(Random random, Directory dir, Version nameVersion)
+      throws IOException {
+    // make sure searching sees right # hits fot term search

Review comment:
       There are other instances of this typo.
   
   ```suggestion
       // make sure searching sees right # hits for term search
   ```

##########
File path: lucene/backward-codecs/src/test/org/apache/lucene/backward_index/TestBackwardsCompatibility.java
##########
@@ -1325,22 +1382,44 @@ public void changeIndexWithAdds(Random random, Directory dir, Version nameVersio
 
     reader = DirectoryReader.open(dir);
     searcher = newSearcher(reader);
+    // make sure searching sees right # hits fot term search
     hits = searcher.search(new TermQuery(new Term("content", "aaa")), 1000).scoreDocs;
     assertEquals("wrong number of hits", 44, hits.length);
     d = searcher.doc(hits[0].doc);
     doTestHits(hits, 44, searcher.getIndexReader());
     assertEquals("wrong first document", "0", d.get("id"));
+
+    // make sure searching sees right # hits for KNN search
+    if (nameVersion.major >= KNN_VECTOR_MIN_SUPPORTED_VERSION) {
+      float[] queryVector = {0.1f, 0.1f, 0.1f};
+      hits =
+          searcher.search(new KnnVectorQuery(KNN_VECTOR_FIELD, queryVector, 1000), 1000).scoreDocs;

Review comment:
       I wonder if we should set `k` to a lower value, e.g. 10.
   
   My reasoning is that I could see us fall back to a linear scan at some point when k is greater than the number of docs in the index, so we wouldn't even read the HNSW graph from disk.

##########
File path: lucene/backward-codecs/src/test/org/apache/lucene/backward_index/TestBackwardsCompatibility.java
##########
@@ -1469,6 +1558,10 @@ private void addDoc(IndexWriter writer, int id) throws IOException {
     customType4.setStoreTermVectorOffsets(true);
     customType4.setIndexOptions(IndexOptions.DOCS_AND_FREQS);
     doc.add(new Field("content6", "here is more content with aaa aaa aaa", customType4));
+
+    float[] vector = {0.1f, 0.1f, 0.1f + id};

Review comment:
       nit: maybe vary the numbers a bit more, e.g.
   ```suggestion
       float[] vector = {0.2f, -0.1f, 0.1f + id};
   ```




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