uschindler commented on PR #13119: URL: https://github.com/apache/lucene/pull/13119#issuecomment-1954160317
> > Hi, as stated in the other issue: I am not really happy to have that enum at all! The similarity/distance functions should be pluggable using `NamedSPILoader`. To implement that, the ordinals must removed in a new file format version and instead names be written using the Codec utility classes. > > Agreed on where we wanna get to. Just trying to get there incrementally, since format changes are quite noisy. > > > As a first step this PR is fine as it does not change file format and just decouples the ordinals from the enum. In future, when we have SPI, we can use the current code of the ordinals > > Exactly, this is just a first step. It (for the most part) encapsulates the translation in the format. When we add a new format and/or evolve VectorSimilarityFunction, this format should be largely immune to the change. > > > In my opinion, the strings as lookup keys are not needed: Just define it as `List<VectorSimilarityFunction>` to get the link between them. At a later stage the backwards layer could then fallback to the list with SPI instances to lookup the legacy ordinals. The coec and the enum are still _enough_ decoupled. > > Yeah, that's probably good enough for now. Updated. The comment is a bit outdated. I was thinking of making it even more vebose by having 2 maps for the lookup... +1 looks fine. -- 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
