Skip runs of excluded docs in ReqExclBulkScorer for two-phase clauses

[jimczi]

ReqExclBulkScorer skips runs of excluded docs via docIDRunEnd() only for a plain DocIdSetIterator excluded clause; a two-phase clause advances one doc at a time. This uses TwoPhaseIterator#docIDRunEnd() in the two-phase branch too — as DenseConjunctionBulkScorer already does — clamped to at least upTo + 1 since its default returns the current doc.

Speeds up filters with a two-phase excluded clause that matches most docs, e.g. a doc-values != v query. Covered by the existing randomized TestReqExclBulkScorer#testRandomTwoPhase (its RandomTwoPhaseView overrides docIDRunEnd()).

[romseygeek]

since its default returns the current doc.

Is this something we should fix? I can see it being a source of bugs in future

[dweiss]

Hi @jimczi - this merge causes a test failure that looks like an off-by-one somewhere.
https://jenkins.thetaphi.de/job/Lucene-nightly-main-Linux/1781/console

gradlew :lucene:facet:test --tests "org.apache.lucene.facet.range.TestRangeFacetCounts.testRandomLongsMultiValued" -Ptests.asserts=true -Ptests.file.encoding=US-ASCII -Ptests.gui=true -Ptests.haltonfailure=false -Ptests.jvmargs= -Ptests.jvms=6 -Ptests.multiplier=3 -Ptests.nightly=true -Ptests.seed=4C2167479558CC44 -Ptests.vectorsize=default

This wasn't present (or was masked?) prior to this change.

[dweiss]

LLM says this, sorry I can't offer more help.

The mechanism

  The DV drill‑down goes SortedNumericDocValuesRangeQuery → (no skipper) DocValuesValueRangeIterator, a two‑phase iterator whose matches() must confirm that a value is in range. For
  dense ranges the bulk scorer is DenseConjunctionBulkScorer. In scoreWindow (DenseConjunctionBulkScorer.java:193‑208) it buckets clauses:

  int docIdRunEnd = w.docIDRunEnd();
  if (w.docID() > min || docIdRunEnd < minRunEndThreshold) {
    windowClauses.add(w);          // needs per-doc matches() confirmation
  } else {
    minDocIDRunEnd = Math.min(minDocIDRunEnd, docIdRunEnd);   // treated as FULLY MATCHING
  }
  ...
  if (acceptDocs == null && windowClauses.isEmpty()) {
    collector.collectRange(min, minDocIDRunEnd);   // collect WITHOUT calling matches()

  For a width‑1 window (max == min+1, i.e. a partition boundary), minRunEndThreshold = min(min+2048, max) = min+1. The clause sits at w.docID() == min, so:
  - Before #16307: docIDRunEnd() == min < min+1 → goes to windowClauses → matches() is confirmed. Correct.
  - After #16307: docIDRunEnd() == min+1 >= min+1 → goes to the "fully matching" bucket → collectRange(min, min+1) collects the boundary doc without confirming matches().

  That boundary doc has the field (so the approximation is positioned on it) but its value may be out of range — it gets counted anyway → the ±1. The default docIDRunEnd()
  optimistically assumes the current (unconfirmed) doc matches, which is fine for ReqExclBulkScorer (which calls it only after matches()), but DenseConjunctionBulkScorer consults it
  before confirmation.

[dweiss]

The LLM came up with a single-line fix and a bloated test I don't understand.
fix.patch

[romseygeek]

Is this something we should fix? I can see it being a source of bugs in future

Anyone who wants to know next week's lottery numbers just email me now.