Fix AbstractStringFieldDataTestCase tests to account for TotalHits lower bound

[dbwiddis]

Signed-off-by: Daniel Widdis [email protected]

Description

From Lucene's IndexSearcher javaDoc:

NOTE: The search(org.apache.lucene.search.Query, int) and searchAfter(org.apache.lucene.search.ScoreDoc, org.apache.lucene.search.Query, int) methods are configured to only count top hits accurately up to 1,000 and may return a lower bound of the hit count if the hit count is greater than or equal to 1,000. On queries that match lots of documents, counting the number of hits may take much longer than computing the top hits so this trade-off allows to get some minimal information about the hit count without slowing down search too much.

The AbstractStringFieldDataTestCase assumed the totalHits value was exact.

This fixes the test to test for either exact accuracy or lower bound, as appropriate.

Issues Resolved

Fixes #4238

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[owaiskazi19]

Can't we just have the below

assertTrue(numDocs >= topDocs.totalHits.value); 

rather the whole condition?

[dbwiddis]

In the test, our total documents (numDocs) is not always over 1000, and so it is more precise to test for equality in the 1/3 of cases where a more accurate result will apply.

OpenSearch/server/src/test/java/org/opensearch/index/fielddata/AbstractStringFieldDataTestCase.java

Line 261 in 05a5819

final int numDocs = scaledRandomIntBetween(10, 3072);

This will protect against a bug where totalHits is always some small constant like 1, which would pass the lower bound test.

[dbwiddis]

(I might argue we could add another assertion that totalHits.value >= 1000 in this case, but that might be overkill.)

[nknize]

Agree it's overkill...

[owaiskazi19]

Pre-commit failing because of the usual Spotless

Execution failed for task ':server:spotlessJavaCheck'.
> The following files had format violations:
      src/test/java/org/opensearch/index/fielddata/AbstractStringFieldDataTestCase.java
          @@ -343,9 +343,9 @@
           ········);
           ········//·As·of·Lucene·9.0.0,·totalHits·may·be·a·lower·bound
           ········if·(topDocs.totalHits.relation·==·TotalHits.Relation.EQUAL_TO)·{
          -············assertEquals(numDocs,·topDocs.totalHits.value);············
          +············assertEquals(numDocs,·topDocs.totalHits.value);
           ········}·else·{
          -············assertTrue(numDocs·>=·topDocs.totalHits.value);························
          +············assertTrue(numDocs·>=·topDocs.totalHits.value);
           ········}
           ········BytesRef·previousValue·=·first·?·null·:·reverse·?·UnicodeUtil.BIG_TERM·:·new·BytesRef();
           ········for·(int·i·=·0;·i·<·topDocs.scoreDocs.length;·++i)·{

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/1949/
• CommitID: 0ec0651

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/1950/
• CommitID: c497919

[nknize]

LGTM! Thx for adding this check @dbwiddis! I had completely forgot about this change.

[heemin32]

Shouldn't we back port this PR to 2.x branch?

[dblock]

I labeled it to backport, let's see.

[dblock]

@dbwiddis Looking at the implementation here: not in love with conditions in specs. When is it one or the other?

[dbwiddis]

@dbwiddis Looking at the implementation here: not in love with conditions in specs. When is it one or the other?

@dblock answering nearly two years later, sorry. The initial PR comment javadoc explains the difference. Lucene can compute the hit count and if it's > 1000 it makes it an approximate bound, indicated by the enum used for the conditional.