Rewrite match and match_phrase queries to term queries on keyword fields #82612

romseygeek · 2022-01-14T13:28:41Z

Term queries can in certain circumstances (eg when run against constant keyword
fields) rewrite themselves to match_no_docs queries, which is very useful for filtering
out shards from searches and field_caps requests. But match and match_phrase
queries can reduce down to simple term queries when there is no fuzziness defined
on them, and when they are run using a keyword analyzer.

This commit makes simple match and match_phrase rewrite themselves to term
queries when run against keyword fields.

Fixes #82515

elasticmachine · 2022-01-14T13:28:45Z

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

romseygeek · 2022-01-14T13:48:51Z

@elasticmachine run elasticsearch-ci/packaging-tests-windows-sample

…Fix in future

…void rewrite confusion

romseygeek · 2022-01-14T16:13:32Z

@elasticmachine run elasticsearch-ci/part-1

…rewrite

romseygeek · 2022-01-17T09:56:36Z

qa/full-cluster-restart/src/test/java/org/elasticsearch/upgrades/QueryBuilderBWCIT.java

@@ -63,26 +63,26 @@

    static {
        addCandidate("""
-            "match": { "keyword_field": "value"}


For reviewers: this test writes percolator queries into an old index, then upgrades and tests that reading the index gets the same queries out. This commit changes how keyword fields are rewritten, so breaks the assumption in this test that the queries will look the same. Changing the query so that it targets a text field instead of a keyword field preserves the constraint under test.

javanna · 2022-01-17T10:17:29Z

server/src/test/java/org/elasticsearch/index/query/MatchPhraseQueryBuilderTests.java

+        QueryBuilder queryBuilder = new MatchPhraseQueryBuilder(KEYWORD_FIELD_NAME, "value");
+        SearchExecutionContext context = createSearchExecutionContext();
+        QueryBuilder rewritten = queryBuilder.rewrite(context);
+        assertThat(rewritten, instanceOf(TermQueryBuilder.class));


do we need to check the field and the value, just to make sure?

javanna · 2022-01-17T10:17:47Z

server/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java

+        SearchExecutionContext context = createSearchExecutionContext();
+        QueryBuilder rewritten = queryBuilder.rewrite(context);
+        assertThat(rewritten, instanceOf(TermQueryBuilder.class));
+        assertThat(rewritten.boost(), equalTo(2f));


check field and value?

cbuescher

I also took a quick look and left one minor remark. Also one more edge case that I had a question about: if the original query contains a "zero_match" of ALL (or Null for that matter), I think we get a slight difference if the query value is empty. All other cases should be fine since a Keyword analyzer never returns no tokens, but I think that case needs to be specifically handled.

cbuescher · 2022-01-17T10:49:31Z

server/src/main/java/org/elasticsearch/index/query/MatchPhraseQueryBuilder.java

+        // and possibly shortcut
+        if (analyzer != null) {
+            if (sec.getIndexAnalyzers().get(analyzer) == Lucene.KEYWORD_ANALYZER) {
+                TermQueryBuilder termQueryBuilder = new TermQueryBuilder(fieldName, value);


nit: maybe also add a test for this code path, I think only keyword fields are covered atm

cbuescher · 2022-01-17T10:49:40Z

server/src/main/java/org/elasticsearch/index/query/MatchQueryBuilder.java

+        // If we're using the default keyword analyzer then we can rewrite this to a TermQueryBuilder
+        // and possibly shortcut
+        if (analyzer != null) {
+            if (sec.getIndexAnalyzers().get(analyzer) == Lucene.KEYWORD_ANALYZER) {


nit: maybe also add a test for this code path, I think only keyword fields are covered atm

…rmsquery

romseygeek · 2022-01-17T12:27:11Z

nit: maybe also add a test for this code path, I think only keyword fields are covered atm

An excellent suggestion, as it turns out the code was incorrect. I've updated, with new tests for query-level analyzer overrides and to deal with zero terms queries as well.

cbuescher · 2022-01-17T12:59:18Z

server/src/main/java/org/elasticsearch/index/query/MatchPhraseQueryBuilder.java

+                if (zeroTermsQuery == ZeroTermsQueryOption.ALL) {
+                    return new MatchAllQueryBuilder();
+                }
+                return new MatchNoneQueryBuilder();


Can we use zeroTermsQuery#asQuery() instead if the if statement here? That would include the NULL option which returns null, I don't know if this can cause problems in further rewriting.

cbuescher · 2022-01-17T13:00:56Z

server/src/main/java/org/elasticsearch/index/query/MatchQueryBuilder.java

        }
        return this;
    }

+    private NamedAnalyzer configuredAnalyzer(SearchExecutionContext context) {


maybe you can make the query analyzer an input argument, then the two copies of this could be merged into a static utility function somewhere?

It uses the fieldname as well; I'm not sure how generally useful this is, really. I think two copies is fine?

romseygeek · 2022-01-17T13:37:03Z

It turns out that the MatchQueryParser already detects if we have a keyword analyzer and skips the zero terms query logic in that case; so I think we need to explicitly not handle it here?

romseygeek · 2022-01-17T13:38:42Z

That's the cause of the failing test in VersionStringFieldTests - it indexes an empty version, and checks that searching for an empty string finds it.

romseygeek · 2022-01-17T16:14:45Z

@elasticmachine run elasticsearch-ci/part-1

cbuescher

I see why you backed out of the zero terms query logic, LGTM

…lds (elastic#82612) Term queries can in certain circumstances (eg when run against constant keyword fields) rewrite themselves to match_no_docs queries, which is very useful for filtering out shards from searches and field_caps requests. But match and match_phrase queries can reduce down to simple term queries when there is no fuzziness defined on them, and when they are run using a keyword analyzer. This commit makes simple match and match_phrase rewrite themselves to term queries when run against keyword fields. Fixes elastic#82515

Rewrite match and match_phrase queries to term queries on keyword fields

3c48fd9

romseygeek added >enhancement :Search/Search Search-related issues that do not fall into other categories v8.1.0 labels Jan 14, 2022

romseygeek requested a review from cbuescher January 14, 2022 13:28

romseygeek self-assigned this Jan 14, 2022

elasticmachine added the Team:Search Meta label for search team label Jan 14, 2022

null check

a34a964

romseygeek added 3 commits January 14, 2022 13:56

Can't use mft=null shortcut because percolator breaks that contract. …

7835897

…Fix in future

check boosts; percolator BWC tests should run against text field to a…

45b359b

…void rewrite confusion

Don't rewrite lenient match queries

4f3611e

Merge remote-tracking branch 'origin/master' into query/match-phrase-…

06982a5

…rewrite

romseygeek commented Jan 17, 2022

View reviewed changes

javanna approved these changes Jan 17, 2022

View reviewed changes

further checks

f6bdb1b

cbuescher reviewed Jan 17, 2022

View reviewed changes

check for general keywordanalyzer; rewrite empty queries using zerote…

66e98c9

…rmsquery

cbuescher reviewed Jan 17, 2022

View reviewed changes

romseygeek added 2 commits January 17, 2022 13:45

back out ztq logic - match query doesn't use it for keyword queries

156377d

spotless

138a00b

cbuescher approved these changes Jan 17, 2022

View reviewed changes

romseygeek added the auto-backport-and-merge Automatically create backport pull requests and merge when ready label Jan 17, 2022

romseygeek removed the auto-backport-and-merge Automatically create backport pull requests and merge when ready label Jan 17, 2022

romseygeek merged commit 2d77ef5 into elastic:master Jan 17, 2022

romseygeek deleted the query/match-phrase-rewrite branch January 17, 2022 17:02

flash1293 mentioned this pull request Jan 18, 2022

[Lens] Filtered field list using field caps API elastic/kibana#122915

Merged

lukasolson mentioned this pull request Oct 18, 2022

[KQL] Use term queries for keyword fields elastic/kibana#143599

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite match and match_phrase queries to term queries on keyword fields #82612

Rewrite match and match_phrase queries to term queries on keyword fields #82612

romseygeek commented Jan 14, 2022 •

edited

Loading

elasticmachine commented Jan 14, 2022

romseygeek commented Jan 14, 2022

romseygeek commented Jan 14, 2022

romseygeek Jan 17, 2022

javanna Jan 17, 2022

javanna Jan 17, 2022

cbuescher left a comment

cbuescher Jan 17, 2022

cbuescher Jan 17, 2022

romseygeek commented Jan 17, 2022

cbuescher Jan 17, 2022

cbuescher Jan 17, 2022

romseygeek Jan 17, 2022

romseygeek commented Jan 17, 2022

romseygeek commented Jan 17, 2022

romseygeek commented Jan 17, 2022

cbuescher left a comment

Rewrite match and match_phrase queries to term queries on keyword fields #82612

Rewrite match and match_phrase queries to term queries on keyword fields #82612

Conversation

romseygeek commented Jan 14, 2022 • edited Loading

elasticmachine commented Jan 14, 2022

romseygeek commented Jan 14, 2022

romseygeek commented Jan 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romseygeek commented Jan 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romseygeek commented Jan 17, 2022

romseygeek commented Jan 17, 2022

romseygeek commented Jan 17, 2022

cbuescher left a comment

Choose a reason for hiding this comment

romseygeek commented Jan 14, 2022 •

edited

Loading