jpountz commented on code in PR #12997: URL: https://github.com/apache/lucene/pull/12997#discussion_r1444533812
########## lucene/core/src/java/org/apache/lucene/codecs/lucene99/Lucene99PostingsReader.java: ########## @@ -399,7 +400,9 @@ public PostingsEnum reset(IntBlockTermState termState, int flags) throws IOExcep doc = -1; this.needsFreq = PostingsEnum.featureRequested(flags, PostingsEnum.FREQS); this.isFreqsRead = true; - if (indexHasFreq == false || needsFreq == false) { + // if reuse, we don't consider whether the needsFreq is changed. this is undefined in freq(). + // see also: BasePostingsFormatTestCase#testPostingsEnumFreqs + if ((indexHasFreq == false || needsFreq == false) && reuse == false) { Review Comment: Do we actually need to check `reuse` here? My understanding is that there are 3 cases: 1. Freqs exist and they are needed. In that case, refillDocs() will make sure to fill this array, so we don't need to do anything here. 2. Freqs are not needed. There is nothing to do as calling `PostingsEnum#freq()` is undefined. 3. Freqs are needed but they don't exist. In that case we need to initialize this array with ones here. So it looks to me that we don't need to check the value of `reuse` and could change this condition to `if (indexHasFreq == false && needsFreqs)`? If you're saying this may be a bottleneck, we may also want to replace the for loop with a call to `Arrays#fill`. -- 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