Change in behaviour of totalHitsThreshold with #14511

[ChrisHegarty]

The changes in #14511 result in a change in behaviour. This should be analysed since #14511 has been backported in the 10.2.1 bugfix release.

I've not yet fully groked what this doing, but it can be seen in the following test case ( which is a minimal reproducer extracted from an Elasticsearch environment ).

  public void testMinScore() throws Exception {
    try (var dir = newDirectory();
        var writer = new RandomIndexWriter(random(), dir)) {
      for (int i = 0; i < 10; i++) {
        Document doc = new Document();
        doc.add(new StringField("foo", "bar", Store.NO));
        writer.addDocument(doc);
      }
      try (IndexReader reader = writer.getReader()) {
        IndexSearcher searcher = new IndexSearcher(reader);
        BooleanQuery query =
            new BooleanQuery.Builder()
                .add(new TermQuery(new Term("foo", "bar")), Occur.MUST)
                .build();
        var collectorManager = new TopScoreDocCollectorManager(1, 5);
        var topDocs = searcher.search(query, collectorManager);
        assertEquals("Number of matched documents", 10, topDocs.totalHits.value());
      }
    }
  }

Prior to #4511, this test passes, topDocs.totalHits.value() is 10. After the change the test fails with:

java.lang.AssertionError: Number of matched documents expected:<10> but was:<6>
at __randomizedtesting.SeedInfo.seed([72CF7E3B874C10BF:2CC9BF76B3C91C58]:0)
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:647)
at org.apache.lucene.search.TestBooleanScorer.testMinScore(TestBooleanScorer.java:419)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1758)
...

The pertinent changes are in

          @Override
          public List<Impact> getImpacts(int level) {
            if (indexHasFreq == false) {
              return DUMMY_IMPACTS_NO_FREQS;     //  <<<< HERE
            }
            ...
            return DUMMY_IMPACTS;
          }

[ChrisHegarty]

//cc @jpountz @gf2121 @expani

[ChrisHegarty]

Maybe this change in behaviour is expected, but it's a little concerning, unless the test doesn't make sense? Tho, I think that this behaviour has been this way for quite a while, so a change could be surprising!?

[jpountz]

new TopScoreDocCollectorManager(1, 5);

The total hits threshold is 5 here, which means that the collector should count hits at least until it can confirm that there are that many hits that match the query. If there are more than 5 hits, it may return any number in [5, numbef of matches] in combination with TotalHits.Relation.GREATER_THAN_OR_EQUAL_TO.

In the test you shared, the query matches 10 documents, and the collector says that the query matched 6 documents or more, which is correct.

In practice, the better a query is at skipping, the closer the returned hit count will be to the totalHitsThreshold because the query/collector will be able to skip hits very efficiently after having collected the first totalHitsThreshold ones. This is consistent with the change at #4511, which makes queries that don't index frequencies much better at skipping by returning lower (while still correct) score upper bounds.

FWIW we released many similar changes to the returned hit counts in the past for various queries.

For reference, some applications like Elasticsearch made the decision to never return a hit count that is greater than the configured totalHitsThreshold to avoid setting expectations too high with users wrt how many hits would be counted. So this change wouldn't even be noticed by Elasticsearch users (except a good speedup hopefully!)

[ChrisHegarty]

Thanks for the detailed explanation @jpountz. @jimczi explained similar. Closing as not an issue.

[ChrisHegarty]

Elasticsearch test update - elastic/elasticsearch@77dc7f8