kaivalnp commented on PR #15979: URL: https://github.com/apache/lucene/pull/15979#issuecomment-4988029764
Apologies for the delay, getting back on this PR now! Thanks @msokolov > overwhelming amount of code to review Sorry, I think the AI was too verbose in the code + comments, I re-wrote this PR from scratch as a human, only relying on AI for tests + some documentation + finding errors -- I hope the next iteration is clearer :) > some kind of syntactic sugar/API on top of all this to hide from the user the idea that we are generating multiple fields over the same data? This makes sense to me -- it is ultimately what we're hoping to do with this change! For this first pass I wanted to add the actual de-duplicating format + make no API changes + test performance of the format more widely -- can we open an issue to add this sugar when the change has baked for a while? (this change also needs a corresponding format for quantized vectors) Thanks @mikemccand (and AI) > Does the dedup'ing optimize the `==` case maybe? It does not -- the current entry point for incoming vectors is [here](https://github.com/apache/lucene/blob/99fc6a17929b7b8eae537d70ad7644a44811c150/lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99FlatVectorsWriter.java#L399), which is per-field, and there is no explicit guarantee that all fields for a doc will be indexed in sequence, before moving on to the next doc (perhaps the recently added columnar indexing API exercises this). Moreover, there is an expectation that the same array can be re-used to pass vectors (since the writer has to [own its copy](https://github.com/apache/lucene/blob/99fc6a17929b7b8eae537d70ad7644a44811c150/lucene/core/src/java/org/apache/lucene/codecs/KnnFieldVectorsWriter.java#L39-L45)). For now, I've relied on full equality checks for de-duping. > Can we add 2nd time measure, which is time to wait for the in-flight merges? This was added in https://github.com/mikemccand/luceneutil/pull/587 > I would rather not pay much attention to either merge time in our comparisons here -- merge timing is exceptionally noisy, and has discrete step changes (a force-merge that needs two rounds vs three rounds for example) Makes sense, I'll also report single-threaded indexing numbers from now! The merge path is important to measure for this format because it can involve random IO for de-duplication against a previously written vector, which won't be captured in the indexing path alone. > I think it's CC stating the purpose of each statement? Optimization vs functionally necessary? Yes, there were a few context leaks about my iterations with the AI :) -- 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]
