shatejas commented on code in PR #13985:
URL: https://github.com/apache/lucene/pull/13985#discussion_r1835999756


##########
lucene/core/src/java/org/apache/lucene/codecs/KnnVectorsReader.java:
##########
@@ -123,4 +123,11 @@ public abstract void search(
   public KnnVectorsReader getMergeInstance() {
     return this;
   }
+
+  /**
+   * Optional: reset or close merge resources used in the reader
+   *
+   * <p>The default implementation is empty
+   */
+  public void finishMerge() throws IOException {}

Review Comment:
   > I wonder if we should reuse the close() method of merge instances for 
this, what do you think @uschindler ?
   
   I went with that solution at first, `close` is called way to late, to be 
able to benefit the search requests going on as per my understanding. I was 
looking for something to switch back to random access earlier and thats how I 
landed with this approach. Happy to move it to `close()` if there is a 
preference
   
   >> Separately, it would be nice to improve the asserting framework 
(AssertingKnnVectorsReader in this case) to validate that finishMerge() is 
always called on merge instances.
   
   Wasn't aware of AssertingKnnVectorsReader. I can take a look



##########
lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99HnswVectorsReader.java:
##########
@@ -113,6 +114,25 @@ public Lucene99HnswVectorsReader(SegmentReadState state, 
FlatVectorsReader flatV
     }
   }
 
+  private Lucene99HnswVectorsReader(
+      Lucene99HnswVectorsReader reader, KnnVectorsReader flatVectorsReader) {
+    assert flatVectorsReader instanceof FlatVectorsReader;

Review Comment:
   >> I would rather make this ctor take a FlatVectorsReader and push the 
responsibility to callers to make the cast
   
   This makes sense to me, I will change it
   
   >> maybe we don't even need a cast if we make getMergeInstance() return a 
FlatVectorsReader
   
   This seems difficult as of now, 
[MergeState](https://github.com/shatejas/lucene/blob/12ca4779b962c96367f3e6a8b06523837e5e6434/lucene/core/src/java/org/apache/lucene/index/MergeState.java#L157)
 expects KNNVectorsReader and FlatVectorsReader is wrapped inside of 
KNNVectorsReader



-- 
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-unsubscr...@lucene.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org

Reply via email to