jimczi commented on PR #16542:
URL: https://github.com/apache/lucene/pull/16542#issuecomment-5439011549
Thanks for the quick turnaround, the `DenseConjunctionBulkScorer` part looks
right to me.
I pulled the branch and reverted just `DenseConjunctionBulkScorer` + the two
suppliers: 5 of the new tests fail, including `*:*` with one deletion at
4095/99999 and `count()` vs COMPLETE collection at 8192/30000. Good catch on
the `ConstantScoreScorerSupplier` path, I had only looked at the
`BooleanScorerSupplier` one, and that's the one that makes this worth fixing.
Two things before this can go in:
**`testMatchAllWithDeletionsStillPrunesUnderTopScores` is flaky.** It fails
with `-Ptests.seed=14471C5085979DA7`:
```
java.lang.AssertionError: TOP_SCORES should still prune, collected=99999
```
`RandomIndexWriter.getReader()` calls `doRandomForceMerge()`, which randomly
runs `forceMerge(1)` or `forceMergeDeletes()`. When that happens the single
deleted document is expunged, `acceptDocs` is null again, the `collectRange()`
shortcut in `scoreWindow()` collects the whole segment in one call and nothing
gets pruned. Same thing silently removes the deletions coverage in
`testConstantScoreQueriesWithDeletionsUnderComplete`, that one keeps passing
but stops testing what it says it tests. Use a plain `IndexWriter` for those
two (or `setDoRandomForceMerge(false)`) and assert `reader.hasDeletions()`.
**The four-argument `DenseConjunctionBulkScorer` constructor defaulting to
`TOP_SCORES` is a trap.** `ReadAheadMatchAllDocsQuery` in the test tree already
calls it, and it has a `scoreMode` in scope two lines above in `get()`, so it
now prunes under `COMPLETE`. It's test-only today but it's exactly the mistake
the next caller will make. I'd rather pass the score mode explicitly
everywhere, the 45 call sites in `TestDenseConjunctionBulkScorer` are a
mechanical sed.
Then some trimming on `TestExhaustiveScoreModeNoPruning`, 500 lines and 14
tests is a lot for what is really three or four cases:
* `testConstantScoreQueryCompleteVisitsEveryMatchDup` duplicates
`testConstantScoreQueryIsUnaffected`, and the `Dup` suffix looks like a
leftover.
* The `{1000, 5000, 20000, 65536, 100000}` loops don't buy much over one
size comfortably above `WINDOW_SIZE`. The suite is 8.5s today, most of it
indexing.
* Class javadoc lists `TOP_DOCS` as exhaustive, but `isExhaustive()` is
false for it. Also I'd drop the PR number and the "verification harness"
framing, this is a permanent test now.
* The comment about the `ScoreMode.COMPLETE` javadoc claiming "only honored
for TOP_SCORES" is stale, you removed that sentence in this same commit.
* A few `org.apache.lucene.document.NumericDocValuesField` / `IntPoint`
fully qualified inline, worth importing.
The javadoc rewording and the 10.6.0 CHANGES entry look good.
`checkGoogleJavaFormat` is clean and `TestDenseConjunctionBulkScorer`,
`TestSortOptimization`, `TestConstantScoreQuery`, `TestMatchAllDocsQuery`,
`TestBooleanQuery` all pass here.
--
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]
