Wildcard field regex query fix.

[markharwood]

Don’t revert to match_all when query only exists of required clauses that can’t be expressed as queries on ngram index.

Reverting to a match_all is an important optimisation e.g. if users type a* - we don't want to decompress all docs and run a regex on the contents when we can know this is literally a match all expression.

Closes #78391

[elasticmachine]

Pinging @elastic/es-search (Team:Search)

[jimczi]

I find the logic difficult to read to be honest. Is it really worth the complexity ?
I am afraid that we could shoot ourselves in the foot if we change the term queries and miss the usage of MatchAllButRequireVerificationQuery.

[markharwood]

I find the logic difficult to read to be honest. Is it really worth the complexity ?

We already need this level of complexity to faithfully represent all of the logic in the regex when it comes to the construction of the ngram query. We cannot create an approximation query that under-matches or we get false negative bugs which often go undetected (unlike the false positive we spotted here).

Rolling back this optimisation won't reduce much of the complexity - we already need to know the difference between:

1. Match all queries (.* etc)
2. MatchAllButVerify (expressions in the regex we don't have an equivalent ngram query for) and
3. All other "concrete" queries (expressions in the regex we can use as filters in the ngram approximation query)

If we find a 2) we must drop all other ORed 3) clauses to avoid false negatives.
There's no avoiding the complexity of this level of parsing.

The revert-to-match-all-with-no-verification optimisation is a comparatively small addition to the necessarily complex task of translating the regex to an ngram query.

[jimczi]

Rolling back this optimisation won't reduce much of the complexity

I don't get that part. We can remove MatchAllButVerify if we decide to always run the verification. That's the simplification I am talking about.

If we find a 2) we must drop all other ORed 3) clauses to avoid false negatives.

The simplification here would be to add the match_all as an additional should clause and let the boolean query simplify this for us during rewrite ?

[markharwood]

OK, I see what you mean.
However, it would be good to retain something that optimises for a* being a match all without verification - I imagine that's a pretty common mistake for those familiar with wildcard syntax.
@cbuescher suggested a simple string based filter (a regex that looks for simple match-all regexes). That might work but we already have logic with a deep understanding of the parsed expressions so there's also an argument for keeping the logic where it is.
I wonder if, as you suggest, we could let BooleanQuery do the rewrite and use MatchAllDocsQuery instead of MatchAllButVerify but do something sneaky. We can't subclass MatchAllDocsQuery because it is final but we could set a marker on it like a boost:12345 or something for the regex clauses that are genuine match alls like a* (as opposed to the clauses like a+ where we just don't have an equivalent ngram query).
We then let BooleanQuery rewrite all the logic and if the surviving query is a MatchAll with this special boost 12345 we assume it is safe to run without verification. Would that work?

[markharwood]

Closed in favour of new approach in #78839