Using prefixQuery to speedup wildcardQuery&regexpQuery in wildcard type when no TermQuery is present

[kkewwei]

Description

[Describe what this change achieves]

Related Issues

Resolves #22471

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• 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]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Incorrect fallback logic In regexpQuery, when the approximation resolves to MatchAllDocsQuery, the code picks a single "longest" prefix term from possiblePrefixTerms and turns it into a PrefixQuery. This is only sound if that single term must appear as a prefix of matching documents. However, possiblePrefixTerms is populated from various sub-nodes (REGEXP_STRING short fragments, REGEXP_CHAR characters, REGEXP_REPEAT_MIN children) collected across concatenations and unions. For a regexp like .*(ab|cd).* or a concatenation, the individual short fragments are not guaranteed prefixes of matching values — they are substrings that may appear anywhere. Using PrefixQuery here can cause valid documents to be missed by the approximation phase (false negatives), leading to incorrect query results (the second-phase matcher never sees candidates outside the prefix set). Please verify that the collected terms are truly required substrings and consider using a substring/NGram-based query rather than PrefixQuery, or restrict the optimization to cases where the term is anchored at the start. List<String> possiblePrefixTerms = new ArrayList<>(); Query approximation = regexpToQuery(name(), regExp, caseInsensitive, possiblePrefixTerms); if (approximation instanceof MatchAllDocsQuery) { if (possiblePrefixTerms.isEmpty()) { approximation = existsQuery(context); } else { String prefixTerm = possiblePrefixTerms.get(0); for (int i = 1; i < possiblePrefixTerms.size(); i++) { String candidate = possiblePrefixTerms.get(i); if (candidate.length() > prefixTerm.length()) { prefixTerm = candidate; } } if (caseInsensitive) { approximation = AutomatonQueries.caseInsensitivePrefixQuery(new Term(name(), prefixTerm)); } else { approximation = new PrefixQuery(new Term(name(), prefixTerm)); } } Prefix vs substring semantics In matchAllTermsQuery, terms shorter than NGRAM_SIZE are converted to PrefixQuery. But getRequiredNGrams collects short terms from non-wildcard sequences that appear between wildcards (i.e., substrings that must appear somewhere, not necessarily as a prefix of the stored value). Using PrefixQuery on such a term will only match documents whose value starts with that term, causing valid matches to be filtered out. For example, pattern *a*b* on a document xxaxxbxx would be excluded because the field value does not start with a or b. This appears to produce incorrect results for the test case "axxb" unless the wildcard field indexes prefix tokens in a way that makes any substring appear as a prefixable token — please confirm this indexing invariant, because if it does not hold, this is a correctness bug. private static BooleanQuery matchAllTermsQuery(String fieldName, Set<String> terms, boolean caseInsensitive) { BooleanQuery.Builder matchAllTermsBuilder = new BooleanQuery.Builder(); Query query; for (String term : terms) { if (term.length() == NGRAM_SIZE) { if (caseInsensitive) { query = AutomatonQueries.caseInsensitiveTermQuery(new Term(fieldName, term)); } else { query = new TermQuery(new Term(fieldName, term)); } } else { assert term.length() < NGRAM_SIZE; if (caseInsensitive) { query = AutomatonQueries.caseInsensitivePrefixQuery(new Term(fieldName, term)); } else { query = new PrefixQuery(new Term(fieldName, term)); } } matchAllTermsBuilder.add(query, BooleanClause.Occur.FILTER); } return matchAllTermsBuilder.build(); Hardcoded ngram size The new branch uses currentSequence.substring(i, i + 3) instead of i + NGRAM_SIZE. If NGRAM_SIZE ever changes, this will silently produce wrong-length terms. Use NGRAM_SIZE for consistency with the surrounding code. if (currentSequence.length() >= NGRAM_SIZE) { for (int i = 0; i < currentSequence.length() - NGRAM_SIZE + 1; i++) { terms.add(currentSequence.substring(i, i + 3)); }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle supplementary code points correctly (char) regExp.c truncates code points above U+FFFF, producing an invalid prefix character for supplementary Unicode characters. Use new String(Character.toChars(regExp.c)) to correctly represent the full code point as a prefix term. server/src/main/java/org/opensearch/index/mapper/WildcardFieldMapper.java [716-720] } else if (regExp.kind == RegExp.Kind.REGEXP_CHAR && possiblePrefixTerms != null) { - possiblePrefixTerms.add(String.valueOf((char) regExp.c)); + possiblePrefixTerms.add(new String(Character.toChars(regExp.c))); return MatchAllDocsQuery.INSTANCE; } else { return MatchAllDocsQuery.INSTANCE; } Suggestion importance[1-10]: 6 __ Why: Valid concern about (char) regExp.c truncating supplementary Unicode code points. Using Character.toChars correctly handles this edge case, though it affects only non-BMP characters.	Low
	Handle non-BMP chars in prefix term When caseInsensitive is true, single-character prefix terms collected via REGEXP_CHAR are added using the raw code point value without lowercasing. Downstream caseInsensitivePrefixQuery may still work, but for consistency and to avoid degenerate cases (e.g. surrogate handling via (char) regExp.c truncating supplementary code points), guard against non-BMP characters or normalize casing before storing. server/src/main/java/org/opensearch/index/mapper/WildcardFieldMapper.java [631-643] +Query approximation = regexpToQuery(name(), regExp, caseInsensitive, possiblePrefixTerms); +if (approximation instanceof MatchAllDocsQuery) { + if (possiblePrefixTerms.isEmpty()) { + approximation = existsQuery(context); + } else { + String prefixTerm = possiblePrefixTerms.get(0); + for (int i = 1; i < possiblePrefixTerms.size(); i++) { + String candidate = possiblePrefixTerms.get(i); + if (candidate.length() > prefixTerm.length()) { + prefixTerm = candidate; + } + } - Suggestion importance[1-10]: 3 __ Why: The suggestion identifies a potential edge case with supplementary code points, but the improved_code is identical to the existing_code, providing no actual fix. Low impact.	Low
General	Fix misleading comment about prefix fallback The comment contradicts the logic: possiblePrefixTerms are only added when terms.isEmpty(), i.e., when there are NO 3-gram terms. Update the comment to reflect that shorter prefix terms are used as a fallback only when no n-gram terms exist, to avoid future confusion during maintenance. server/src/main/java/org/opensearch/index/mapper/WildcardFieldMapper.java [520-523] if (terms.isEmpty()) { - // If there are 3-gram terms, keep the first phase selective with shorter prefix terms. + // If there are no 3-gram terms, fall back to shorter prefix terms to keep the first phase selective. terms.addAll(possiblePrefixTerms); } Suggestion importance[1-10]: 4 __ Why: The existing comment is indeed misleading since prefix terms are added when there are NO 3-gram terms. This is a minor readability improvement.	Low
	Move teardown block to correct location A top-level teardown section defined at the end of the file with --- separator is treated as a regular test, not the YAML test framework's teardown block. If a teardown already exists near the top (from setup), this may conflict; if not, this should be placed at the top of the file (after setup) per OpenSearch YAML test conventions to ensure it runs after each test. rest-api-spec/src/main/resources/rest-api-spec/test/search/270_wildcard_fieldtype_queries.yml [546-551] +--- +teardown: + - do: + indices.delete: + index: test,test1,test2 + ignore: 404 - Suggestion importance[1-10]: 2 __ Why: The improved_code is identical to the existing_code, providing no concrete fix. Also, in OpenSearch YAML tests, teardown at the end of the file is a valid convention.	Low

[github-actions]

✅ Gradle check result for e06492a: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 70.83333% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.50%. Comparing base (d603ae1) to head (e06492a).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
...g/opensearch/index/mapper/WildcardFieldMapper.java	70.83%	5 Missing and 9 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22510      +/-   ##
============================================
+ Coverage     73.46%   73.50%   +0.04%     
- Complexity    76477    76503      +26     
============================================
  Files          6104     6104              
  Lines        346568   346596      +28     
  Branches      49883    49897      +14     
============================================
+ Hits         254598   254767     +169     
+ Misses        71693    71546     -147     
- Partials      20277    20283       +6     

☔ View full report in Codecov by Harness.
📢 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.

[kkewwei]

@msfroh Would you mind take a look in your spare time?

[msfroh]

While it's outside of the scope of this issue, the PrefixQuery instances that get returned don't specify a RewriteMethod (but neither does the pre-existing caseInsensitiveTermQuery).

I can think of a potential edge case where a* still matches a lot of terms. If we assume non-ASCII characters, it can theoretically be as much as 2^32. At that point, we would potentially be better off using a pure DV query, if there's a cheaper lead iterator. Of course, there's some existing Lucene work being done around improving the logic there. (Specifically, I need to get back to apache/lucene#16240. Thanks to this issue, I think I might see some opportunity around prefix queries in particular.)