Add FloatArray interface to allow easy float[] vector extraction from MemorySegmentVectorFloat and ArrayVectorFloat instances#694
Conversation
|
Before you submit for review:
If you did not complete any of these, then please explain below. |
r-devulap
left a comment
There was a problem hiding this comment.
Curious about the need for introducing a separate FloatArray interface here. Could we instead add an abstract method on VectorFloat<?> itself similar to the array() method introduced in this PR and require implementations like ArrayVectorFloat and MemorySegmentVectorFloat to provide it? That seems like it might keep the API simpler by avoiding an extra type just for array extraction.
If we don't care about compatibility much (since this classes are public and if by chance there are implementations outside the library), than yes - we could certainly do that. It will definitely make the pull request simpler. What we could do is to get rid of But again, if compatibility is not a concern - we could get rid of |
|
I will let @ashkrisk weigh in on it, he is more qualified to answer that :) |
ashkrisk
left a comment
There was a problem hiding this comment.
While there are legitimate, performance related reasons to want direct access to the underlying float[] store, the API as it stands is not designed with that in mind. IMO either of the proposed approaches breaks the encapsulation of the VectorFloat interface in potentially undesirable ways.
At the moment I can only see one place where Opensearch accesses the underlying array. Another solution that might work here would be to add a toArray() method that works on all VectorFloat types and returns a copy of its underlying data.
@reta can you open an issue explaining your use-case in more detail? With more information we can figure out what the best solution is.
@ashkrisk this is exactly the reason why VectorFloat is not changed, there is a new interface that adds this functionality to implementing classes that could be implemented.
See please this pull request https://github.com/opensearch-project/opensearch-jvector/pull/507/changes#diff-ddf73468c0184ab1f6ef7973480de2cbdf98d8eb63d0aec938ec4dd6b5d6e73cR153 |
… MemorySegmentVectorFloat and ArrayVectorFloat instances Signed-off-by: Andriy Redko <drreta@gmail.com>
Add
FloatArrayinterface to allow easyfloat[]vector extraction fromMemorySegmentVectorFloatandArrayVectorFloatinstances. It solves the problem that we have to:VectorFloathasThe suggestion is to introduce new
FloatArrayinterface to have a fast trackfloat[]array conversion forVectorFloats that support it. The change is 100% non-breaking.Closes #696