DiversifiedTopDocsCollector counts a document without a key as key 0, so a real key-0 hit is dropped

DiversifiedTopDocsCollector (lucene/misc, @lucene.experimental) returns the top hits "ensuring no more than maxHitsPerKey results from a common source"; a subclass supplies each document's key through getKeys(), and DiversifiedTopDocsCollectorManager (10.5.0) re-applies the cap when it merges slices. When the key source has no value for a document (advanceExact returns false), insert() sets the key to 0 (DiversifiedTopDocsCollector.java:146-151), and the manager's reduce counts by that key (DiversifiedTopDocsCollectorManager.java:77-89). A keyless document therefore shares one quota with the real key-0 documents, and the lower-ranked of them is dropped although the cap does not require it. With ordinal keys, the pattern of Lucene's own test, key 0 is the first value in sort order: 3 songs (no artist, ABBA, Zappa) with numHits 3 and maxHitsPerKey 1 return [no-artist, zappa]. No error is raised and totalHits stays correct. The else value = 0 branch came with LUCENE-7407 (7.0.0), which also silently changed the test's ordinal pattern from "missing is its own group" to "missing is the first artist"; with a plain numeric key source the collision is as old as the class (5.1.0). Giving keyless documents their own group in the collector and in the manager fixes it; documenting the behaviour is the alternative.

• Diversified collector: DiversifiedTopDocsCollector (lucene/misc, @lucene.experimental, abstract) "controls diversity in results by ensuring no more than maxHitsPerKey results from a common source are collected" (DiversifiedTopDocsCollector.java:37-39); its javadoc example is "the top 20 hit records of that year - with a max of 3 Beatles hits" (:52-53).
• Keys: the subclass implements getKeys(LeafReaderContext), "a source of values used for grouping keys" (:115-116), as a NumericDocValues. For string keys, upstream's test returns the term ordinal of a SortedDocValues field (TestDiversifiedTopDocsCollector.java:167-204); ordinal 0 is the first value in sort order.
• No value: advanceExact(doc) returns "whether target has a value" (DocValuesIterator.java:24-33), and longValue() "is illegal to call ... after advanceExact(int) returned false" (NumericDocValues.java:32-37).
• Manager: DiversifiedTopDocsCollectorManager (#16216, 10.5.0) collects per slice and merges, "re-applying the diversity constraint globally" (DiversifiedTopDocsCollectorManager.java:53-58).

1. Key lookup (DiversifiedTopDocsCollector.java:144-151): if (keys.advanceExact(leafDocID)) value = keys.longValue(); else value = 0; then addition.key = value.
2. Per-key queue (:153-171): the queue is looked up by that key, so a keyless document and the real key-0 documents fill one queue of size maxHitsPerKey. When it is full, the lower-ranked document is rejected (:166-171) or evicted from the global queue (:179-186).
3. Manager merge (DiversifiedTopDocsCollectorManager.java:77-89): reduce keeps at most maxHitsPerKey hits per ScoreDocKey.key; a keyless hit carries key 0, so the merge conflates them again even when the two documents were collected in different slices (executed: 2 segments, 2 slices, 1 of 2 hits).
4. Who is dropped: the lower-ranked of the two (score, then the higher doc id). In the repro all scores tie and the keyless document is doc 0, so the real key-0 document goes; with the scores reversed the keyless one goes. Two keyless documents also share one quota, and the public ScoreDocKey.getKey() reports 0L for a keyless hit.

totalHits is always correct. A bounded-exhaustive comparison of the collector and the manager (every slicing in which each segment is whole or cut once into two intra-segment partitions) with an independent reference found no other deviation within its bounds: 0 mismatches in 974,644,128 exhaustive checks (up to 5 documents, 1-3 segments, keys {0, 1, 2, missing}) and 48,000,000 random ones when the reference also counts a missing key as key 0; with "missing is its own group" the smallest mismatch has 2 documents. This is bounded evidence, not a proof.

• Who is exposed. Users of DiversifiedTopDocsCollector (opt-in, @lucene.experimental, lucene/misc, abstract: it must be subclassed) or DiversifiedTopDocsCollectorManager (10.5.0) whose key field is optional, such as products without a retailer or songs without an artist. The class's own examples are retailers and artists (DiversifiedTopDocsCollector.java:41-53); its javadoc does not discuss missing values.
• Ordinal keys always collide. Ordinal 0 is the first value in sort order, so any keyless document takes the quota of the alphabetically first source. Numeric keys collide only when 0 is a real id.
• No scale, no special setup. 2 documents and a plain new IndexWriterConfig(). The test framework randomizes the codec, directory and searcher (the recorded runs got CheapBastard and Asserting(Lucene104)); every recorded run, with 4 seeds and assertions on and off, gave the same result.
• Silent. No exception and a correct totalHits; one source just gets fewer slots than the cap allows.
• How likely. Rare: keyless matching documents must compete with a real key-0 document for the top hits. No report was found (see prior reports).

# Lucene main 645d4fe14a20, assertions on (-ea), seed DEADBEEF: 1 test(s) run, 1 failed
    => java.lang.AssertionError: hits expected:<2> but was:<1>
       org.junit.Assert.fail(Assert.java:89)
       org.junit.Assert.failNotEquals(Assert.java:835)
       org.junit.Assert.assertEquals(Assert.java:647)
       org.apache.lucene.misc.search.TestDiversifiedMissingKey.testDocWithoutKeyDoesNotUseKeyZeroQuota(TestDiversifiedMissingKey.java:66)

• No policy anywhere. The class promises only an upper bound (DiversifiedTopDocsCollector.java:37-39), which holds. CHANGES says "collecting no more than a given number of results under a choice of key" (LUCENE-6066, lucene/CHANGES.txt:10929-10931) and "enable concurrent search" (#16216, :1015-1017). Upstream's test adds one reused artist field to every document (TestDiversifiedTopDocsCollector.java:435-459).
• The key is invented. getKeys() is "a source of values used for grouping keys" (:115), but for a keyless document the source cannot report a value (NumericDocValues.java:32-37); the 0 comes from the collector.
• A silent change in 7.0. LUCENE-7407 (f7aa200d406) ported addition.key = keys.get(addition.doc - docBase); to the iterator API with an explicit else value = 0, and in the same commit ported the test's ordinal adapter from return sdv.getOrd(context.docBase + docID); ("missing values are indicated by -1") to an iterator whose longValue() returns ordValue(). The adapter's advanceExact came with LUCENE-7462 (9aca4c9d560) and the collector's with 99af830223a, all in 7.0.0. Executed: on 5.1.0 and 6.6.6 that adapter gives the keyless song key -1 and all 3 hits; from 7.0.0 on, key 0 and ABBA is lost. None of LUCENE-7407's 42 comments mentions this collector.
• Upstream's two adapters disagree. The test picks randomly between "Slower key lookup but 100% accurate" (ordinals) and "Faster key lookup but with potential for collisions" (hashes) (TestDiversifiedTopDocsCollector.java:380-385). With a song without an artist plus an ABBA song, the accurate one returns 1 hit, the hashed one 2 (executed).
• Grouping keeps them apart by default. The class javadoc compares itself with GroupingSearch (:50-51). There a document without a value gets a null group of its own (TermGroupSelector.java:70-74, :91-96; FirstPassGroupingCollector.java:220-234), and excluding such documents is an option: "Whether to ignore documents that don't have the group field instead of putting them in a null group" (GroupingSearch.java:393-394).

The strongest "intended" case (rated strong by a cross-model review, moderate by the other and by the devil's advocate): the literal bound holds; getKeys() is a subclass hook, so the subclass owns the policy; 0 is what a missing number read as for years (before 7.0 the standard doc-values implementations returned 0 from get(), executed on 5.1.0 and 6.6.6, although the 6.6.6 javadoc does not promise it; numeric sorts default a missing value to 0, LongComparator.java:37); the class is experimental; a dense key source that reports a sentinel such as Long.MIN_VALUE for keyless documents avoids it (both reviews' control tests pass); the manager asserts sdk.key != null (DiversifiedTopDocsCollectorManager.java:80); and a change alters results that have been stable for years.

Why it falls short: it shows the behaviour is unspecified rather than chosen. Nothing documents the 0 or makes it configurable, unlike sort missing values; since 7.0 advanceExact == false is the only way a sparse source can say "no key", and the collector overrides it; upstream's own "100% accurate" pattern flipped behaviour in a mechanical port; and the grouping module keeps such documents apart by default. The workaround needs knowledge nothing documents. The rebuttal is clearest for ordinal keys; for plain numeric keys, where 0 has been the result since 5.1.0, the intended case is stronger (a fact-check reviewer rated it strong there and weak for ordinal keys). Verdict: a defect, low tier (adjudication: confirmed, medium); a javadoc-only answer stays a legitimate maintainer choice, in particular for numeric keys.

Both classes need the change: the manager's merge conflates a keyless and a key-0 hit across slices on its own. A partial fix fails (read, not run): with only the collector changed, the unchanged merge hits assert sdk.key != null with assertions on and unboxes a null key (a NullPointerException) with them off; with only the manager changed, the collector has already stored 0. The alternatives are equally reasonable maintainer choices: leave keyless documents uncapped, or document today's behaviour together with the dense-sentinel workaround, which avoids any behaviour change. Until a fix ships, applications can wrap their key source so that every document reports a key, with a sentinel that no real key uses, such as Long.MIN_VALUE, for documents without one.

In DiversifiedTopDocsCollector.insert, set ScoreDocKey.key to null when advanceExact is false and keep those hits in a dedicated missingKeyQueue (used where perKeyQueues is read, written and cleared, also in perKeyGroupRemove); in DiversifiedTopDocsCollectorManager.reduce, count them in a separate missingKeyCount (the assert sdk.key != null goes). +24/-16 in two files.

     int leafDocID = addition.doc - docBase;
-    long value;
-    if (keys.advanceExact(leafDocID)) {
-      value = keys.longValue();
-    } else {
-      value = 0;
-    }
-    addition.key = value;
+    addition.key = keys.advanceExact(leafDocID) ? keys.longValue() : null;
 
-    // For this to work the choice of key class needs to implement
-    // hashcode and equals.
-    PriorityQueue<ScoreDocKey> thisKeyQ = perKeyQueues.get(addition.key);
+    PriorityQueue<ScoreDocKey> thisKeyQ =
+        addition.key == null ? missingKeyQueue : perKeyQueues.get(addition.key);

Trade-offs

• Behaviour change for code that relied on "missing = key 0", in particular numeric key sources, where it has been the behaviour since 5.1.0: worth a CHANGES entry.
• ScoreDocKey.getKey() is public and returns Long; for a keyless hit it returns null instead of 0L, so a caller that unboxes it would throw. Mention it in the CHANGES/MIGRATE note, or keep getKey() at 0 with a separate flag.
• Subclass-visible changes beyond getKey(): the protected insert(...) can return a ScoreDocKey whose key is null, the protected maxNumPerKey also sizes the missing-key queue, and ScoreDocKey.toString() prints key:null. No test covers these directly.
• Cost: competitive keyless hits get their own bounded queue (recycled like the others) instead of sharing key 0's, and insert, perKeyGroupRemove and reduce gain a null check. Not measured.
• Keyless documents are still capped together (maxHitsPerKey of them in total), a policy choice.

What the checks validated

• Gradle, scratch worktree of a9779ef3c193, JDK 25: stock + the new test fails 2 of 4 with assertions on and off; with the fix the new test and TestDiversifiedTopDocsCollector (2 suites, 8 tests) pass with assertions on, off and with nightly tests; the whole lucene/misc module, 24 suites, 319 tests (9 skipped), 0 failures with assertions on and off; :lucene:misc:check -x test (ecjLint, forbiddenApis, checkGoogleJavaFormat) passes.
• Standalone runner: the new test 4 of 4 with -ea and -da.
• After the test's comment-only javadoc fix, in a fresh scratch worktree: checkGoogleJavaFormat passes, the 2 suites pass with the fix with assertions on and off, :lucene:misc:check -x test passes, and the standalone runner gives 4 of 4.
• Earlier: the original repro 2 of 2, upstream TestDiversifiedTopDocsCollector 4 of 4, the cross-model reviewers' expected-behaviour suites, and the bounded-exhaustive harness against the own-group reference, 0 mismatches in 24,515,232 checks with assertions on.

Skip the per-key queue when advanceExact is false (and skip perKeyGroupRemove for such hits); in reduce, always keep them.

Trade-offs

Treats a document without a source as from no source at all, but keyless documents can then fill every slot, which defeats the diversity the class exists for when many documents lack the field. Same getKey() and CHANGES considerations as the recommended option.

Add to the class javadoc and getKeys(): a document for which the key source has no value is grouped under key 0; to keep such documents apart, return a key source whose advanceExact is always true and whose longValue() returns a sentinel such as Long.MIN_VALUE for them. A variant: a protected hook or constructor argument for the missing key, defaulting to 0.

@Override
public boolean advanceExact(int target) throws IOException {
  doc = target;
  has = in.advanceExact(target);
  return true; // every document has a key
}

@Override
public long longValue() throws IOException {
  return has ? in.longValue() : Long.MIN_VALUE; // keyless: own key
}

Trade-offs

No behaviour change, the safest for compatibility; but the ordinal trap stays (a keyless document silently takes the first value's quota) and every user must know to wrap the source. The workaround itself was executed: 2 of 2 hits on main. Its caveats: the sentinel must be a value no real key uses; the snippet shows only the two methods the collector calls, a real wrapper also implements docID(), nextDoc(), advance() and cost() and is not a general dense NumericDocValues (the executed one throws from nextDoc and advance); each lookup gains a branch.

Method: code reading, then bounded-exhaustive translation validation on the stock main jars.

• The hunt plan read the lead on main: keys.advanceExact(leafDocID) false gives value = 0, so a document with no key shares the per-key cap with documents whose key really is 0 (rated "likely Low, and possibly intended").
• The hunt then built an independent reference (sort by score descending, then doc id; at most maxHitsPerKey per key; the first numHits; totalHits = all matches) and compared it with the real collector and with the manager through IndexSearcher under every slicing in which each segment is whole or cut once into two intra-segment partitions: every index of up to 5 documents in 1-3 segments with scores {1, 2, 3}, keys {0, 1, 2, missing}, numHits 1-4 and maxHitsPerKey 1-3 (974,644,128 checks with assertions off, 22,115,232 with them on for up to 4 documents) plus 48,000,000 random checks with up to 8 documents.
• With the reference's missing-key policy set to "counts as key 0" there were 0 mismatches; with "its own group" the smallest mismatch has 2 documents. A mutant reference was told apart, and two implementation mutants (a <= cap in the manager, remove() without downHeap) were killed, so the check is not vacuous.
• An AI finder then reduced it to a public-API JUnit, and independent AI tools reproduced, reviewed and judged it (see the checks).