thecoop commented on code in PR #14702:
URL: https://github.com/apache/lucene/pull/14702#discussion_r2104201080
##########
lucene/core/src/java/org/apache/lucene/codecs/KnnVectorsReader.java:
##########
@@ -124,7 +124,7 @@ public abstract void search(
*
* <p>The default implementation returns {@code this}
*/
- public KnnVectorsReader getMergeInstance() {
+ public KnnVectorsReader getMergeInstance() throws IOException {
Review Comment:
I think throwing an `IOException` is ok here - the top-level `MergeState`
constructor already throws `IOException`, so no core semantics are being
modified here. And it seems reasonable for a io-related method to throw an
IOException. The alternative is to throw an `UncheckedIOException`, which may
break the caller of `MergeState`. I can modify the other `getMergeInstance`
methods to declare `throws IOException` for consistency here.
For the setter idea, that's a tricky one. The other `getMergeInstance`
methods _do_ create an independent instance, and there are also some
implementations of `KnnVectorsReader.getMergeInstance` which do create a new
instance (albeit to wrap other `getMergeInstance` objects). But we can't do
that for `Lucene99FlatVectorsReader`, due to only having one underlying memory
segment that we have to share between the `IndexInput` objects we use.
Ideally, we would clone - but we can't for this specific implementation. My
hunch is that this is ok as-is, given we have `AssertingKnnVectorsReader`
ensuring the calls happen as expected so that nothing breaks (I've tightened up
the assertions for this). Any other changes in this area and we would need to
re-evaluate though.
--
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
