benwtrent commented on PR #12064: URL: https://github.com/apache/lucene/pull/12064#issuecomment-1372294789
Digging into it more, removing `AbstractVectorValues` will add a fair bit of extra code to the KnnVectorWriters and testing (though testing is a lesser concern I suppose). My thoughts on keeping it are that eventually, we will want to add support to binary vectors (to be used specifically with hamming distance) and half-float (or float16, admittedly, this one may wait until JVM has better float16 support). I am not sure there are other vector encodings we will want to support, but I can see Lucene supporting at least these 4 (including our byte & float32) eventually. There is already a fair bit of duplication. If the prevailing opinion is completely remove `AbstractVectorValues<T>` and make the writers handle individual vector encodings (instead of relying on the underlying BytesRef), I will comply. What say you @rmuir && @jpountz ? -- 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
