kaivalnp commented on PR #15979: URL: https://github.com/apache/lucene/pull/15979#issuecomment-5113138539
I moved the de-duplicating format to the `sandbox` module, I'm thinking we can promote it to `core` with a `LuceneXX` version when it has been tested enough? (say one or two versions down the line). Being sandboxed allows us to change internals of the format more freely. Also threw Claude Opus 5 at the change for fun, and it found some interesting bugs! 1. The `RandomVectorScorerSupplier#copy` [here](https://github.com/kaivalnp/lucene/blob/4970f93a7dcd7c302cb6f12868a28211bf4ba73e/lucene/core/src/java/org/apache/lucene/codecs/lucene106/dedup/DedupFlatVectorsScorer.java#L114-L119) missed `copy`-ing the `fieldOrdToGroupOrd`, which causes issues in a multi-threaded HNSW merge setup! (the AI confirmed with a sample test) 2. The `VectorScorer#bulk` implementation was [always assuming a dense case](https://github.com/apache/lucene/blob/4970f93a7dcd7c302cb6f12868a28211bf4ba73e/lucene/core/src/java/org/apache/lucene/codecs/lucene106/dedup/DedupUtil.java#L435), which throws errors for exact search fallback with sparse vectors! (the AI confirmed with a sample test) 3. Subtle things like the class [not being exported from `module-info.java`](https://github.com/apache/lucene/blob/4970f93a7dcd7c302cb6f12868a28211bf4ba73e/lucene/core/src/java/module-info.java#L87-L90), so module based consumers would not be able to use the format! IMO the change looks more polished / complete now, would appreciate a final review! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
