From 1b195286400eaa8a5ce2565149dfb051ad86d982 Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Wed, 7 Nov 2018 23:03:58 +0100 Subject: [PATCH 1/2] Remove the distinction between query and filter context in QueryBuilders 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 --- docs/reference/query-dsl/bool-query.asciidoc | 17 +---- .../cluster/metadata/AliasValidator.java | 2 +- .../index/query/AbstractQueryBuilder.java | 13 ---- .../index/query/BoolQueryBuilder.java | 18 +---- .../query/ConstantScoreQueryBuilder.java | 2 +- .../index/query/FuzzyQueryBuilder.java | 3 - .../index/query/QueryBuilder.java | 10 --- .../index/query/QueryShardContext.java | 28 -------- .../index/query/SpanNearQueryBuilder.java | 5 -- .../search/DefaultSearchContext.java | 2 +- .../AdjacencyMatrixAggregatorFactory.java | 2 +- .../filter/FilterAggregatorFactory.java | 2 +- .../filter/FiltersAggregatorFactory.java | 2 +- .../SignificantTermsAggregatorFactory.java | 2 +- .../search/sort/SortBuilder.java | 4 +- .../index/query/BoolQueryBuilderTests.java | 43 ------------ .../query/SpanMultiTermQueryBuilderTests.java | 5 -- .../query/plugin/CustomQueryParserIT.java | 68 ------------------- .../index/query/plugin/DummyQueryBuilder.java | 4 +- .../query/plugin/DummyQueryParserPlugin.java | 5 -- .../bucket/filter/FilterAggregatorTests.java | 25 ------- .../bucket/filter/FiltersAggregatorTests.java | 24 ------- .../SignificantTermsAggregatorTests.java | 28 -------- .../aggregations/AggregatorTestCase.java | 3 - .../test/AbstractQueryTestCase.java | 6 -- .../SecurityIndexSearcherWrapper.java | 2 +- ...yIndexSearcherWrapperIntegrationTests.java | 2 +- 27 files changed, 15 insertions(+), 312 deletions(-) diff --git a/docs/reference/query-dsl/bool-query.asciidoc b/docs/reference/query-dsl/bool-query.asciidoc index a7092aaaab113..d4b5919454836 100644 --- a/docs/reference/query-dsl/bool-query.asciidoc +++ b/docs/reference/query-dsl/bool-query.asciidoc @@ -17,15 +17,7 @@ contribute to the score. in <>, 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 <> 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 <> -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 -<> 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 <> meaning @@ -33,13 +25,6 @@ that scoring is ignored and clauses are considered for caching. Because scoring 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. diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java b/server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java index c9258806d51db..ebcffacf0a6f5 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/AliasValidator.java @@ -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); } } diff --git a/server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java index c0fb00128e556..cca1ca0fcc0d1 100644 --- a/server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java @@ -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; /** diff --git a/server/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java index ee1779d3d5190..4c3c8944cf347 100644 --- a/server/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java @@ -384,12 +384,6 @@ 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; } @@ -397,17 +391,7 @@ protected Query doToQuery(QueryShardContext context) throws IOException { private static void addBooleanClauses(QueryShardContext context, BooleanQuery.Builder booleanQueryBuilder, List 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)); } } diff --git a/server/src/main/java/org/elasticsearch/index/query/ConstantScoreQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/ConstantScoreQueryBuilder.java index e8d98a6b00b0e..9752265a70184 100644 --- a/server/src/main/java/org/elasticsearch/index/query/ConstantScoreQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/ConstantScoreQueryBuilder.java @@ -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); } diff --git a/server/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java index 93528bb952042..954107c656086 100644 --- a/server/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/FuzzyQueryBuilder.java @@ -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); diff --git a/server/src/main/java/org/elasticsearch/index/query/QueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/QueryBuilder.java index 5b765c5cbda5d..19adfebafabc5 100644 --- a/server/src/main/java/org/elasticsearch/index/query/QueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/QueryBuilder.java @@ -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 diff --git a/server/src/main/java/org/elasticsearch/index/query/QueryShardContext.java b/server/src/main/java/org/elasticsearch/index/query/QueryShardContext.java index ac19298ae322d..82bae93e84d49 100644 --- a/server/src/main/java/org/elasticsearch/index/query/QueryShardContext.java +++ b/server/src/main/java/org/elasticsearch/index/query/QueryShardContext.java @@ -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> indexFieldDataLookup, MapperService mapperService, @@ -132,7 +131,6 @@ private void reset() { this.lookup = null; this.namedQueries.clear(); this.nestedScope = new NestedScope(); - this.isFilter = false; } public IndexAnalyzers getIndexAnalyzers() { @@ -178,22 +176,6 @@ public Map 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. @@ -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); diff --git a/server/src/main/java/org/elasticsearch/index/query/SpanNearQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/SpanNearQueryBuilder.java index ceeef6112ae46..7b1d237b825cb 100644 --- a/server/src/main/java/org/elasticsearch/index/query/SpanNearQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/SpanNearQueryBuilder.java @@ -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(); diff --git a/server/src/main/java/org/elasticsearch/search/DefaultSearchContext.java b/server/src/main/java/org/elasticsearch/search/DefaultSearchContext.java index 0b17ec72fbb17..091fd5f8c85e0 100644 --- a/server/src/main/java/org/elasticsearch/search/DefaultSearchContext.java +++ b/server/src/main/java/org/elasticsearch/search/DefaultSearchContext.java @@ -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); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/adjacency/AdjacencyMatrixAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/adjacency/AdjacencyMatrixAggregatorFactory.java index 69bc2de39dca9..7be588bcb5607 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/adjacency/AdjacencyMatrixAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/adjacency/AdjacencyMatrixAggregatorFactory.java @@ -51,7 +51,7 @@ public AdjacencyMatrixAggregatorFactory(String name, List 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); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorFactory.java index c7d500e81ca26..04dd8d3a53cea 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorFactory.java @@ -43,7 +43,7 @@ public class FilterAggregatorFactory extends AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder, Map metaData) throws IOException { super(name, context, parent, subFactoriesBuilder, metaData); - filter = filterBuilder.toFilter(context.getQueryShardContext()); + filter = filterBuilder.toQuery(context.getQueryShardContext()); } /** diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregatorFactory.java index 81a78632d4bd6..cc299765621aa 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregatorFactory.java @@ -56,7 +56,7 @@ public FiltersAggregatorFactory(String name, List 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()); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregatorFactory.java index 3a424b0055f7a..4054f3796d9b5 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregatorFactory.java @@ -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 diff --git a/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java b/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java index a7861dee9bba0..abef6d8ebec33 100644 --- a/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/sort/SortBuilder.java @@ -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(); diff --git a/server/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java index 4a5e303a0542e..36fe12f11d686 100644 --- a/server/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java @@ -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; @@ -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; @@ -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())); @@ -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() diff --git a/server/src/test/java/org/elasticsearch/index/query/SpanMultiTermQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/SpanMultiTermQueryBuilderTests.java index a4bbd1989dabc..47db7d42d8cd0 100644 --- a/server/src/test/java/org/elasticsearch/index/query/SpanMultiTermQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/SpanMultiTermQueryBuilderTests.java @@ -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; diff --git a/server/src/test/java/org/elasticsearch/index/query/plugin/CustomQueryParserIT.java b/server/src/test/java/org/elasticsearch/index/query/plugin/CustomQueryParserIT.java index 7df1f768bc991..f1ebcd971741e 100644 --- a/server/src/test/java/org/elasticsearch/index/query/plugin/CustomQueryParserIT.java +++ b/server/src/test/java/org/elasticsearch/index/query/plugin/CustomQueryParserIT.java @@ -19,10 +19,6 @@ package org.elasticsearch.index.query.plugin; -import org.apache.lucene.search.BooleanClause; -import org.apache.lucene.search.BooleanQuery; -import org.apache.lucene.search.ConstantScoreQuery; -import org.apache.lucene.search.Query; import org.elasticsearch.index.query.BoolQueryBuilder; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.indices.IndicesService; @@ -33,10 +29,7 @@ import java.util.Arrays; import java.util.Collection; -import static org.elasticsearch.index.query.QueryBuilders.boolQuery; -import static org.elasticsearch.index.query.QueryBuilders.constantScoreQuery; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount; -import static org.hamcrest.Matchers.instanceOf; public class CustomQueryParserIT extends ESIntegTestCase { @Override @@ -77,65 +70,4 @@ private static QueryShardContext queryShardContext() { return indicesService.indexServiceSafe(resolveIndex("index")).newQueryShardContext( randomInt(20), null, () -> { throw new UnsupportedOperationException(); }, null); } - - //see #11120 - public void testConstantScoreParsesFilter() throws Exception { - Query q = constantScoreQuery(new DummyQueryBuilder()).toQuery(queryShardContext()); - Query inner = ((ConstantScoreQuery) q).getQuery(); - assertThat(inner, instanceOf(DummyQueryParserPlugin.DummyQuery.class)); - assertEquals(true, ((DummyQueryParserPlugin.DummyQuery) inner).isFilter); - } - - //see #11120 - public void testBooleanParsesFilter() throws Exception { - // single clause, serialized as inner object - Query q = boolQuery() - .should(new DummyQueryBuilder()) - .must(new DummyQueryBuilder()) - .filter(new DummyQueryBuilder()) - .mustNot(new DummyQueryBuilder()).toQuery(queryShardContext()); - assertThat(q, instanceOf(BooleanQuery.class)); - BooleanQuery bq = (BooleanQuery) q; - assertEquals(4, bq.clauses().size()); - for (BooleanClause clause : bq.clauses()) { - DummyQueryParserPlugin.DummyQuery dummy = (DummyQueryParserPlugin.DummyQuery) clause.getQuery(); - switch (clause.getOccur()) { - case FILTER: - case MUST_NOT: - assertEquals(true, dummy.isFilter); - break; - case MUST: - case SHOULD: - assertEquals(false, dummy.isFilter); - break; - default: - throw new AssertionError(); - } - } - - // multiple clauses, serialized as inner arrays - q = boolQuery() - .should(new DummyQueryBuilder()).should(new DummyQueryBuilder()) - .must(new DummyQueryBuilder()).must(new DummyQueryBuilder()) - .filter(new DummyQueryBuilder()).filter(new DummyQueryBuilder()) - .mustNot(new DummyQueryBuilder()).mustNot(new DummyQueryBuilder()).toQuery(queryShardContext()); - assertThat(q, instanceOf(BooleanQuery.class)); - bq = (BooleanQuery) q; - assertEquals(8, bq.clauses().size()); - for (BooleanClause clause : bq.clauses()) { - DummyQueryParserPlugin.DummyQuery dummy = (DummyQueryParserPlugin.DummyQuery) clause.getQuery(); - switch (clause.getOccur()) { - case FILTER: - case MUST_NOT: - assertEquals(true, dummy.isFilter); - break; - case MUST: - case SHOULD: - assertEquals(false, dummy.isFilter); - break; - default: - throw new AssertionError(); - } - } - } } diff --git a/server/src/test/java/org/elasticsearch/index/query/plugin/DummyQueryBuilder.java b/server/src/test/java/org/elasticsearch/index/query/plugin/DummyQueryBuilder.java index cbd76877ce49a..16f7726a87732 100644 --- a/server/src/test/java/org/elasticsearch/index/query/plugin/DummyQueryBuilder.java +++ b/server/src/test/java/org/elasticsearch/index/query/plugin/DummyQueryBuilder.java @@ -58,7 +58,7 @@ public static DummyQueryBuilder fromXContent(XContentParser parser) throws IOExc @Override protected Query doToQuery(QueryShardContext context) throws IOException { - return new DummyQuery(context.isFilter()); + return new DummyQuery(); } @Override @@ -75,4 +75,4 @@ protected boolean doEquals(DummyQueryBuilder other) { public String getWriteableName() { return NAME; } -} \ No newline at end of file +} diff --git a/server/src/test/java/org/elasticsearch/index/query/plugin/DummyQueryParserPlugin.java b/server/src/test/java/org/elasticsearch/index/query/plugin/DummyQueryParserPlugin.java index 02653dcfd0e4d..3f57712a51b7e 100644 --- a/server/src/test/java/org/elasticsearch/index/query/plugin/DummyQueryParserPlugin.java +++ b/server/src/test/java/org/elasticsearch/index/query/plugin/DummyQueryParserPlugin.java @@ -40,13 +40,8 @@ public List> getQueries() { } public static class DummyQuery extends Query { - public final boolean isFilter; private final Query matchAllDocsQuery = new MatchAllDocsQuery(); - public DummyQuery(boolean isFilter) { - this.isFilter = isFilter; - } - @Override public String toString(String field) { return getClass().getSimpleName(); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorTests.java index f3d057d8e8cd0..d92fb7ff62e43 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FilterAggregatorTests.java @@ -23,24 +23,17 @@ import org.apache.lucene.index.DirectoryReader; import org.apache.lucene.index.IndexOptions; import org.apache.lucene.index.IndexReader; -import org.apache.lucene.index.MultiReader; import org.apache.lucene.index.RandomIndexWriter; -import org.apache.lucene.search.BooleanQuery; import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.MatchAllDocsQuery; -import org.apache.lucene.search.Query; import org.apache.lucene.store.Directory; import org.elasticsearch.index.mapper.KeywordFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryBuilders; -import org.elasticsearch.search.aggregations.AggregatorFactory; import org.elasticsearch.search.aggregations.AggregatorTestCase; -import org.hamcrest.Matchers; import org.junit.Before; -import java.io.IOException; - public class FilterAggregatorTests extends AggregatorTestCase { private MappedFieldType fieldType; @@ -107,22 +100,4 @@ public void testRandom() throws Exception { indexReader.close(); directory.close(); } - - public void testParsedAsFilter() throws IOException { - IndexReader indexReader = new MultiReader(); - IndexSearcher indexSearcher = newSearcher(indexReader); - QueryBuilder filter = QueryBuilders.boolQuery() - .must(QueryBuilders.termQuery("field", "foo")) - .should(QueryBuilders.termQuery("field", "bar")); - FilterAggregationBuilder builder = new FilterAggregationBuilder("test", filter); - AggregatorFactory factory = createAggregatorFactory(builder, indexSearcher, fieldType); - assertThat(factory, Matchers.instanceOf(FilterAggregatorFactory.class)); - FilterAggregatorFactory filterFactory = (FilterAggregatorFactory) factory; - Query parsedQuery = filterFactory.getWeight().getQuery(); - assertThat(parsedQuery, Matchers.instanceOf(BooleanQuery.class)); - assertEquals(2, ((BooleanQuery) parsedQuery).clauses().size()); - // means the bool query has been parsed as a filter, if it was a query minShouldMatch would - // be 0 - assertEquals(1, ((BooleanQuery) parsedQuery).getMinimumNumberShouldMatch()); - } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregatorTests.java index 6fdf207249f43..05ed091978270 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregatorTests.java @@ -23,23 +23,17 @@ import org.apache.lucene.index.DirectoryReader; import org.apache.lucene.index.IndexOptions; import org.apache.lucene.index.IndexReader; -import org.apache.lucene.index.MultiReader; import org.apache.lucene.index.RandomIndexWriter; -import org.apache.lucene.search.BooleanQuery; import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.MatchAllDocsQuery; -import org.apache.lucene.search.Query; import org.apache.lucene.store.Directory; import org.elasticsearch.index.mapper.KeywordFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryBuilders; -import org.elasticsearch.search.aggregations.AggregatorFactory; import org.elasticsearch.search.aggregations.AggregatorTestCase; -import org.hamcrest.Matchers; import org.junit.Before; -import java.io.IOException; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -203,22 +197,4 @@ public void testRandom() throws Exception { indexReader.close(); directory.close(); } - - public void testParsedAsFilter() throws IOException { - IndexReader indexReader = new MultiReader(); - IndexSearcher indexSearcher = newSearcher(indexReader); - QueryBuilder filter = QueryBuilders.boolQuery() - .must(QueryBuilders.termQuery("field", "foo")) - .should(QueryBuilders.termQuery("field", "bar")); - FiltersAggregationBuilder builder = new FiltersAggregationBuilder("test", filter); - AggregatorFactory factory = createAggregatorFactory(builder, indexSearcher, fieldType); - assertThat(factory, Matchers.instanceOf(FiltersAggregatorFactory.class)); - FiltersAggregatorFactory filtersFactory = (FiltersAggregatorFactory) factory; - Query parsedQuery = filtersFactory.getWeights()[0].getQuery(); - assertThat(parsedQuery, Matchers.instanceOf(BooleanQuery.class)); - assertEquals(2, ((BooleanQuery) parsedQuery).clauses().size()); - // means the bool query has been parsed as a filter, if it was a query minShouldMatch would - // be 0 - assertEquals(1, ((BooleanQuery) parsedQuery).getMinimumNumberShouldMatch()); - } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregatorTests.java index 0485d4f58550f..72403f8f7820b 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregatorTests.java @@ -28,11 +28,8 @@ import org.apache.lucene.index.IndexReader; import org.apache.lucene.index.IndexWriter; import org.apache.lucene.index.IndexWriterConfig; -import org.apache.lucene.index.MultiReader; import org.apache.lucene.index.Term; -import org.apache.lucene.search.BooleanQuery; import org.apache.lucene.search.IndexSearcher; -import org.apache.lucene.search.Query; import org.apache.lucene.search.TermQuery; import org.apache.lucene.store.Directory; import org.apache.lucene.util.BytesRef; @@ -46,12 +43,9 @@ import org.elasticsearch.index.mapper.TextFieldMapper.TextFieldType; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryBuilders; -import org.elasticsearch.search.aggregations.AggregatorFactory; import org.elasticsearch.search.aggregations.AggregatorTestCase; import org.elasticsearch.search.aggregations.bucket.significant.SignificantTermsAggregatorFactory.ExecutionMode; import org.elasticsearch.search.aggregations.bucket.terms.IncludeExclude; -import org.elasticsearch.search.aggregations.support.ValueType; -import org.hamcrest.Matchers; import org.junit.Before; import java.io.IOException; @@ -86,28 +80,6 @@ protected Map getFieldAliases(MappedFieldType... fieldT Function.identity())); } - public void testParsedAsFilter() throws IOException { - IndexReader indexReader = new MultiReader(); - IndexSearcher indexSearcher = newSearcher(indexReader); - QueryBuilder filter = QueryBuilders.boolQuery() - .must(QueryBuilders.termQuery("field", "foo")) - .should(QueryBuilders.termQuery("field", "bar")); - SignificantTermsAggregationBuilder builder = new SignificantTermsAggregationBuilder( - "test", ValueType.STRING) - .field("field") - .backgroundFilter(filter); - AggregatorFactory factory = createAggregatorFactory(builder, indexSearcher, fieldType); - assertThat(factory, Matchers.instanceOf(SignificantTermsAggregatorFactory.class)); - SignificantTermsAggregatorFactory sigTermsFactory = - (SignificantTermsAggregatorFactory) factory; - Query parsedQuery = sigTermsFactory.filter; - assertThat(parsedQuery, Matchers.instanceOf(BooleanQuery.class)); - assertEquals(2, ((BooleanQuery) parsedQuery).clauses().size()); - // means the bool query has been parsed as a filter, if it was a query minShouldMatch would - // be 0 - assertEquals(1, ((BooleanQuery) parsedQuery).getMinimumNumberShouldMatch()); - } - /** * Uses the significant terms aggregation to find the keywords in text fields */ diff --git a/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java b/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java index 9ed246ae2768b..40fe0d9135dda 100644 --- a/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java @@ -75,7 +75,6 @@ import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.InternalAggregationTestCase; import org.junit.After; -import org.mockito.Matchers; import java.io.IOException; import java.util.ArrayList; @@ -303,8 +302,6 @@ protected QueryShardContext queryShardContextMock(MapperService mapperService) { QueryShardContext queryShardContext = mock(QueryShardContext.class); when(queryShardContext.getMapperService()).thenReturn(mapperService); NestedScope nestedScope = new NestedScope(); - when(queryShardContext.isFilter()).thenCallRealMethod(); - Mockito.doCallRealMethod().when(queryShardContext).setIsFilter(Matchers.anyBoolean()); when(queryShardContext.nestedScope()).thenReturn(nestedScope); return queryShardContext; } diff --git a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java index cebd63dfa324c..004e3801d7485 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java @@ -466,12 +466,6 @@ public void testToQuery() throws IOException { assertNotEquals("modifying the boost doesn't affect the corresponding lucene query", rewrite(firstLuceneQuery), rewrite(thirdLuceneQuery)); } - - // check that context#isFilter is not changed by invoking toQuery/rewrite - boolean filterFlag = randomBoolean(); - context.setIsFilter(filterFlag); - rewriteQuery(firstQuery, context).toQuery(context); - assertEquals("isFilter should be unchanged", filterFlag, context.isFilter()); } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexSearcherWrapper.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexSearcherWrapper.java index dbb359bb70f1a..a8651701448d2 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexSearcherWrapper.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexSearcherWrapper.java @@ -136,7 +136,7 @@ protected DirectoryReader wrap(DirectoryReader reader) { QueryBuilder queryBuilder = queryShardContext.parseInnerQueryBuilder(parser); verifyRoleQuery(queryBuilder); failIfQueryUsesClient(queryBuilder, queryShardContext); - Query roleQuery = queryShardContext.toFilter(queryBuilder).query(); + Query roleQuery = queryShardContext.toQuery(queryBuilder).query(); filter.add(roleQuery, SHOULD); if (queryShardContext.getMapperService().hasNested()) { NestedHelper nestedHelper = new NestedHelper(queryShardContext.getMapperService()); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexSearcherWrapperIntegrationTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexSearcherWrapperIntegrationTests.java index 90b0c6ee77362..ff132894af8ed 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexSearcherWrapperIntegrationTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexSearcherWrapperIntegrationTests.java @@ -142,7 +142,7 @@ protected IndicesAccessControl getIndicesAccessControl() { for (int i = 0; i < numValues; i++) { ParsedQuery parsedQuery = new ParsedQuery(new TermQuery(new Term("field", values[i]))); doReturn(new TermQueryBuilder("field", values[i])).when(queryShardContext).parseInnerQueryBuilder(any(XContentParser.class)); - when(queryShardContext.toFilter(new TermsQueryBuilder("field", values[i]))).thenReturn(parsedQuery); + when(queryShardContext.toQuery(new TermsQueryBuilder("field", values[i]))).thenReturn(parsedQuery); DirectoryReader wrappedDirectoryReader = wrapper.wrap(directoryReader); IndexSearcher indexSearcher = wrapper.wrap(new IndexSearcher(wrappedDirectoryReader)); From ac4f49d1b966c3d722051f1e2378a73cc0979f83 Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Mon, 3 Dec 2018 10:14:04 +0100 Subject: [PATCH 2/2] add breaking change note --- docs/reference/migration/migrate_7_0/search.asciidoc | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/reference/migration/migrate_7_0/search.asciidoc b/docs/reference/migration/migrate_7_0/search.asciidoc index 348763762aa5e..5774f17c3e3aa 100644 --- a/docs/reference/migration/migrate_7_0/search.asciidoc +++ b/docs/reference/migration/migrate_7_0/search.asciidoc @@ -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. +