NextbrickInc commented on PR #16542:
URL: https://github.com/apache/lucene/pull/16542#issuecomment-5446188986
Thanks for putting these inline, that made them easy to work through. All
eight points taken, and all of them were right. Pushed as `8161b4f`, plus a
merge of `main` to clear a `CHANGES.txt` conflict that had appeared since the
last push.
**Summary of the changes**
- The four-argument `DenseConjunctionBulkScorer` constructor is gone;
`ScoreMode` is explicit at all 45 call sites, including
`ReadAheadMatchAllDocsQuery`, which now passes the `scoreMode` it already held.
- Both deletion tests use a plain `IndexWriter` and assert
`reader.hasDeletions()`.
- `testConstantScoreQueryCompleteVisitsEveryMatchDup` removed.
- All size loops collapsed to a single `NUM_DOCS = 20_000`, and the MatchAll
/ FieldExists / point-range cases merged onto one index.
- Class javadoc corrected (`TOP_DOCS` is not exhaustive), PR number and
"verification harness" framing dropped, stale `ScoreMode.COMPLETE` comment
reworded, `IntPoint` / `NumericDocValuesField` imported.
14 tests to 10, suite 8.5s to 1.6s.
To confirm the trimming did not weaken anything: with
`applyMinCompetitiveScore` forced true, the reduced suite still fails in
exactly the five places you identified, and
`testConstantScoreQueriesWithDeletionsUnderComplete` now fails 30/30 iterations
at 4095/19999 rather than depending on the merge coin flip.
**One thing I should have caught before you saw this.**
Beasting turned up a flake that this PR introduces into
`TestMultiCollector`. I had changed `newSearcher(reader, false, false, false)`
to `(true, true, true)` in the previous commit. The third argument is
`wrapWithAssertions`; `AssertingWeight` sets `canSetMinCompetitiveScore =
scoreMode == ScoreMode.TOP_SCORES && topLevelScoringClause`, and
`AssertingScorer` asserts on it. So whenever the random draw produced an
`AssertingIndexSearcher`, the test's deliberately contract-violating collector
tripped the assert:
```
java.lang.AssertionError
at
org.apache.lucene.tests.search.AssertingScorer.setMinCompetitiveScore(AssertingScorer.java:91)
at
org.apache.lucene.search.TopScoreDocCollector$1.updateGlobalMinCompetitiveScore(TopScoreDocCollector.java:147)
at
org.apache.lucene.search.TestMultiCollector$1.setScorer(TestMultiCollector.java:85)
```
22 failures in 500 iterations, seed `44073997F88AC6EA`. That `false, false,
false` was load bearing and I should not have flipped it. My own new test had
the same problem, 2 in 20.
Both now use `newSearcher(reader, true, false, true)`, which keeps the
reader wrapping and intra-segment concurrency but drops the assertion wrapping.
0 failures in 500 and 400 iterations respectively, and clean under
`-Ptests.nightly=true`.
That does raise a question I would rather you decide than decide myself:
`AssertingScorer` currently encodes "only `TOP_SCORES` may call
`setMinCompetitiveScore`", while this PR's premise is that a nested collector
may call it under an exhaustive score mode and it must be harmless. I have left
the assertion untouched, since relaxing it is a contract change that belongs in
its own issue rather than inside this one. Happy to open that issue if you
agree it is worth doing.
**Verification**
| Check | Result |
| --- | --- |
| `:lucene:core:test` on `-Ptests.seed=14471C5085979DA7` | 8696 tests, pass |
| `:lucene:core:test` full module, random seed | 8696 tests, pass |
| `:lucene:sandbox:test` | 339 tests, pass |
| `TestExhaustiveScoreModeNoPruning -Ptests.iters=40` | 400 cases, 0
failures |
| `TestMultiCollector -Ptests.iters=500` | 0 failures (22 before the fix) |
| Both suites, `-Ptests.iters=15 -Ptests.nightly=true` | 0 failures |
| Negative control, guard disabled | 5 tests fail, matching your count |
| `check -x test` (checkGoogleJavaFormat, ecjLint, forbiddenApis) | clean |
| `renderJavadoc -Pvalidation.errorprone=true` | clean |
Toolchain: Temurin JDK 25.0.4.1, Gradle 9.7.0. The branch is mergeable again
after the `main` merge.
Could you take another look when you have a moment? If it looks good to you
now I would appreciate an approval and a merge. And if you would still rather
see the test file tighter, or want the `AssertingScorer` contract settled
first, say so and I will turn it around quickly.
--
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]
