Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the distinction between query and filter context in QueryBuilders #35354

Merged
merged 3 commits into from
Dec 3, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also add "optionally" here, like "should optionally appear".


|`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";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the part that confuses me. Why do we need to set minimum_should_match in this case ? I cannot think of one example where this fixes something ? @martijnvg can you confirm ?

} 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 @@ -342,11 +342,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 @@ -175,24 +173,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 @@ -213,29 +193,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