Skip to content

Commit

Permalink
Update MatchPhrase- and TermQueryBuilder to be able to rewrite withou…
Browse files Browse the repository at this point in the history
…t a SearchExecutionContext. (#96905)

With this change, both query builders can rewrite without using a search context, because QueryRewriteContext often has all the mapping and other index metadata available.

The TermQueryBuilder can with this change resolve to a MatchNoneQueryBuilder without needing a SearchExecutionContext, which during the can_match phase means that no searcher needs to be acquired and therefor avoid making a shard search active and doing a potentially refresh.

The AbstractQueryBuilder#doRewrite(...) method is altered to by default attempt a coordination rewrite, then fall back to attempt a search rewrite, then finally fall back to do an index metadata aware rewrite. This is inline with what was discussed here: #96161 (comment)

This change was forgotten as part of #96161 and is needed to complete #95776.
  • Loading branch information
martijnvg authored Jun 19, 2023
1 parent 733d465 commit ee4fbe4
Show file tree
Hide file tree
Showing 14 changed files with 175 additions and 82 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.elasticsearch.common.lucene.search.Queries;
import org.elasticsearch.common.regex.Regex;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.index.query.QueryRewriteContext;
import org.elasticsearch.index.query.SearchExecutionContext;

import java.util.Collection;
Expand Down Expand Up @@ -44,14 +45,18 @@ public final boolean isAggregatable() {
* Return whether the constant value of this field matches the provided {@code pattern}
* as documented in {@link Regex#simpleMatch}.
*/
protected abstract boolean matches(String pattern, boolean caseInsensitive, SearchExecutionContext context);
protected abstract boolean matches(String pattern, boolean caseInsensitive, QueryRewriteContext context);

private static String valueToString(Object value) {
return value instanceof BytesRef ? ((BytesRef) value).utf8ToString() : value.toString();
}

@Override
public final Query termQuery(Object value, SearchExecutionContext context) {
return internalTermQuery(value, context);
}

public final Query internalTermQuery(Object value, QueryRewriteContext context) {
String pattern = valueToString(value);
if (matches(pattern, false, context)) {
return Queries.newMatchAllQuery();
Expand All @@ -62,6 +67,10 @@ public final Query termQuery(Object value, SearchExecutionContext context) {

@Override
public final Query termQueryCaseInsensitive(Object value, SearchExecutionContext context) {
return internalTermQueryCaseInsensitive(value, context);
}

public final Query internalTermQueryCaseInsensitive(Object value, QueryRewriteContext context) {
String pattern = valueToString(value);
if (matches(pattern, true, context)) {
return Queries.newMatchAllQuery();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.elasticsearch.index.fielddata.IndexFieldData;
import org.elasticsearch.index.fielddata.ScriptDocValues;
import org.elasticsearch.index.fielddata.plain.ConstantIndexFieldData;
import org.elasticsearch.index.query.QueryRewriteContext;
import org.elasticsearch.index.query.SearchExecutionContext;
import org.elasticsearch.script.field.DelegateDocValuesField;
import org.elasticsearch.search.aggregations.support.CoreValuesSourceType;
Expand Down Expand Up @@ -49,7 +50,7 @@ public String typeName() {
}

@Override
protected boolean matches(String pattern, boolean caseInsensitive, SearchExecutionContext context) {
protected boolean matches(String pattern, boolean caseInsensitive, QueryRewriteContext context) {
if (caseInsensitive) {
// Thankfully, all index names are lower-cased so we don't have to pass a case_insensitive mode flag
// down to all the index name-matching logic. We just lower-case the search string
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,10 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws
if (sec != null) {
return doSearchRewrite(sec);
}
final QueryRewriteContext context = queryRewriteContext.convertToIndexMetadataContext();
if (context != null) {
return doIndexMetadataRewrite(context);
}
return this;
}

Expand All @@ -321,6 +325,18 @@ protected QueryBuilder doSearchRewrite(final SearchExecutionContext searchExecut
return this;
}

/**
* Optional rewrite logic that only needs access to index level metadata and services (e.g. index settings and mappings)
* on the data node, but not the shard / Lucene index.
* The can_match phase can use this logic to early terminate a search without doing any search related i/o.
*
* @param context an {@link QueryRewriteContext} instance that has access the mappings and other index metadata
* @return A {@link QueryBuilder} representing the rewritten query, that could be used to determine whether this query yields result.
*/
protected QueryBuilder doIndexMetadataRewrite(final QueryRewriteContext context) throws IOException {
return this;
}

/**
* For internal usage only!
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,24 +157,26 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep
}

@Override
protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException {
SearchExecutionContext sec = queryRewriteContext.convertToSearchExecutionContext();
if (sec == null) {
return this;
}
protected QueryBuilder doSearchRewrite(SearchExecutionContext searchExecutionContext) throws IOException {
return doIndexMetadataRewrite(searchExecutionContext);
}

@Override
protected QueryBuilder doIndexMetadataRewrite(QueryRewriteContext context) throws IOException {
// If we're using the default keyword analyzer then we can rewrite this to a TermQueryBuilder
// and possibly shortcut
// If we're using a keyword analyzer then we can rewrite this to a TermQueryBuilder
// and possibly shortcut
NamedAnalyzer configuredAnalyzer = configuredAnalyzer(sec);
NamedAnalyzer configuredAnalyzer = configuredAnalyzer(context);
if (configuredAnalyzer != null && configuredAnalyzer.analyzer() instanceof KeywordAnalyzer) {
TermQueryBuilder termQueryBuilder = new TermQueryBuilder(fieldName, value);
return termQueryBuilder.rewrite(sec);
return termQueryBuilder.rewrite(context);
} else {
return this;
}
return this;
}

private NamedAnalyzer configuredAnalyzer(SearchExecutionContext context) {
private NamedAnalyzer configuredAnalyzer(QueryRewriteContext context) {
if (analyzer != null) {
return context.getIndexAnalyzers().get(analyzer);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,13 @@ public CoordinatorRewriteContext convertToCoordinatorRewriteContext() {
return null;
}

/**
* @return an {@link QueryRewriteContext} instance that is aware of the mapping and other index metadata or <code>null</code> otherwise.
*/
public QueryRewriteContext convertToIndexMetadataContext() {
return mapperService != null ? this : null;
}

/**
* Returns the {@link MappedFieldType} for the provided field name.
* If the field is not mapped, the behaviour depends on the index.query.parse.allow_unmapped_fields setting, which defaults to true.
Expand Down Expand Up @@ -258,4 +265,21 @@ public void onFailure(Exception e) {
public Index getFullyQualifiedIndex() {
return fullyQualifiedIndex;
}

/**
* Returns the index settings for this context. This might return null if the
* context has not index scope.
*/
public IndexSettings getIndexSettings() {
return indexSettings;
}

/**
* Given an index pattern, checks whether it matches against the current shard. The pattern
* may represent a fully qualified index name if the search targets remote shards.
*/
public boolean indexMatches(String pattern) {
assert indexNameMatcher != null;
return indexNameMatcher.test(pattern);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -468,15 +468,6 @@ public IndexVersion indexVersionCreated() {
return indexSettings.getIndexVersionCreated();
}

/**
* Given an index pattern, checks whether it matches against the current shard. The pattern
* may represent a fully qualified index name if the search targets remote shards.
*/
public boolean indexMatches(String pattern) {
assert indexNameMatcher != null;
return indexNameMatcher.test(pattern);
}

public boolean indexSortedOnField(String field) {
IndexSortConfig indexSortConfig = indexSettings.getIndexSortConfig();
return indexSortConfig.hasPrimarySortOnField(field);
Expand Down Expand Up @@ -607,14 +598,6 @@ public final SearchExecutionContext convertToSearchExecutionContext() {
return this;
}

/**
* Returns the index settings for this context. This might return null if the
* context has not index scope.
*/
public IndexSettings getIndexSettings() {
return indexSettings;
}

/** Return the current {@link IndexReader}, or {@code null} if no index reader is available,
* for instance if this rewrite context is used to index queries (percolation).
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,33 +168,35 @@ protected void addExtraXContent(XContentBuilder builder, Params params) throws I
}

@Override
protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException {
SearchExecutionContext context = queryRewriteContext.convertToSearchExecutionContext();
if (context != null) {
MappedFieldType fieldType = context.getFieldType(this.fieldName);
if (fieldType == null) {
return new MatchNoneQueryBuilder();
} else if (fieldType instanceof ConstantFieldType) {
// This logic is correct for all field types, but by only applying it to constant
// fields we also have the guarantee that it doesn't perform I/O, which is important
// since rewrites might happen on a network thread.
Query query = null;
if (caseInsensitive) {
query = fieldType.termQueryCaseInsensitive(value, context);
} else {
query = fieldType.termQuery(value, context);
}
protected QueryBuilder doSearchRewrite(SearchExecutionContext searchExecutionContext) throws IOException {
return doIndexMetadataRewrite(searchExecutionContext);
}

if (query instanceof MatchAllDocsQuery) {
return new MatchAllQueryBuilder();
} else if (query instanceof MatchNoDocsQuery) {
return new MatchNoneQueryBuilder();
} else {
assert false : "Constant fields must produce match-all or match-none queries, got " + query;
}
@Override
protected QueryBuilder doIndexMetadataRewrite(QueryRewriteContext context) throws IOException {
MappedFieldType fieldType = context.getFieldType(this.fieldName);
if (fieldType == null) {
return new MatchNoneQueryBuilder();
} else if (fieldType instanceof ConstantFieldType constantFieldType) {
// This logic is correct for all field types, but by only applying it to constant
// fields we also have the guarantee that it doesn't perform I/O, which is important
// since rewrites might happen on a network thread.
Query query;
if (caseInsensitive) {
query = constantFieldType.internalTermQueryCaseInsensitive(value, context);
} else {
query = constantFieldType.internalTermQuery(value, context);
}

if (query instanceof MatchAllDocsQuery) {
return new MatchAllQueryBuilder();
} else if (query instanceof MatchNoDocsQuery) {
return new MatchNoneQueryBuilder();
} else {
assert false : "Constant fields must produce match-all or match-none queries, got " + query;
}
}
return super.doRewrite(queryRewriteContext);
return this;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,36 +192,40 @@ public void testParseFailsWithMultipleFields() throws IOException {

public void testRewriteToTermQueries() throws IOException {
QueryBuilder queryBuilder = new MatchPhraseQueryBuilder(KEYWORD_FIELD_NAME, "value");
SearchExecutionContext context = createSearchExecutionContext();
QueryBuilder rewritten = queryBuilder.rewrite(context);
assertThat(rewritten, instanceOf(TermQueryBuilder.class));
TermQueryBuilder tqb = (TermQueryBuilder) rewritten;
assertEquals(KEYWORD_FIELD_NAME, tqb.fieldName);
assertEquals(new BytesRef("value"), tqb.value);
for (QueryRewriteContext context : new QueryRewriteContext[] { createSearchExecutionContext(), createQueryRewriteContext() }) {
QueryBuilder rewritten = queryBuilder.rewrite(context);
assertThat(rewritten, instanceOf(TermQueryBuilder.class));
TermQueryBuilder tqb = (TermQueryBuilder) rewritten;
assertEquals(KEYWORD_FIELD_NAME, tqb.fieldName);
assertEquals(new BytesRef("value"), tqb.value);
}
}

public void testRewriteToTermQueryWithAnalyzer() throws IOException {
MatchPhraseQueryBuilder queryBuilder = new MatchPhraseQueryBuilder(TEXT_FIELD_NAME, "value");
queryBuilder.analyzer("keyword");
SearchExecutionContext context = createSearchExecutionContext();
QueryBuilder rewritten = queryBuilder.rewrite(context);
assertThat(rewritten, instanceOf(TermQueryBuilder.class));
TermQueryBuilder tqb = (TermQueryBuilder) rewritten;
assertEquals(TEXT_FIELD_NAME, tqb.fieldName);
assertEquals(new BytesRef("value"), tqb.value);
for (QueryRewriteContext context : new QueryRewriteContext[] { createSearchExecutionContext(), createQueryRewriteContext() }) {
QueryBuilder rewritten = queryBuilder.rewrite(context);
assertThat(rewritten, instanceOf(TermQueryBuilder.class));
TermQueryBuilder tqb = (TermQueryBuilder) rewritten;
assertEquals(TEXT_FIELD_NAME, tqb.fieldName);
assertEquals(new BytesRef("value"), tqb.value);
}
}

public void testRewriteIndexQueryToMatchNone() throws IOException {
QueryBuilder query = new MatchPhraseQueryBuilder("_index", "does_not_exist");
SearchExecutionContext searchExecutionContext = createSearchExecutionContext();
QueryBuilder rewritten = query.rewrite(searchExecutionContext);
assertThat(rewritten, instanceOf(MatchNoneQueryBuilder.class));
for (QueryRewriteContext context : new QueryRewriteContext[] { createSearchExecutionContext(), createQueryRewriteContext() }) {
QueryBuilder rewritten = query.rewrite(context);
assertThat(rewritten, instanceOf(MatchNoneQueryBuilder.class));
}
}

public void testRewriteIndexQueryToNotMatchNone() throws IOException {
QueryBuilder query = new MatchPhraseQueryBuilder("_index", getIndex().getName());
SearchExecutionContext searchExecutionContext = createSearchExecutionContext();
QueryBuilder rewritten = query.rewrite(searchExecutionContext);
assertThat(rewritten, instanceOf(MatchAllQueryBuilder.class));
for (QueryRewriteContext context : new QueryRewriteContext[] { createSearchExecutionContext(), createQueryRewriteContext() }) {
QueryBuilder rewritten = query.rewrite(context);
assertThat(rewritten, instanceOf(MatchAllQueryBuilder.class));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -205,16 +205,18 @@ public void testParseAndSerializeBigInteger() throws IOException {

public void testRewriteIndexQueryToMatchNone() throws IOException {
TermQueryBuilder query = QueryBuilders.termQuery("_index", "does_not_exist");
SearchExecutionContext searchExecutionContext = createSearchExecutionContext();
QueryBuilder rewritten = query.rewrite(searchExecutionContext);
assertThat(rewritten, instanceOf(MatchNoneQueryBuilder.class));
for (QueryRewriteContext context : new QueryRewriteContext[] { createSearchExecutionContext(), createQueryRewriteContext() }) {
QueryBuilder rewritten = query.rewrite(context);
assertThat(rewritten, instanceOf(MatchNoneQueryBuilder.class));
}
}

public void testRewriteIndexQueryToNotMatchNone() throws IOException {
TermQueryBuilder query = QueryBuilders.termQuery("_index", getIndex().getName());
SearchExecutionContext searchExecutionContext = createSearchExecutionContext();
QueryBuilder rewritten = query.rewrite(searchExecutionContext);
assertThat(rewritten, instanceOf(MatchAllQueryBuilder.class));
for (QueryRewriteContext context : new QueryRewriteContext[] { createSearchExecutionContext(), createQueryRewriteContext() }) {
QueryBuilder rewritten = query.rewrite(context);
assertThat(rewritten, instanceOf(MatchAllQueryBuilder.class));
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1027,13 +1027,15 @@ public void testCanMatch() throws Exception {

CountDownLatch latch = new CountDownLatch(1);
SearchShardTask task = new SearchShardTask(123L, "", "", "", null, Collections.emptyMap());
assertEquals(7, numWrapInvocations.get());
service.executeQueryPhase(request, task, new ActionListener<SearchPhaseResult>() {
// Because the foo field used in alias filter is unmapped the term query builder rewrite can resolve to a match no docs query,
// without acquiring a searcher and that means the wrapper is not called
assertEquals(5, numWrapInvocations.get());
service.executeQueryPhase(request, task, new ActionListener<>() {
@Override
public void onResponse(SearchPhaseResult searchPhaseResult) {
try {
// make sure that the wrapper is called when the query is actually executed
assertEquals(8, numWrapInvocations.get());
assertEquals(6, numWrapInvocations.get());
} finally {
latch.countDown();
}
Expand Down
Loading

0 comments on commit ee4fbe4

Please sign in to comment.