From b906c8a389b36206685b46d70f114d6cef82cb15 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Fri, 12 Feb 2016 12:53:12 +0100 Subject: [PATCH] apply fixes after review --- .../action/search/TransportSearchAction.java | 1 - .../index/query/AbstractQueryBuilder.java | 19 +++++ .../index/query/BoolQueryBuilder.java | 14 +++- .../index/query/BoostingQueryBuilder.java | 9 +-- .../query/ConstantScoreQueryBuilder.java | 6 +- .../index/query/EmptyQueryBuilder.java | 8 -- .../index/query/GeoShapeQueryBuilder.java | 7 +- .../index/query/HasChildQueryBuilder.java | 6 +- .../index/query/HasParentQueryBuilder.java | 4 +- .../index/query/IndicesQueryBuilder.java | 4 +- .../index/query/MoreLikeThisQueryBuilder.java | 2 +- .../index/query/NestedQueryBuilder.java | 6 +- .../index/query/QueryBuilder.java | 8 +- .../index/query/QueryRewriteContext.java | 19 +++-- .../index/query/QueryShardContext.java | 4 - .../index/query/ScriptQueryBuilder.java | 2 - .../index/query/TemplateQueryBuilder.java | 11 ++- .../index/query/TermsQueryBuilder.java | 6 +- .../index/query/WrapperQueryBuilder.java | 13 ++-- .../FunctionScoreQueryBuilder.java | 25 +++++-- .../index/query/AbstractQueryTestCase.java | 13 ++++ .../index/query/BoolQueryBuilderTests.java | 74 ++++++++++++++++++- .../query/BoostingQueryBuilderTests.java | 56 ++++++++------ .../GeoBoundingBoxQueryBuilderTests.java | 6 ++ .../query/GeoDistanceQueryBuilderTests.java | 6 ++ .../query/GeoDistanceRangeQueryTests.java | 6 ++ .../query/GeoPolygonQueryBuilderTests.java | 6 ++ .../query/GeoShapeQueryBuilderTests.java | 20 +++++ .../query/GeohashCellQueryBuilderTests.java | 6 ++ .../query/TemplateQueryBuilderTests.java | 38 ++++++++++ .../index/query/TermsQueryBuilderTests.java | 12 +++ .../index/query/WrapperQueryBuilderTests.java | 30 ++++++++ 32 files changed, 351 insertions(+), 96 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java b/core/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java index 77afd123644..4eec61b3a63 100644 --- a/core/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java +++ b/core/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java @@ -33,7 +33,6 @@ import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexNotFoundException; import org.elasticsearch.indices.IndexClosedException; -import org.elasticsearch.search.SearchService; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; diff --git a/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java index d2116ae3c05..39c835e4b09 100644 --- a/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java @@ -258,4 +258,23 @@ public abstract class AbstractQueryBuilder> } return queries; } + + @Override + public final QueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOException { + QueryBuilder rewritten = doRewrite(queryShardContext); + if (rewritten == this) { + return rewritten; + } + if (queryName() != null && rewritten.queryName() == null) { // we inherit the name + rewritten.queryName(queryName()); + } + if (boost() != DEFAULT_BOOST && rewritten.boost() == DEFAULT_BOOST) { + rewritten.boost(boost()); + } + return rewritten; + } + + protected QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws IOException { + return this; + } } diff --git a/core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java index 72e13c2c31a..261acff33ce 100644 --- a/core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java @@ -263,6 +263,13 @@ public class BoolQueryBuilder extends AbstractQueryBuilder { @Override protected Query doToQuery(QueryShardContext context) throws IOException { + final int numClauses = mustNotClauses.size() + filterClauses.size() + shouldClauses.size() + mustClauses.size(); + if (numClauses == 1 && must().size() == 1 && boost() == DEFAULT_BOOST) { + // we really only optimize this for testing since we use this to rewrite + // named queries TemplateQueryBuilder and WrapperQueryBuilder. + // it's equivalent but will anyways happen later down the road in the BQ#rewrite method + return mustClauses.get(0).toQuery(context); + } BooleanQuery.Builder booleanQueryBuilder = new BooleanQuery.Builder(); booleanQueryBuilder.setDisableCoord(disableCoord); addBooleanClauses(context, booleanQueryBuilder, mustClauses, BooleanClause.Occur.MUST); @@ -273,6 +280,7 @@ public class BoolQueryBuilder extends AbstractQueryBuilder { if (booleanQuery.clauses().isEmpty()) { return new MatchAllDocsQuery(); } + final String minimumShouldMatch; if (context.isFilter() && this.minimumShouldMatch == null && shouldClauses.size() > 0) { minimumShouldMatch = "1"; @@ -349,7 +357,7 @@ public class BoolQueryBuilder extends AbstractQueryBuilder { } @Override - public QueryBuilder rewrite(QueryRewriteContext queryRewriteContext) throws IOException { + protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException { BoolQueryBuilder newBuilder = new BoolQueryBuilder(); boolean changed = false; final int clauses = mustClauses.size() + mustNotClauses.size() + filterClauses.size() + shouldClauses.size(); @@ -372,14 +380,14 @@ public class BoolQueryBuilder extends AbstractQueryBuilder { return this; } - private static boolean rewriteClauses(QueryRewriteContext queryRewriteContext, List> builders, Consumer> conusmer) throws IOException { + private static boolean rewriteClauses(QueryRewriteContext queryRewriteContext, List> builders, Consumer> consumer) throws IOException { boolean changed = false; for (QueryBuilder builder : builders) { QueryBuilder result = builder.rewrite(queryRewriteContext); if (result != builder) { changed = true; } - conusmer.accept(result); + consumer.accept(result); } return changed; } diff --git a/core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java index 507e85687dc..d03d6894b09 100644 --- a/core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/BoostingQueryBuilder.java @@ -160,12 +160,11 @@ public class BoostingQueryBuilder extends AbstractQueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOException { - QueryBuilder positiveQuery = this.positiveQuery.rewrite(queryShardContext); - QueryBuilder negativeQuery = this.negativeQuery.rewrite(queryShardContext); + protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException { + QueryBuilder positiveQuery = this.positiveQuery.rewrite(queryRewriteContext); + QueryBuilder negativeQuery = this.negativeQuery.rewrite(queryRewriteContext); if (positiveQuery != this.positiveQuery || negativeQuery != this.negativeQuery) { - BoostingQueryBuilder newQueryBuilder = new BoostingQueryBuilder(positiveQuery, negativeQuery) - .boost(boost()).queryName(queryName()); + BoostingQueryBuilder newQueryBuilder = new BoostingQueryBuilder(positiveQuery, negativeQuery); newQueryBuilder.negativeBoost = negativeBoost; return newQueryBuilder; } diff --git a/core/src/main/java/org/elasticsearch/index/query/ConstantScoreQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/ConstantScoreQueryBuilder.java index ad00bba28b0..ee6fa4c2c2c 100644 --- a/core/src/main/java/org/elasticsearch/index/query/ConstantScoreQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/ConstantScoreQueryBuilder.java @@ -106,10 +106,10 @@ public class ConstantScoreQueryBuilder extends AbstractQueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOException { - QueryBuilder rewrite = filterBuilder.rewrite(queryShardContext); + protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException { + QueryBuilder rewrite = filterBuilder.rewrite(queryRewriteContext); if (rewrite != filterBuilder) { - return new ConstantScoreQueryBuilder(rewrite).boost(boost()).queryName(queryName()); + return new ConstantScoreQueryBuilder(rewrite); } return this; } diff --git a/core/src/main/java/org/elasticsearch/index/query/EmptyQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/EmptyQueryBuilder.java index 83f7abae7ae..e810292a95c 100644 --- a/core/src/main/java/org/elasticsearch/index/query/EmptyQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/EmptyQueryBuilder.java @@ -20,24 +20,16 @@ package org.elasticsearch.index.query; import org.apache.lucene.search.Query; -import org.elasticsearch.action.support.ToXContentToBytes; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; -import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentType; import java.io.IOException; -import java.util.Objects; /** * A {@link QueryBuilder} that is a stand in replacement for an empty query clause in the DSL. * The current DSL allows parsing inner queries / filters like "{ }", in order to have a * valid non-null representation of these clauses that actually do nothing we can use this class. - * - * This builder has no corresponding parser and it is not registered under the query name. It is - * intended to be used internally as a stand-in for nested queries that are left empty and should - * be ignored upstream. */ public final class EmptyQueryBuilder extends AbstractQueryBuilder { diff --git a/core/src/main/java/org/elasticsearch/index/query/GeoShapeQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/GeoShapeQueryBuilder.java index db64aa3a7d4..dd186f92ba3 100644 --- a/core/src/main/java/org/elasticsearch/index/query/GeoShapeQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/GeoShapeQueryBuilder.java @@ -235,7 +235,7 @@ public class GeoShapeQueryBuilder extends AbstractQueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOException { + protected QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws IOException { if (this.shape == null) { GetRequest getRequest = new GetRequest(indexedShapeIndex, indexedShapeType, indexedShapeId); ShapeBuilder shape = fetch(queryShardContext.getClient(), getRequest, indexedShapePath); - return new GeoShapeQueryBuilder(this.fieldName, shape).relation(relation).strategy(strategy) - .boost(boost()).queryName(queryName()); + return new GeoShapeQueryBuilder(this.fieldName, shape).relation(relation).strategy(strategy); } return this; } diff --git a/core/src/main/java/org/elasticsearch/index/query/HasChildQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/HasChildQueryBuilder.java index 66c9d77534d..940fa89f03a 100644 --- a/core/src/main/java/org/elasticsearch/index/query/HasChildQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/HasChildQueryBuilder.java @@ -398,15 +398,15 @@ public class HasChildQueryBuilder extends AbstractQueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOException { - QueryBuilder rewrite = query.rewrite(queryShardContext); + protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException { + QueryBuilder rewrite = query.rewrite(queryRewriteContext); if (rewrite != query) { HasChildQueryBuilder hasChildQueryBuilder = new HasChildQueryBuilder(type, rewrite); hasChildQueryBuilder.minChildren = minChildren; hasChildQueryBuilder.maxChildren = maxChildren; hasChildQueryBuilder.scoreMode = scoreMode; hasChildQueryBuilder.queryInnerHits = queryInnerHits; - return hasChildQueryBuilder.queryName(queryName()).boost(boost()); + return hasChildQueryBuilder; } return this; } diff --git a/core/src/main/java/org/elasticsearch/index/query/HasParentQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/HasParentQueryBuilder.java index 10ed269a2bc..9a3637de3f9 100644 --- a/core/src/main/java/org/elasticsearch/index/query/HasParentQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/HasParentQueryBuilder.java @@ -257,13 +257,13 @@ public class HasParentQueryBuilder extends AbstractQueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOException { + protected QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws IOException { QueryBuilder rewrite = query.rewrite(queryShardContext); if (rewrite != query) { HasParentQueryBuilder hasParentQueryBuilder = new HasParentQueryBuilder(type, rewrite); hasParentQueryBuilder.score = score; hasParentQueryBuilder.innerHit = innerHit; - return hasParentQueryBuilder.queryName(queryName()).boost(boost()); + return hasParentQueryBuilder; } return this; } diff --git a/core/src/main/java/org/elasticsearch/index/query/IndicesQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/IndicesQueryBuilder.java index c366394976e..019e18d1344 100644 --- a/core/src/main/java/org/elasticsearch/index/query/IndicesQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/IndicesQueryBuilder.java @@ -142,11 +142,11 @@ public class IndicesQueryBuilder extends AbstractQueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOException { + protected QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws IOException { QueryBuilder newInnnerQuery = innerQuery.rewrite(queryShardContext); QueryBuilder newNoMatchQuery = noMatchQuery.rewrite(queryShardContext); if (newInnnerQuery != innerQuery || newNoMatchQuery != noMatchQuery) { - return new IndicesQueryBuilder(innerQuery, indices).noMatchQuery(noMatchQuery).boost(boost()).queryName(queryName()); + return new IndicesQueryBuilder(innerQuery, indices).noMatchQuery(noMatchQuery); } return this; } diff --git a/core/src/main/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilder.java index c08cf205e62..9f6c8b24c4f 100644 --- a/core/src/main/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilder.java @@ -1052,7 +1052,7 @@ public class MoreLikeThisQueryBuilder extends AbstractQueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOException { + protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException { // TODO this needs heavy cleanups before we can rewrite it return this; } diff --git a/core/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java index 461ebdb52a0..596c2499211 100644 --- a/core/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java @@ -226,10 +226,10 @@ public class NestedQueryBuilder extends AbstractQueryBuilder } @Override - public QueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOException { - QueryBuilder rewrite = query.rewrite(queryShardContext); + protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException { + QueryBuilder rewrite = query.rewrite(queryRewriteContext); if (rewrite != query) { - return new NestedQueryBuilder(path, rewrite).queryName(queryName()).boost(boost()).scoreMode(scoreMode); + return new NestedQueryBuilder(path, rewrite).scoreMode(scoreMode); } return this; } diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java index 3667905bacf..f8010e7b523 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java @@ -74,16 +74,16 @@ public interface QueryBuilder> extends NamedWriteabl String getName(); /** - * Rewrites this query builder into it's primitive form. By default this method return theb builder itself. If the builder - * did not change the identity reference must be returend otherwise the builder will be rewritten infinitely. + * Rewrites this query builder into its primitive form. By default this method return the builder itself. If the builder + * did not change the identity reference must be returned otherwise the builder will be rewritten infinitely. */ default QueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOException { return this; } /** - * Rewrites the given query into it's primitive form. Queries that for instance fetch resources from remote hosts or - * can simplify / optimize itself should do their heavy lifting duringt {@link #rewrite(QueryRewriteContext)}. This method + * Rewrites the given query into its primitive form. Queries that for instance fetch resources from remote hosts or + * can simplify / optimize itself should do their heavy lifting during {@link #rewrite(QueryRewriteContext)}. This method * rewrites the query until it doesn't change anymore. * @throws IOException if an {@link IOException} occurs */ diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java b/core/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java index 425673cb548..e057aff06b1 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java @@ -24,6 +24,7 @@ import org.elasticsearch.indices.query.IndicesQueriesRegistry; import org.elasticsearch.script.ScriptService; /** + * Context object used to rewrite {@link QueryBuilder} instances into simplified version. */ public class QueryRewriteContext { protected final ScriptService scriptService; @@ -38,26 +39,34 @@ public class QueryRewriteContext { this.parseContext = new QueryParseContext(indicesQueriesRegistry); } + /** + * Returns a clients to fetch resources from local or remove nodes. + */ public final Client getClient() { return scriptService.getClient(); } + /** + * Returns the index settings for this context. This might return null if the + * context has not index scope. + */ public final IndexSettings getIndexSettings() { return indexSettings; } + /** + * Returns a script service to fetch scripts. + */ public final ScriptService getScriptService() { return scriptService; } + /** + * Returns a new {@link QueryParseContext} to parse template or wrapped queries. + */ public QueryParseContext newParseContext() { QueryParseContext queryParseContext = new QueryParseContext(indicesQueriesRegistry); queryParseContext.parseFieldMatcher(parseContext.parseFieldMatcher()); return queryParseContext; } - - public boolean hasIndex() { - return indexSettings != null; - } - } diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java b/core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java index 09978122dca..83066fba7bc 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java @@ -191,10 +191,6 @@ public class QueryShardContext extends QueryRewriteContext { return unmodifiableMap(new HashMap<>(namedQueries)); } - public void combineNamedQueries(QueryShardContext context) { - namedQueries.putAll(context.namedQueries); - } - /** * Return whether we are currently parsing a filter or a query. */ diff --git a/core/src/main/java/org/elasticsearch/index/query/ScriptQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/ScriptQueryBuilder.java index 31484fe804a..353dbd668ac 100644 --- a/core/src/main/java/org/elasticsearch/index/query/ScriptQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/ScriptQueryBuilder.java @@ -25,7 +25,6 @@ import org.apache.lucene.search.Query; import org.apache.lucene.search.RandomAccessWeight; import org.apache.lucene.search.Weight; import org.apache.lucene.util.Bits; -import org.elasticsearch.client.Client; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -33,7 +32,6 @@ import org.elasticsearch.script.LeafSearchScript; import org.elasticsearch.script.Script; import org.elasticsearch.script.Script.ScriptField; import org.elasticsearch.script.ScriptContext; -import org.elasticsearch.script.ScriptEngineService; import org.elasticsearch.script.ScriptService; import org.elasticsearch.script.SearchScript; import org.elasticsearch.search.lookup.SearchLookup; diff --git a/core/src/main/java/org/elasticsearch/index/query/TemplateQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/TemplateQueryBuilder.java index 4067f6d87cb..4b3e81ebf16 100644 --- a/core/src/main/java/org/elasticsearch/index/query/TemplateQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/TemplateQueryBuilder.java @@ -127,7 +127,7 @@ public class TemplateQueryBuilder extends AbstractQueryBuilder rewrite(QueryRewriteContext queryRewriteContext) throws IOException { + protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException { ExecutableScript executable = queryRewriteContext.getScriptService().executable(template, ScriptContext.Standard.SEARCH, Collections.emptyMap()); BytesReference querySource = (BytesReference) executable.run(); @@ -135,11 +135,10 @@ public class TemplateQueryBuilder extends AbstractQueryBuilder queryBuilder = queryParseContext.parseInnerQueryBuilder(); - if (queryBuilder.boost() == DEFAULT_BOOST) { - queryBuilder.boost(boost()); // only pass down the boost if it has it's own boost - } - if (queryName() != null) { - queryBuilder.queryName(queryName()); + if (boost() != DEFAULT_BOOST || queryName() != null) { + final BoolQueryBuilder boolQueryBuilder = new BoolQueryBuilder(); + boolQueryBuilder.must(queryBuilder); + return boolQueryBuilder; } return queryBuilder; } diff --git a/core/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java index 570fc76dab8..e75d982ebe0 100644 --- a/core/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/TermsQueryBuilder.java @@ -316,18 +316,18 @@ public class TermsQueryBuilder extends AbstractQueryBuilder { } @Override - public QueryBuilder rewrite(QueryRewriteContext queryRewriteContext) throws IOException { + protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException { if (this.termsLookup != null) { TermsLookup termsLookup = new TermsLookup(this.termsLookup); if (termsLookup.index() == null) { // TODO this should go away? - if (queryRewriteContext.hasIndex()) { + if (queryRewriteContext.getIndexSettings() != null) { termsLookup.index(queryRewriteContext.getIndexSettings().getIndex().getName()); } else { return this; // can't rewrite until we have index scope on the shard } } List values = fetch(termsLookup, queryRewriteContext.getClient()); - return new TermsQueryBuilder(this.fieldName, values).boost(boost()).queryName(queryName()); + return new TermsQueryBuilder(this.fieldName, values); } return this; } diff --git a/core/src/main/java/org/elasticsearch/index/query/WrapperQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/WrapperQueryBuilder.java index 6ba07f766e7..6c7ac0e9084 100644 --- a/core/src/main/java/org/elasticsearch/index/query/WrapperQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/WrapperQueryBuilder.java @@ -105,7 +105,7 @@ public class WrapperQueryBuilder extends AbstractQueryBuilder rewrite(QueryRewriteContext context) throws IOException { + protected QueryBuilder doRewrite(QueryRewriteContext context) throws IOException { try (XContentParser qSourceParser = XContentFactory.xContent(source).createParser(source)) { QueryParseContext parseContext = context.newParseContext(); parseContext.reset(qSourceParser); final QueryBuilder queryBuilder = parseContext.parseInnerQueryBuilder(); - if (queryBuilder.boost() == DEFAULT_BOOST) { - queryBuilder.boost(boost()); - } - if (queryName() != null) { // we inherit the name - queryBuilder.queryName(queryName()); + if (boost() != DEFAULT_BOOST || queryName() != null) { + final BoolQueryBuilder boolQueryBuilder = new BoolQueryBuilder(); + boolQueryBuilder.must(queryBuilder); + return boolQueryBuilder; } return queryBuilder; } diff --git a/core/src/main/java/org/elasticsearch/index/query/functionscore/FunctionScoreQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/functionscore/FunctionScoreQueryBuilder.java index 14a28260842..60959a9789e 100644 --- a/core/src/main/java/org/elasticsearch/index/query/functionscore/FunctionScoreQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/functionscore/FunctionScoreQueryBuilder.java @@ -394,17 +394,32 @@ public class FunctionScoreQueryBuilder extends AbstractQueryBuilder rewrite = filter.rewrite(context); + if (rewrite != filter) { + return new FilterFunctionBuilder(rewrite, scoreFunction); + } + return this; + } } @Override - public QueryBuilder rewrite(QueryRewriteContext queryShardContext) throws IOException { - QueryBuilder queryBuilder = this.query.rewrite(queryShardContext); - if (queryBuilder != query) { - FunctionScoreQueryBuilder newQueryBuilder = new FunctionScoreQueryBuilder(queryBuilder, filterFunctionBuilders); + protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException { + QueryBuilder queryBuilder = this.query.rewrite(queryRewriteContext); + FilterFunctionBuilder[] rewrittenBuilders = new FilterFunctionBuilder[this.filterFunctionBuilders.length]; + boolean rewritten = false; + for (int i = 0; i < rewrittenBuilders.length; i++) { + FilterFunctionBuilder rewrite = filterFunctionBuilders[i].rewrite(queryRewriteContext); + rewritten |= rewrite != filterFunctionBuilders[i]; + rewrittenBuilders[i] = rewrite; + } + if (queryBuilder != query || rewritten) { + FunctionScoreQueryBuilder newQueryBuilder = new FunctionScoreQueryBuilder(queryBuilder, rewrittenBuilders); newQueryBuilder.scoreMode = scoreMode; newQueryBuilder.minScore = minScore; newQueryBuilder.maxBoost = maxBoost; - return newQueryBuilder.queryName(queryName()).boost(boost()); + return newQueryBuilder; } return this; } diff --git a/core/src/test/java/org/elasticsearch/index/query/AbstractQueryTestCase.java b/core/src/test/java/org/elasticsearch/index/query/AbstractQueryTestCase.java index 5b8550b7b1c..19e2cab044e 100644 --- a/core/src/test/java/org/elasticsearch/index/query/AbstractQueryTestCase.java +++ b/core/src/test/java/org/elasticsearch/index/query/AbstractQueryTestCase.java @@ -554,6 +554,7 @@ public abstract class AbstractQueryTestCase> private QueryBuilder rewriteQuery(QB queryBuilder, QueryRewriteContext rewriteContext) throws IOException { QueryBuilder rewritten = QueryBuilder.rewriteQuery(queryBuilder, rewriteContext); + // extra safety to fail fast - serialize the rewritten version to ensure it's serializable. assertSerialization(rewritten); return rewritten; } @@ -975,4 +976,16 @@ public abstract class AbstractQueryTestCase> } return ""; } + + /** + * This test ensures that queries that need to be rewritten have dedicated tests. + * These queries must override this method accordingly. + */ + public void testMustRewrite() throws IOException { + QueryShardContext context = createShardContext(); + context.setAllowUnmappedFields(true); + QB queryBuilder = createTestQueryBuilder(); + setSearchContext(randomTypes); // only set search context for toQuery to be more realistic + queryBuilder.toQuery(context); + } } diff --git a/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java index 30dbcdf4b60..7b3f3ecb7fd 100644 --- a/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/BoolQueryBuilderTests.java @@ -195,14 +195,14 @@ public class BoolQueryBuilderTests extends AbstractQueryTestCase rewritten = boolQueryBuilder.rewrite(queryShardContext()); + if (mustRewrite == false && boolQueryBuilder.must().isEmpty()) { + // if it's empty we rewrite to match all + assertEquals(rewritten, new MatchAllQueryBuilder()); + } else { + BoolQueryBuilder rewrite = (BoolQueryBuilder) rewritten; + if (mustRewrite) { + assertNotSame(rewrite, boolQueryBuilder); + if (boolQueryBuilder.must().isEmpty() == false) { + assertEquals(new TermsQueryBuilder("foo", "must"), rewrite.must().get(0)); + } + if (boolQueryBuilder.should().isEmpty() == false) { + assertEquals(new TermsQueryBuilder("foo", "should"), rewrite.should().get(0)); + } + if (boolQueryBuilder.mustNot().isEmpty() == false) { + assertEquals(new TermsQueryBuilder("foo", "must_not"), rewrite.mustNot().get(0)); + } + if (boolQueryBuilder.filter().isEmpty() == false) { + assertEquals(new TermsQueryBuilder("foo", "filter"), rewrite.filter().get(0)); + } + } else { + assertSame(rewrite, boolQueryBuilder); + if (boolQueryBuilder.must().isEmpty() == false) { + assertSame(boolQueryBuilder.must().get(0), rewrite.must().get(0)); + } + } + } + } + + public void testRewriteMultipleTimes() throws IOException { + BoolQueryBuilder boolQueryBuilder = new BoolQueryBuilder(); + boolQueryBuilder.must(new WrapperQueryBuilder(new WrapperQueryBuilder(new MatchAllQueryBuilder().toString()).toString())); + QueryBuilder rewritten = boolQueryBuilder.rewrite(queryShardContext()); + BoolQueryBuilder expected = new BoolQueryBuilder(); + expected.must(new WrapperQueryBuilder(new MatchAllQueryBuilder().toString())); + assertEquals(expected, rewritten); + + expected = new BoolQueryBuilder(); + expected.must(new MatchAllQueryBuilder()); + QueryBuilder rewrittenAgain = rewritten.rewrite(queryShardContext()); + assertEquals(rewrittenAgain, expected); + assertEquals(QueryBuilder.rewriteQuery(boolQueryBuilder, queryShardContext()), expected); + } } diff --git a/core/src/test/java/org/elasticsearch/index/query/BoostingQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/BoostingQueryBuilderTests.java index f7f50136f77..414ab1f1131 100644 --- a/core/src/test/java/org/elasticsearch/index/query/BoostingQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/BoostingQueryBuilderTests.java @@ -20,6 +20,7 @@ package org.elasticsearch.index.query; import org.apache.lucene.queries.BoostingQuery; +import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.search.Query; import java.io.IOException; @@ -72,27 +73,27 @@ public class BoostingQueryBuilderTests extends AbstractQueryTestCase rewrite = qb.rewrite(queryShardContext()); + if (positive instanceof MatchAllQueryBuilder && negative instanceof MatchAllQueryBuilder) { + assertSame(rewrite, qb); + } else { + assertNotSame(rewrite, qb); + assertEquals(new BoostingQueryBuilder(positive.rewrite(queryShardContext()), negative.rewrite(queryShardContext())), rewrite); + } + } } diff --git a/core/src/test/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilderTests.java index 290a05d1fa1..410db8fc04d 100644 --- a/core/src/test/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilderTests.java @@ -468,4 +468,10 @@ public class GeoBoundingBoxQueryBuilderTests extends AbstractQueryTestCase 0); + super.testMustRewrite(); + } } diff --git a/core/src/test/java/org/elasticsearch/index/query/GeoDistanceQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/GeoDistanceQueryBuilderTests.java index e7b2d862f5b..3d1f02c2e6c 100644 --- a/core/src/test/java/org/elasticsearch/index/query/GeoDistanceQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/GeoDistanceQueryBuilderTests.java @@ -411,4 +411,10 @@ public class GeoDistanceQueryBuilderTests extends AbstractQueryTestCase 0); + super.testMustRewrite(); + } } diff --git a/core/src/test/java/org/elasticsearch/index/query/GeoDistanceRangeQueryTests.java b/core/src/test/java/org/elasticsearch/index/query/GeoDistanceRangeQueryTests.java index 6d9095017bd..f07e695a1a0 100644 --- a/core/src/test/java/org/elasticsearch/index/query/GeoDistanceRangeQueryTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/GeoDistanceRangeQueryTests.java @@ -316,4 +316,10 @@ public class GeoDistanceRangeQueryTests extends AbstractQueryTestCase 0); + super.testMustRewrite(); + } } diff --git a/core/src/test/java/org/elasticsearch/index/query/GeoPolygonQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/GeoPolygonQueryBuilderTests.java index ea16187f6d4..7ab62753889 100644 --- a/core/src/test/java/org/elasticsearch/index/query/GeoPolygonQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/GeoPolygonQueryBuilderTests.java @@ -343,4 +343,10 @@ public class GeoPolygonQueryBuilderTests extends AbstractQueryTestCase 0); + super.testMustRewrite(); + } } diff --git a/core/src/test/java/org/elasticsearch/index/query/GeoShapeQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/GeoShapeQueryBuilderTests.java index 93193cf11e8..533ce3bfb05 100644 --- a/core/src/test/java/org/elasticsearch/index/query/GeoShapeQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/GeoShapeQueryBuilderTests.java @@ -240,4 +240,24 @@ public class GeoShapeQueryBuilderTests extends AbstractQueryTestCase rewrite = sqb.rewrite(queryShardContext()); + GeoShapeQueryBuilder geoShapeQueryBuilder = new GeoShapeQueryBuilder(GEO_SHAPE_FIELD_NAME, indexedShapeToReturn); + geoShapeQueryBuilder.strategy(sqb.strategy()); + geoShapeQueryBuilder.relation(sqb.relation()); + assertEquals(geoShapeQueryBuilder, rewrite); + } } diff --git a/core/src/test/java/org/elasticsearch/index/query/GeohashCellQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/GeohashCellQueryBuilderTests.java index bdc892605d5..bf71e73aa46 100644 --- a/core/src/test/java/org/elasticsearch/index/query/GeohashCellQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/GeohashCellQueryBuilderTests.java @@ -145,4 +145,10 @@ public class GeohashCellQueryBuilderTests extends AbstractQueryTestCase checkGeneratedJson(json, parsed); assertEquals(json, 3, parsed.precision().intValue()); } + + @Override + public void testMustRewrite() throws IOException { + assumeTrue("test runs only when at least a type is registered", getCurrentTypes().length > 0); + super.testMustRewrite(); + } } diff --git a/core/src/test/java/org/elasticsearch/index/query/TemplateQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/TemplateQueryBuilderTests.java index 7a602f09418..c111684b25b 100644 --- a/core/src/test/java/org/elasticsearch/index/query/TemplateQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/TemplateQueryBuilderTests.java @@ -22,6 +22,7 @@ package org.elasticsearch.index.query; import org.apache.lucene.search.Query; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.script.Script.ScriptParseException; import org.elasticsearch.script.ScriptService.ScriptType; @@ -29,6 +30,7 @@ import org.elasticsearch.script.Template; import org.junit.BeforeClass; import java.io.IOException; +import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -118,4 +120,40 @@ public class TemplateQueryBuilderTests extends AbstractQueryTestCase builder = new TemplateQueryBuilder(new Template(query, ScriptType.INLINE, "mockscript", + XContentType.JSON, Collections.emptyMap())); + try { + builder.toQuery(queryShardContext()); + fail(); + } catch (UnsupportedOperationException ex) { + assertEquals("this query must be rewritten first", ex.getMessage()); + } + assertEquals(new MatchAllQueryBuilder(), builder.rewrite(queryShardContext())); + } + + public void testRewriteWithInnerName() throws IOException { + final String query = "{ \"match_all\" : {\"_name\" : \"foobar\"}}"; + QueryBuilder builder = new TemplateQueryBuilder(new Template(query, ScriptType.INLINE, "mockscript", + XContentType.JSON, Collections.emptyMap())); + assertEquals(new MatchAllQueryBuilder().queryName("foobar"), builder.rewrite(queryShardContext())); + + builder = new TemplateQueryBuilder(new Template(query, ScriptType.INLINE, "mockscript", + XContentType.JSON, Collections.emptyMap())).queryName("outer"); + assertEquals(new BoolQueryBuilder().must(new MatchAllQueryBuilder().queryName("foobar")).queryName("outer"), builder.rewrite(queryShardContext())); + } + + public void testRewriteWithInnerBoost() throws IOException { + final TermQueryBuilder query = new TermQueryBuilder("foo", "bar").boost(2); + QueryBuilder builder = new TemplateQueryBuilder(new Template(query.toString(), ScriptType.INLINE, "mockscript", + XContentType.JSON, Collections.emptyMap())); + assertEquals(query, builder.rewrite(queryShardContext())); + + builder = new TemplateQueryBuilder(new Template(query.toString(), ScriptType.INLINE, "mockscript", + XContentType.JSON, Collections.emptyMap())).boost(3); + assertEquals(new BoolQueryBuilder().must(query).boost(3), builder.rewrite(queryShardContext())); + } } diff --git a/core/src/test/java/org/elasticsearch/index/query/TermsQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/TermsQueryBuilderTests.java index 72241330289..9156108c63b 100644 --- a/core/src/test/java/org/elasticsearch/index/query/TermsQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/TermsQueryBuilderTests.java @@ -272,5 +272,17 @@ public class TermsQueryBuilderTests extends AbstractQueryTestCase rewrite = qb.rewrite(queryShardContext()); + assertEquals(tqb, rewrite); + } + + public void testRewriteWithInnerName() throws IOException { + QueryBuilder builder = new WrapperQueryBuilder("{ \"match_all\" : {\"_name\" : \"foobar\"}}"); + assertEquals(new MatchAllQueryBuilder().queryName("foobar"), builder.rewrite(queryShardContext())); + builder = new WrapperQueryBuilder("{ \"match_all\" : {\"_name\" : \"foobar\"}}").queryName("outer"); + assertEquals(new BoolQueryBuilder().must(new MatchAllQueryBuilder().queryName("foobar")).queryName("outer"), builder.rewrite(queryShardContext())); + } + + public void testRewriteWithInnerBoost() throws IOException { + final TermQueryBuilder query = new TermQueryBuilder("foo", "bar").boost(2); + QueryBuilder builder = new WrapperQueryBuilder(query.toString()); + assertEquals(query, builder.rewrite(queryShardContext())); + builder = new WrapperQueryBuilder(query.toString()).boost(3); + assertEquals(new BoolQueryBuilder().must(query).boost(3), builder.rewrite(queryShardContext())); + } + }