NextbrickInc commented on PR #16542:
URL: https://github.com/apache/lucene/pull/16542#issuecomment-5437158365
@jimczi
Thanks for the thorough review — I went through every point locally and they
all hold up.
Full write-up with the runs attached; the short version:
**`DenseConjunctionBulkScorer` — confirmed, and it's worse than we thought.**
Your repro lands exactly: 5000 docs, two FILTER clauses, the collector from
the new test →
4096 collected instead of 5000.
But it isn't only `BooleanScorerSupplier`.
`ConstantScoreScorerSupplier#bulkScorer()` also
builds a `DenseConjunctionBulkScorer`, and it doesn't consult the score mode
at all. That
supplier backs `MatchAllDocsQuery`, `FieldExistsQuery` and point ranges,
whose constant score
is the boost rather than `0f` — and `nextUp(1.0f) > 1.0f`, so the same
window abort fires.
Those queries look fine today only because `scoreWindow()` takes the
`collectRange(min, minDocIDRunEnd)` shortcut when `acceptDocs == null` and
every clause matches
the whole run: the segment is collected in one call, so the check at the top
of the loop only
runs once. Delete a single document and `acceptDocs` becomes non-null, the
shortcut is skipped,
and it goes window by window. On a 100k-doc segment with one deletion,
`COMPLETE`:
| query | main | this PR | + Dense fix | expected |
|---|---|---|---|---|
| `*:*` | 4,095 | 4,095 | 99,999 | 99,999 |
| `FieldExistsQuery[n]` | 4,095 | 4,095 | 99,999 | 99,999 |
| `p:[0 TO 100000]` | 4,095 | 4,095 | 99,999 | 99,999 |
| `#a:x #b:y` | 4,095 | 4,095 | 99,999 | 99,999 |
| `a:x` | 2 | 99,999 | 99,999 | 99,999 |
| `a:x` (COMPLETE_NO_SCORES) | 4,095 | 4,095 | 99,999 | 99,999 |
So I'd rather not leave it out of scope — `*:*` is the most common query
there is, and shipping
"an exhaustive score mode never prunes" while half the exhaustive paths
still do would be worse
than the status quo. It's the same two-line shape as the
`BatchScoreBulkScorer` change:
private final SimpleScorable scorable;
+ private final boolean applyMinCompetitiveScore;
+ // Exhaustive collection must visit every match even if a nested
collector calls
+ // setMinCompetitiveScore (GITHUB#15239).
+ this.applyMinCompetitiveScore = scoreMode.isExhaustive() == false;
while (min < max) {
- if (scorable.minCompetitiveScore > scorable.score) {
+ if (applyMinCompetitiveScore && scorable.minCompetitiveScore >
scorable.score) {
plus threading `scoreMode` through `BooleanScorerSupplier:364`, `:437` and
`ConstantScoreScorerSupplier:92`. Keeping the existing 4-arg constructor as
a delegate that
passes `TOP_SCORES` leaves the 45 call sites in
`TestDenseConjunctionBulkScorer` untouched.
I also audited the rest: `BlockMaxConjunctionBulkScorer`,
`MaxScoreBulkScorer` and
`DisjunctionMaxBulkScorer` are `TOP_SCORES`-only by construction, and
`BooleanScorer` /
`SortRescorer` hold a `SimpleScorable` but never read `minCompetitiveScore`.
So this is the
last one.
**Javadoc** — agreed, and it's measurable rather than theoretical: the same
`TermQuery` that
collects all 1000 matches under `COMPLETE` collects 2 under
`TOP_DOCS_WITH_SCORES`. I'll link
`Scorable#setMinCompetitiveScore` and drop the `(GITHUB#15239)` reference.
**CHANGES.txt** — agreed, 10.6.0 too. Worth noting the
`DenseConjunctionBulkScorer` check
arrived earlier, in 10.2.0 (#14293), so that half is affected from 10.2.0
onward.
**Nits** — all three taken. `newSearcher(reader, true, true, true)` passes,
so I'll keep the
asserting searcher. Comment added for the reused `Document` (doc `i`
accumulates `bar0..bari`,
which is what makes `bar1` match 999 docs with distinct norms). And there's
now a
`CombinedFieldQuery` test — it fails on `main` and passes here, at
1k/20k/65k docs.
**Validation:** new suite `TestExhaustiveScoreModeNoPruning` (14 tests,
1k–100k docs) — 14/14
pass with the fix, 5 fail without. `:lucene:core:test` 8694 tests × 4 seeds,
all green.
Downstream modules (queries, sandbox, facet, join, grouping, suggest, misc,
queryparser,
highlighter, monitor, memory, classification, expressions, spatial-*,
backward-codecs,
analysis:common) — 7227 tests, all green. `checkGoogleJavaFormat` clean.
`TOP_SCORES` latency
on a 1M-doc segment is within ±1.9% (best-of-20 × 3), so the pruning path is
untouched.
Lucene_PR16542_Technical_Review_NextBricks_Shrey_Narayan.pdf
[Lucene_PR16542_Technical_Review_NextBricks_Shrey_Narayan.pdf](https://github.com/user-attachments/files/31508738/Lucene_PR16542_Technical_Review_NextBricks_Shrey_Narayan.pdf)
--
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]
