Boolean must --> filter rewrite for queries with constant scores

[peteralfonsi]

Description

This PR automatically rewrites boolean must clauses to filter clauses when that clause would always return the same score. This happens for range/geo-bounding-box queries and match/term/terms queries on numeric fields.

This can cause a significant speedup in some cases, because Lucene may be able to use ImpactsDISI + MaxScoreCache to skip large amounts of docs with non-competitive scores. It can only do this if there is exactly 1 must clause which leads iteration and uses scoring (so, not a numeric query). In cases where this rewrite doesn't enable ImpactsDISI, there should be no perf impact. The time saved on actual scoring is negligible.

Note that it's never possible for this change to take a query which originally used ImpactsDISI and change it so that it doesn't use it after being rewritten. This is because only numeric clauses are moved, and numeric queries aren't eligible for ImpactsDISI in the first place, so they couldn't have been causing speedups from being in a must clause rather than a filter clause.

This PR was blocked for a while by apache/lucene#14542 but now that Lucene 10.2 is in OpenSearch it should be safe to move these clauses around without hurting performance.

Here are some benchmark results from http_logs. After rewriting, the numeric must clause becomes filter so we expect the contender's p50 for the must case to equal both p50s from the filter case. Before each clause I listed whether it was must or filter in the original query.

Original query	p50 before changes (ms)	p50 after changes (ms)
must "request" matches "images" & must "status" matches "200"	2495	29.5
must "request' matches "images" & filter "status" matches "200"	28.0	28.7
must "request" matches "images" & must "status" matches "404"	75.1	24.3
must "request" matches "images" & filter "status" matches "404"	22.1	22.5
must "request" matches "images" & must "status" matches "500"	9.9	9.6
must "request" matches "images" & filter "status" matches "500"	9.7	9.3
must "request" matches "images" & must "timestamp" from 6/10-6/13	1342	32.5
must "request" matches "images" & filter "timestamp" from 6/10-6/13	31.3	30.0
must "request" matches "images" & must "timestamp" from 6/10 00:00 - 6/10 00:01	11.4	7.5
must "request" matches "images" & filter "timestamp" from 6/10 00:00 - 6/10 00:01	7.0	9.3
must "request" matches "images" & must "timestamp" from 6/1-6/13	1428	23.2
must "request" matches "images" & filter "timestamp" from 6/1-6/13	24.6	24.7

Related Issues

Part of #17586

Check List

• Functionality includes testing.
• [N/A] API changes companion pull request created, if applicable.
• [N/A] Public documentation issue/PR created, if applicable.

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]

❌ Gradle check result for 216c461: 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?

[peteralfonsi]

Flaky test: #14509

[github-actions]

✅ Gradle check result for 049c59f: SUCCESS

[codecov]

Codecov Report

Attention: Patch coverage is 83.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 72.82%. Comparing base (5ced78c) to head (8e9d383).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...a/org/opensearch/index/query/BoolQueryBuilder.java	83.33%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #18541      +/-   ##
============================================
- Coverage     72.83%   72.82%   -0.02%     
- Complexity    68450    68460      +10     
============================================
  Files          5563     5563              
  Lines        314144   314164      +20     
  Branches      45544    45554      +10     
============================================
- Hits         228810   228776      -34     
- Misses        66807    66851      +44     
- Partials      18527    18537      +10     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rishabhmaurya]

@peteralfonsi I like this rewrite strategy especially for range query on numeric fields.
If I understand the intention right, we are also including term query in must for rewrites? Aren't scoring relevant there?

[peteralfonsi]

We're only rewriting it for term queries on numeric field, which as I understand it should return a constant score, right?

[msfroh]

We're only rewriting it for term queries on numeric field, which as I understand it should return a constant score, right?

Correct!

Under the hood, the transformation is term query -> "exact" query (via <Type>Point.newExactQuery or SortedNumericDocValuesField.newSlowExactQuery) -> range query (with upper == lower and includeLower == includeUpper == true). In other words, term queries on numeric fields are range queries anyway.

[rishabhmaurya]

ah, I missed that. I think this change makes sense. I will take another look

[github-actions]

✅ Gradle check result for 8e9d383: SUCCESS

[rishabhmaurya]

I think we need to revisit the query rewrite refactoring independent of this change as it is getting a bit complicated over the time.