Re: [PR] Unify how missing field entries are handle in knn formats [lucene]

["jpountz (via GitHub)" <gi...@apache.org>]

jpountz commented on code in PR #13641:
URL: https://github.com/apache/lucene/pull/13641#discussion_r1753859763


##########
lucene/test-framework/src/java/org/apache/lucene/tests/index/BaseKnnVectorsFormatTestCase.java:
##########
@@ -230,6 +236,120 @@ public void testIllegalDimChangeTwoWriters() throws Exception {
     }
   }
 
+  @SuppressWarnings("unused")
+  public void testMergingWithDifferentKnnFields() throws Exception {
+    try (var dir = newDirectory()) {
+      IndexWriterConfig iwc = new IndexWriterConfig();
+      Codec codec = getCodec();
+      if (codec.knnVectorsFormat() instanceof PerFieldKnnVectorsFormat perFieldKnnVectorsFormat) {
+        final KnnVectorsFormat format =
+            perFieldKnnVectorsFormat.getKnnVectorsFormatForField("field");
+        iwc.setCodec(
+            new FilterCodec(codec.getName(), codec) {
+              @Override
+              public KnnVectorsFormat knnVectorsFormat() {
+                return format;
+              }
+            });
+      }
+      TestMergeScheduler mergeScheduler = new TestMergeScheduler();
+      iwc.setMergeScheduler(mergeScheduler);
+      iwc.setMergePolicy(new ForceMergePolicy(iwc.getMergePolicy()));
+      try (var writer = new IndexWriter(dir, iwc)) {
+        for (int i = 0; i < 10; i++) {
+          var doc = new Document();
+          doc.add(new KnnFloatVectorField("field", new float[] {i, i + 1, i + 2, i + 3}));
+          writer.addDocument(doc);
+        }
+        writer.commit();
+        for (int i = 0; i < 10; i++) {
+          var doc = new Document();
+          doc.add(new KnnFloatVectorField("otherVector", new float[] {i, i, i, i}));
+          writer.addDocument(doc);
+        }
+        writer.commit();
+        try {
+          writer.forceMerge(1);
+        } catch (IllegalStateException e) {
+          // do nothing, we expect this exception
+        }
+        assertNotNull(mergeScheduler.ex.get());
+        Exception ex = mergeScheduler.ex.get();
+        assertTrue(ex instanceof IllegalArgumentException);
+      }
+    }
+  }
+
+  @SuppressWarnings("unused")
+  public void testMergingWithDifferentByteKnnFields() throws Exception {
+    try (var dir = newDirectory()) {
+      IndexWriterConfig iwc = new IndexWriterConfig();
+      Codec codec = getCodec();
+      if (codec.knnVectorsFormat() instanceof PerFieldKnnVectorsFormat perFieldKnnVectorsFormat) {
+        final KnnVectorsFormat format =
+            perFieldKnnVectorsFormat.getKnnVectorsFormatForField("field");
+        iwc.setCodec(
+            new FilterCodec(codec.getName(), codec) {
+              @Override
+              public KnnVectorsFormat knnVectorsFormat() {
+                return format;
+              }
+            });

Review Comment:
   Does it actually work? I would expect the read logic to look up the codec by name and to get the original codec back rather than your FilterCodec.



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