jimczi commented on PR #16542: URL: https://github.com/apache/lucene/pull/16542#issuecomment-5436026363
Thanks @NextbrickInc, the approach looks good to me, that's what I had in mind in the issue. I checked out the branch locally: the new test does fail without the change in `BatchScoreBulkScorer`, and `TestBooleanScorer`, `TestTermQuery`, `TestCombinedFieldQuery`, `TestTopDocsCollector`, `TestBooleanQuery` all still pass. A few comments: **`DenseConjunctionBulkScorer` has the same hole.** It does `if (scorable.minCompetitiveScore > scorable.score) return NO_MORE_DOCS;` without checking the score mode, and it is reachable with `COMPLETE`: `BooleanScorerSupplier` builds it for a FILTER-only conjunction under `scoreMode != TOP_SCORES` with a constant score of `0f`. With a 5000 docs index and a two clauses FILTER-only `BooleanQuery`, using the same collector as the new test, I get 4096 collected docs instead of 5000 on this branch. A single `setMinCompetitiveScore(nextUp(0f))` kills the second window. That one predates the 10.3 regression (it came with #14293) so I'm fine if you want to keep it out of scope, but if we state that an exhaustive score mode never prunes then we should be consistent. I'd rather fix it here, it's a two lines change. I checked the other `SimpleScorable` users and this is the only remaining one: `BlockMaxConjunctionBulkScorer` and `DisjunctionMaxBulkScorer` are `TOP_SCORES` only, the rest don't look at the min score. **The `ScoreMode.COMPLETE` javadoc doesn't match the code.** It says the API is "only honored for `TOP_SCORES`" but the check is `isExhaustive() == false`, so `TOP_DOCS_WITH_SCORES` honors it too. It also mentions `Scorer#setMinCompetitiveScore` and "callers", while the contract we care about is `Scorable#setMinCompetitiveScore`, which is called by the collector. That javadoc already says "This method may only be called from collectors that use ScoreMode.TOP_SCORES", which is exactly what @hossman was looking for, so I'd just link to it and drop the `(GITHUB#15239)` reference, we don't usually put issue numbers in public javadocs. **CHANGES.txt**: the entry is only in the 11.0.0 section. The regression is in 10.3 so we'll want it in the 10.6.0 section as well when this gets backported. Minor nits: * `newSearcher(reader, false, false, false)` disables the asserting searcher. I ran it with `wrapWithAssertions=true` and it passes, so we can keep the extra coverage. * The reused `Document` in the test is load bearing (doc `i` ends up with `bar0..bari`, which is what makes `bar1` match 999 docs with different norms). Worth a one line comment, it reads like a bug otherwise. * No test for the `CombinedFieldQuery` call site. Nothing blocking, the change itself is correct. Mostly want to agree on the `DenseConjunctionBulkScorer` part before merging. -- 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]
