Fix SearchQueryIT to not depend on percentage

[andrross]

The CommonTermsQuery tests in SearchQueryIT had one case that depended on the default cutoff frequency percentage. A recent change for concurrent search increased the number of deleted documents that could be indexed in the "indexRandom" helper method, and it turns out the cutoff percentage is calculated from a max docs value that includes deleted docs. This change replaces the default percentage with an absolute value (number of documents that must match) so it is no longer susceptible to behavior change due to number of deleted documents.

Related Issues

Resolves #11208

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
• Commits are signed per the DCO using --signoff
• Commit changes are listed out in CHANGELOG.md file (See: Changelog)
• Public documentation issue/PR created

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.

[github-actions]

Compatibility status:

Checks if related components are compatible with change 1d6543b

Incompatible components

Incompatible components: [https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/performance-analyzer.git]

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/performance-analyzer-rca.git]

[reta]

Thanks a lot @andrross !

[jed326]

Thanks for looking into this! Is the behavior of the cutoff percentage accounting for deleted docs correct or should we also track fixing that underlying issue? It's not clear to me why that would make this test flaky instead of always failing.

[msfroh]

Thanks for looking into this! Is the behavior of the cutoff percentage accounting for deleted docs correct or should we also track fixing that underlying issue?

That logic is defined in Lucene: https://github.com/apache/lucene/blob/a26a80c89c2e625607b8d2ddb74230b57076d1da/lucene/queries/src/java/org/apache/lucene/queries/CommonTermsQuery.java#L113 (maxDoc includes deletes).

I would argue that the logic is valid, using maxDoc as the denominator. Deleted docs still add cost at matching time, which is what the query is trying to address.

[github-actions]

❌ Gradle check result for 1d6543b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[jed326]

@msfroh thanks for the details, that makes sense to me.

On a different note, it looks like this common terms query is actually deprecated from Elastic 7.3 so wonder if we should also move towards completely removing it as well:

OpenSearch/server/src/main/java/org/opensearch/index/query/CommonTermsQueryBuilder.java

Lines 55 to 70 in aca2e9d

	/**
	* CommonTermsQuery query is a query that executes high-frequency terms in a
	* optional sub-query to prevent slow queries due to "common" terms like
	* stopwords. This query basically builds 2 queries off the {@code #add(Term)
	* added} terms where low-frequency terms are added to a required boolean clause
	* and high-frequency terms are added to an optional boolean clause. The
	* optional clause is only executed if the required "low-frequency' clause
	* matches.
	*
	* @deprecated Since max_optimization optimization landed in 7.0, normal MatchQuery
	* will achieve the same result without any configuration.
	*
	* @opensearch.internal
	*/
	@Deprecated
	public class CommonTermsQueryBuilder extends AbstractQueryBuilder<CommonTermsQueryBuilder> {

[andrross]

It's not clear to me why that would make this test flaky instead of always failing.

@jed326 I was able to get it to deterministically fail with a particular random seed. The reason was due to this line: https://github.com/opensearch-project/OpenSearch/blob/main/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java#L1700

If the randomIntBetween(2, 10) produced a 9 or a 10 then it would fail, otherwise it would not.

[andrross]

❌ Gradle check result for 1d6543b: FAILURE

#10006

[github-actions]

❌ Gradle check result for 1d6543b:

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❕ Gradle check result for 1d6543b: UNSTABLE

• TEST FAILURES:

      1 org.opensearch.smoketest.SmokeTestMultiNodeClientYamlTestSuiteIT.test {yaml=pit/10_basic/Delete all}

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (54ff353) 71.09% compared to head (1d6543b) 71.12%.
Report is 6 commits behind head on main.

Files	Patch %	Lines
.../java/org/opensearch/common/settings/Settings.java	77.77%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #11233      +/-   ##
============================================
+ Coverage     71.09%   71.12%   +0.03%     
- Complexity    58752    58757       +5     
============================================
  Files          4888     4888              
  Lines        277207   277222      +15     
  Branches      40282    40287       +5     
============================================
+ Hits         197077   197177     +100     
+ Misses        63654    63563      -91     
- Partials      16476    16482       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.