Skip to content

Commit

Permalink
Remove the distinction between query and filter context in QueryBuild…
Browse files Browse the repository at this point in the history
…ers (#35354)

When building a query Lucene distinguishes two cases, queries that require to produce a score and queries that only need to match. We cloned this mechanism in the QueryBuilders in order to be able to produce different queries based on whether they need to produce a score or not. However the only case in es that require this distinction is the BoolQueryBuilder that sets a different minimum_should_match when a `bool` query is built in a filter context..
This behavior doesn't seem right because it makes the matching of `should` clauses different when the score is not required.

Closes #35293
  • Loading branch information
jimczi authored Dec 3, 2018
1 parent 328d022 commit 74aca75
Show file tree
Hide file tree
Showing 28 changed files with 26 additions and 314 deletions.
11 changes: 11 additions & 0 deletions docs/reference/migration/migrate_7_0/search.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -159,3 +159,14 @@ Negative scores in the Function Score Query are deprecated in 6.x, and are
not allowed in this version. If a negative score is produced as a result
of computation (e.g. in `script_score` or `field_value_factor` functions),
an error will be thrown.

[float]
==== The filter context has been removed

The `filter` context has been removed from Elasticsearch's query builders,
the distinction between queries and filters is now decided in Lucene depending
on whether queries need to access score or not. As a result `bool` queries with
`should` clauses that don't need to access the score will no longer set their
`minimum_should_match` to 1. This behavior has been deprecated in the previous
major version.

17 changes: 1 addition & 16 deletions docs/reference/query-dsl/bool-query.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -17,29 +17,14 @@ contribute to the score.
in <<query-filter-context,filter context>>, meaning that scoring is ignored
and clauses are considered for caching.

|`should` |The clause (query) should appear in the matching document. If the
`bool` query is in a <<query-filter-context,query context>> and has a `must` or
`filter` clause then a document will match the `bool` query even if none of the
`should` queries match. In this case these clauses are only used to influence
the score. If the `bool` query is a <<query-filter-context,filter context>>
or has neither `must` or `filter` then at least one of the `should` queries
must match a document for it to match the `bool` query. This behavior may be
explicitly controlled by settings the
<<query-dsl-minimum-should-match,`minimum_should_match`>> parameter.
|`should` |The clause (query) should appear in the matching document.

|`must_not` |The clause (query) must not appear in the matching
documents. Clauses are executed in <<query-filter-context,filter context>> meaning
that scoring is ignored and clauses are considered for caching. Because scoring is
ignored, a score of `0` for all documents is returned.
|=======================================================================

[IMPORTANT]
.Bool query in filter context
========================================================================
If this query is used in a filter context and it has `should`
clauses then at least one `should` clause is required to match.
========================================================================

The `bool` query takes a _more-matches-is-better_ approach, so the score from
each matching `must` or `should` clause will be added together to provide the
final `_score` for each document.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,6 @@ public void validateAliasFilter(String alias, byte[] filter, QueryShardContext q
private static void validateAliasFilter(XContentParser parser, QueryShardContext queryShardContext) throws IOException {
QueryBuilder parseInnerQueryBuilder = parseInnerQueryBuilder(parser);
QueryBuilder queryBuilder = Rewriteable.rewrite(parseInnerQueryBuilder, queryShardContext, true);
queryBuilder.toFilter(queryShardContext);
queryBuilder.toQuery(queryShardContext);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -111,19 +111,6 @@ public final Query toQuery(QueryShardContext context) throws IOException {
return query;
}

@Override
public final Query toFilter(QueryShardContext context) throws IOException {
Query result;
final boolean originalIsFilter = context.isFilter();
try {
context.setIsFilter(true);
result = toQuery(context);
} finally {
context.setIsFilter(originalIsFilter);
}
return result;
}

protected abstract Query doToQuery(QueryShardContext context) throws IOException;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -384,30 +384,14 @@ protected Query doToQuery(QueryShardContext context) throws IOException {
return new MatchAllDocsQuery();
}

final String minimumShouldMatch;
if (context.isFilter() && this.minimumShouldMatch == null && shouldClauses.size() > 0) {
minimumShouldMatch = "1";
} else {
minimumShouldMatch = this.minimumShouldMatch;
}
Query query = Queries.applyMinimumShouldMatch(booleanQuery, minimumShouldMatch);
return adjustPureNegative ? fixNegativeQueryIfNeeded(query) : query;
}

private static void addBooleanClauses(QueryShardContext context, BooleanQuery.Builder booleanQueryBuilder,
List<QueryBuilder> clauses, Occur occurs) throws IOException {
for (QueryBuilder query : clauses) {
Query luceneQuery = null;
switch (occurs) {
case MUST:
case SHOULD:
luceneQuery = query.toQuery(context);
break;
case FILTER:
case MUST_NOT:
luceneQuery = query.toFilter(context);
break;
}
Query luceneQuery = query.toQuery(context);
booleanQueryBuilder.add(new BooleanClause(luceneQuery, occurs));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ public static ConstantScoreQueryBuilder fromXContent(XContentParser parser) thro

@Override
protected Query doToQuery(QueryShardContext context) throws IOException {
Query innerFilter = filterBuilder.toFilter(context);
Query innerFilter = filterBuilder.toQuery(context);
return new ConstantScoreQuery(innerFilter);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -326,9 +326,6 @@ public String getWriteableName() {
protected Query doToQuery(QueryShardContext context) throws IOException {
Query query = null;
String rewrite = this.rewrite;
if (rewrite == null && context.isFilter()) {
rewrite = QueryParsers.CONSTANT_SCORE.getPreferredName();
}
MappedFieldType fieldType = context.fieldMapper(fieldName);
if (fieldType != null) {
query = fieldType.fuzzyQuery(value, fuzziness, prefixLength, maxExpansions, transpositions);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,6 @@ public interface QueryBuilder extends NamedWriteable, ToXContentObject, Rewritea
*/
Query toQuery(QueryShardContext context) throws IOException;

/**
* Converts this QueryBuilder to an unscored lucene {@link Query} that acts as a filter.
* Returns {@code null} if this query should be ignored in the context of
* parent queries.
*
* @param context additional information needed to construct the queries
* @return the {@link Query} or {@code null} if this query should be ignored upstream
*/
Query toFilter(QueryShardContext context) throws IOException;

/**
* Sets the arbitrary name to be assigned to the query (see named queries).
* Implementers should return the concrete type of the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ public String[] getTypes() {
private boolean allowUnmappedFields;
private boolean mapUnmappedFieldAsString;
private NestedScope nestedScope;
private boolean isFilter;

public QueryShardContext(int shardId, IndexSettings indexSettings, BitsetFilterCache bitsetFilterCache,
BiFunction<MappedFieldType, String, IndexFieldData<?>> indexFieldDataLookup, MapperService mapperService,
Expand Down Expand Up @@ -132,7 +131,6 @@ private void reset() {
this.lookup = null;
this.namedQueries.clear();
this.nestedScope = new NestedScope();
this.isFilter = false;
}

public IndexAnalyzers getIndexAnalyzers() {
Expand Down Expand Up @@ -178,22 +176,6 @@ public Map<String, Query> copyNamedQueries() {
return unmodifiableMap(new HashMap<>(namedQueries));
}

/**
* Return whether we are currently parsing a filter or a query.
*/
public boolean isFilter() {
return isFilter;
}

/**
* Public for testing only!
*
* Sets whether we are currently parsing a filter or a query
*/
public void setIsFilter(boolean isFilter) {
this.isFilter = isFilter;
}

/**
* Returns all the fields that match a given pattern. If prefixed with a
* type then the fields will be returned with a type prefix.
Expand Down Expand Up @@ -289,16 +271,6 @@ public Version indexVersionCreated() {
return indexSettings.getIndexVersionCreated();
}

public ParsedQuery toFilter(QueryBuilder queryBuilder) {
return toQuery(queryBuilder, q -> {
Query filter = q.toFilter(this);
if (filter == null) {
return null;
}
return filter;
});
}

public ParsedQuery toQuery(QueryBuilder queryBuilder) {
return toQuery(queryBuilder, q -> {
Query query = q.toQuery(this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -346,11 +346,6 @@ public Query toQuery(QueryShardContext context) throws IOException {
throw new UnsupportedOperationException();
}

@Override
public Query toFilter(QueryShardContext context) throws IOException {
throw new UnsupportedOperationException();
}

@Override
public String queryName() {
throw new UnsupportedOperationException();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ public void preProcess(boolean rewrite) {
// initialize the filtering alias based on the provided filters
try {
final QueryBuilder queryBuilder = request.getAliasFilter().getQueryBuilder();
aliasFilter = queryBuilder == null ? null : queryBuilder.toFilter(queryShardContext);
aliasFilter = queryBuilder == null ? null : queryBuilder.toQuery(queryShardContext);
} catch (IOException e) {
throw new UncheckedIOException(e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public AdjacencyMatrixAggregatorFactory(String name, List<KeyedFilter> filters,
for (int i = 0; i < filters.size(); ++i) {
KeyedFilter keyedFilter = filters.get(i);
this.keys[i] = keyedFilter.key();
Query filter = keyedFilter.filter().toFilter(context.getQueryShardContext());
Query filter = keyedFilter.filter().toQuery(context.getQueryShardContext());
this.weights[i] = contextSearcher.createWeight(contextSearcher.rewrite(filter), ScoreMode.COMPLETE_NO_SCORES, 1f);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public class FilterAggregatorFactory extends AggregatorFactory<FilterAggregatorF
public FilterAggregatorFactory(String name, QueryBuilder filterBuilder, SearchContext context,
AggregatorFactory<?> parent, AggregatorFactories.Builder subFactoriesBuilder, Map<String, Object> metaData) throws IOException {
super(name, context, parent, subFactoriesBuilder, metaData);
filter = filterBuilder.toFilter(context.getQueryShardContext());
filter = filterBuilder.toQuery(context.getQueryShardContext());
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public FiltersAggregatorFactory(String name, List<KeyedFilter> filters, boolean
for (int i = 0; i < filters.size(); ++i) {
KeyedFilter keyedFilter = filters.get(i);
this.keys[i] = keyedFilter.key();
this.filters[i] = keyedFilter.filter().toFilter(context.getQueryShardContext());
this.filters[i] = keyedFilter.filter().toQuery(context.getQueryShardContext());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public SignificantTermsAggregatorFactory(String name,
this.executionHint = executionHint;
this.filter = filterBuilder == null
? null
: filterBuilder.toFilter(context.getQueryShardContext());
: filterBuilder.toQuery(context.getQueryShardContext());
IndexSearcher searcher = context.searcher();
this.supersetNumDocs = filter == null
// Important - need to use the doc count that includes deleted docs
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,9 +226,9 @@ private static Query resolveNestedQuery(QueryShardContext context, NestedSortBui
assert nestedFilter == Rewriteable.rewrite(nestedFilter, context) : "nested filter is not rewritten";
if (parentQuery == null) {
// this is for back-compat, original single level nested sorting never applied a nested type filter
childQuery = nestedFilter.toFilter(context);
childQuery = nestedFilter.toQuery(context);
} else {
childQuery = Queries.filtered(nestedObjectMapper.nestedTypeFilter(), nestedFilter.toFilter(context));
childQuery = Queries.filtered(nestedObjectMapper.nestedTypeFilter(), nestedFilter.toQuery(context));
}
} else {
childQuery = nestedObjectMapper.nestedTypeFilter();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

import org.apache.lucene.search.BooleanClause;
import org.apache.lucene.search.BooleanQuery;
import org.apache.lucene.search.ConstantScoreQuery;
import org.apache.lucene.search.MatchAllDocsQuery;
import org.apache.lucene.search.Query;
import org.elasticsearch.common.ParsingException;
Expand All @@ -41,7 +40,6 @@
import java.util.Map;

import static org.elasticsearch.index.query.QueryBuilders.boolQuery;
import static org.elasticsearch.index.query.QueryBuilders.constantScoreQuery;
import static org.elasticsearch.index.query.QueryBuilders.termQuery;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.instanceOf;
Expand Down Expand Up @@ -176,26 +174,6 @@ public void testEmptyBooleanQuery() throws Exception {
}
}

public void testDefaultMinShouldMatch() throws Exception {
// Queries have a minShouldMatch of 0
BooleanQuery bq = (BooleanQuery) parseQuery(boolQuery().must(termQuery("foo", "bar"))).toQuery(createShardContext());
assertEquals(0, bq.getMinimumNumberShouldMatch());

bq = (BooleanQuery) parseQuery(boolQuery().should(termQuery("foo", "bar"))).toQuery(createShardContext());
assertEquals(0, bq.getMinimumNumberShouldMatch());

// Filters have a minShouldMatch of 0/1
ConstantScoreQuery csq = (ConstantScoreQuery) parseQuery(constantScoreQuery(boolQuery()
.must(termQuery("foo", "bar")))).toQuery(createShardContext());
bq = (BooleanQuery) csq.getQuery();
assertEquals(0, bq.getMinimumNumberShouldMatch());

csq = (ConstantScoreQuery) parseQuery(constantScoreQuery(boolQuery().should(termQuery("foo", "bar"))))
.toQuery(createShardContext());
bq = (BooleanQuery) csq.getQuery();
assertEquals(1, bq.getMinimumNumberShouldMatch());
}

public void testMinShouldMatchFilterWithoutShouldClauses() throws Exception {
BoolQueryBuilder boolQueryBuilder = new BoolQueryBuilder();
boolQueryBuilder.filter(new BoolQueryBuilder().must(new MatchAllQueryBuilder()));
Expand All @@ -216,29 +194,6 @@ public void testMinShouldMatchFilterWithoutShouldClauses() throws Exception {
assertThat(innerBooleanClause.getQuery(), instanceOf(MatchAllDocsQuery.class));
}

public void testMinShouldMatchFilterWithShouldClauses() throws Exception {
BoolQueryBuilder boolQueryBuilder = new BoolQueryBuilder();
boolQueryBuilder.filter(new BoolQueryBuilder().must(new MatchAllQueryBuilder()).should(new MatchAllQueryBuilder()));
Query query = boolQueryBuilder.toQuery(createShardContext());
assertThat(query, instanceOf(BooleanQuery.class));
BooleanQuery booleanQuery = (BooleanQuery) query;
assertThat(booleanQuery.getMinimumNumberShouldMatch(), equalTo(0));
assertThat(booleanQuery.clauses().size(), equalTo(1));
BooleanClause booleanClause = booleanQuery.clauses().get(0);
assertThat(booleanClause.getOccur(), equalTo(BooleanClause.Occur.FILTER));
assertThat(booleanClause.getQuery(), instanceOf(BooleanQuery.class));
BooleanQuery innerBooleanQuery = (BooleanQuery) booleanClause.getQuery();
//we didn't set minimum should match initially, but there are should clauses so it should be 1
assertThat(innerBooleanQuery.getMinimumNumberShouldMatch(), equalTo(1));
assertThat(innerBooleanQuery.clauses().size(), equalTo(2));
BooleanClause innerBooleanClause1 = innerBooleanQuery.clauses().get(0);
assertThat(innerBooleanClause1.getOccur(), equalTo(BooleanClause.Occur.MUST));
assertThat(innerBooleanClause1.getQuery(), instanceOf(MatchAllDocsQuery.class));
BooleanClause innerBooleanClause2 = innerBooleanQuery.clauses().get(1);
assertThat(innerBooleanClause2.getOccur(), equalTo(BooleanClause.Occur.SHOULD));
assertThat(innerBooleanClause2.getQuery(), instanceOf(MatchAllDocsQuery.class));
}

public void testMinShouldMatchBiggerThanNumberOfShouldClauses() throws Exception {
BooleanQuery bq = (BooleanQuery) parseQuery(
boolQuery()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,6 @@ public Query toQuery(QueryShardContext context) throws IOException {
return new TermQuery(new Term("foo", "bar"));
}

@Override
public Query toFilter(QueryShardContext context) throws IOException {
return toQuery(context);
}

@Override
public QueryBuilder queryName(String queryName) {
return this;
Expand Down
Loading

0 comments on commit 74aca75

Please sign in to comment.