Skip to content

Commit

Permalink
Update MatchPhrase- and TermQueryBuilder to make use of being able to…
Browse files Browse the repository at this point in the history
… rewrite without a SearchExecutionContext.

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 resolve to a `MatchAllQueryBuilder` with needing a `SearchExecutionContext`, which during the can_match phase means that no searcher needs to be acquired and therefor avoiding making a shard search active / 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 was forgotten as part of elastic#96161 and is needed to complete elastic#95776.
  • Loading branch information
martijnvg committed Jun 18, 2023
1 parent 6d62e09 commit a185e32
Show file tree
Hide file tree
Showing 14 changed files with 179 additions and 85 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.convertToMappingMetadataAwareContext();
if (context != null) {
return doMappingMetadataAwareRewrite(context);
}
return this;
}

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

/**
* Optional rewrite logic that only needs access to mappings or other index metadata.
* 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 doMappingMetadataAwareRewrite(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 doMappingMetadataAwareRewrite(searchExecutionContext);
}

@Override
protected QueryBuilder doMappingMetadataAwareRewrite(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 convertToMappingMetadataAwareContext() {
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 doMappingMetadataAwareRewrite(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 doMappingMetadataAwareRewrite(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 a185e32

Please sign in to comment.