msokolov commented on PR #13872: URL: https://github.com/apache/lucene/pull/13872#issuecomment-2464749787
OK; re allocating vectors in supplier, yes that can't work because of aliasing across threads. I also found we do not adequately test this case. There are *many* things in here that are not tested; I think we need to add randomization in RandomCodec as we have for other codecs (postings, docvalues) to get better coverage, and we should also explicitly be testing many of the codec options we have added (multithreaded merges, different forms of quantization). Maybe we have coverage for some of these things, but it looks to me as if our format-specific tests just choose default options. > My understanding is that conflating access and cloning/copying into a single method means that we cannot achieve both the above use cases in an optimal way. I am coming to a similar conclusion. Although I do think this API change can be made to work if we cache the scorers in the suppliers and return them when done (I was thinking of doing this by making them closeable), this does add a lot of extra bookkeeping and I'm not sure it's going to end up being better in the end. As a side note, I see that IndexInput is Closeable -- but I don't think we ever close() the ones we allocate? Is this a problem? -- 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
