[DRAFT] Change vector input from IndexInput to RandomAccessInput #13981
Conversation
dungba88
changed the title
| assert values instanceof ByteVectorValues; | ||
| if (!(slice instanceof IndexInput input)) { |
There was a problem hiding this comment.
Nit : input is not used
There was a problem hiding this comment.
It's used in the next block though
input = FilterIndexInput.unwrapOnlyTest(input);
| @@ -77,4 +85,6 @@ default void readBytes(long pos, byte[] bytes, int offset, int length) throws IO | |||
| * @see IndexInput#prefetch | |||
| */ | |||
| default void prefetch(long offset, long length) throws IOException {} | |||
|
|
|||
| Object clone(); | |||
There was a problem hiding this comment.
Change this to RandomAccessInput clone(); so you don't have to cast in all places?
There was a problem hiding this comment.
This baffles me a lot! I can't let the return type to be RandomAccessInput because it will conflict with the DataInput.clone() which returns DataInput.
Adding the clone method seems a bit weird to me, but I'm not sure if there is a better way to go around it.
There was a problem hiding this comment.
I am not sure we should add a clone() here to random access input. This is a wide ranging public interface in lucene. Adding something here should be carefully considered.
There was a problem hiding this comment.
I agree it's not ideal, but clone is needed for copy() method. If not, maybe we need to have a CloneableRandomAccessInput?
Description
This PR changes the input of vector reader from IndexInput to RandomAccessInput, as we mostly random-access the vectors with seek() followed by readFloats().
One thing I'm uncertain is the need for cloning RandomAccessInput.
TODO:
fixes #13938