From 91144fc92f80d02282c7b741787200539566d713 Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Thu, 11 Sep 2014 09:57:23 +0200 Subject: [PATCH] Parent/child: If a p/c query is wrapped in a query filter then CustomQueryWrappingFilter must always be used and any filter wrapping the query filter must never be cached. Closes #7685 --- .../common/lucene/search/Queries.java | 15 +++- .../index/query/FQueryFilterParser.java | 3 +- .../index/query/NestedFilterParser.java | 2 +- .../index/query/QueryFilterParser.java | 2 +- .../index/query/QueryParseContext.java | 18 ++++- .../child/SimpleChildQuerySearchTests.java | 74 +++++++++++++++++++ 6 files changed, 105 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/elasticsearch/common/lucene/search/Queries.java b/src/main/java/org/elasticsearch/common/lucene/search/Queries.java index 8cc9751a016..3e1030c9941 100644 --- a/src/main/java/org/elasticsearch/common/lucene/search/Queries.java +++ b/src/main/java/org/elasticsearch/common/lucene/search/Queries.java @@ -21,6 +21,7 @@ package org.elasticsearch.common.lucene.search; import org.apache.lucene.search.*; import org.elasticsearch.common.Nullable; +import org.elasticsearch.index.query.QueryParseContext; import org.elasticsearch.index.search.child.CustomQueryWrappingFilter; import java.util.List; @@ -141,8 +142,14 @@ public class Queries { } - public static Filter wrap(Query query) { - return FACTORY.wrap(query); + /** + * Wraps a query in a filter. + * + * If a filter has an anti per segment execution / caching nature then @{@link CustomQueryWrappingFilter} is returned + * otherwise the standard {@link org.apache.lucene.search.QueryWrapperFilter} is returned. + */ + public static Filter wrap(Query query, QueryParseContext context) { + return FACTORY.wrap(query, context); } private static final QueryWrapperFilterFactory FACTORY = new QueryWrapperFilterFactory(); @@ -151,8 +158,8 @@ public class Queries { // and potentially miss a forbidden API usage! private static final class QueryWrapperFilterFactory { - public Filter wrap(Query query) { - if (CustomQueryWrappingFilter.shouldUseCustomQueryWrappingFilter(query)) { + public Filter wrap(Query query, QueryParseContext context) { + if (context.requireCustomQueryWrappingFilter() || CustomQueryWrappingFilter.shouldUseCustomQueryWrappingFilter(query)) { return new CustomQueryWrappingFilter(query); } else { return new QueryWrapperFilter(query); diff --git a/src/main/java/org/elasticsearch/index/query/FQueryFilterParser.java b/src/main/java/org/elasticsearch/index/query/FQueryFilterParser.java index cb81cb13027..5e4c51c14d5 100644 --- a/src/main/java/org/elasticsearch/index/query/FQueryFilterParser.java +++ b/src/main/java/org/elasticsearch/index/query/FQueryFilterParser.java @@ -25,7 +25,6 @@ import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.lucene.search.Queries; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.cache.filter.support.CacheKeyFilter; -import org.elasticsearch.index.search.child.CustomQueryWrappingFilter; import java.io.IOException; @@ -86,7 +85,7 @@ public class FQueryFilterParser implements FilterParser { if (query == null) { return null; } - Filter filter = Queries.wrap(query); + Filter filter = Queries.wrap(query, parseContext); if (cache) { filter = parseContext.cacheFilter(filter, cacheKey); } diff --git a/src/main/java/org/elasticsearch/index/query/NestedFilterParser.java b/src/main/java/org/elasticsearch/index/query/NestedFilterParser.java index 1630540b99b..62a69cc3154 100644 --- a/src/main/java/org/elasticsearch/index/query/NestedFilterParser.java +++ b/src/main/java/org/elasticsearch/index/query/NestedFilterParser.java @@ -148,7 +148,7 @@ public class NestedFilterParser implements FilterParser { // expects FixedBitSet instances parentFilter = parseContext.fixedBitSetFilter(parentFilter); - Filter nestedFilter = Queries.wrap(new ToParentBlockJoinQuery(query, parentFilter, ScoreMode.None)); + Filter nestedFilter = Queries.wrap(new ToParentBlockJoinQuery(query, parentFilter, ScoreMode.None), parseContext); if (cache) { nestedFilter = parseContext.cacheFilter(nestedFilter, cacheKey); diff --git a/src/main/java/org/elasticsearch/index/query/QueryFilterParser.java b/src/main/java/org/elasticsearch/index/query/QueryFilterParser.java index 9e2d821854c..aaa5a9d1e99 100644 --- a/src/main/java/org/elasticsearch/index/query/QueryFilterParser.java +++ b/src/main/java/org/elasticsearch/index/query/QueryFilterParser.java @@ -48,6 +48,6 @@ public class QueryFilterParser implements FilterParser { if (query == null) { return null; } - return Queries.wrap(query); + return Queries.wrap(query, parseContext); } } \ No newline at end of file diff --git a/src/main/java/org/elasticsearch/index/query/QueryParseContext.java b/src/main/java/org/elasticsearch/index/query/QueryParseContext.java index 5159579d92a..764da0d78c7 100644 --- a/src/main/java/org/elasticsearch/index/query/QueryParseContext.java +++ b/src/main/java/org/elasticsearch/index/query/QueryParseContext.java @@ -42,6 +42,7 @@ import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.mapper.FieldMapper; import org.elasticsearch.index.mapper.FieldMappers; import org.elasticsearch.index.mapper.MapperService; +import org.elasticsearch.index.search.child.CustomQueryWrappingFilter; import org.elasticsearch.index.similarity.SimilarityService; import org.elasticsearch.script.ScriptService; import org.elasticsearch.search.internal.SearchContext; @@ -79,6 +80,8 @@ public class QueryParseContext { private boolean propagateNoCache = false; + private boolean requireCustomQueryWrappingFilter = false; + final IndexQueryParserService indexQueryParser; private final Map namedFilters = Maps.newHashMap(); @@ -118,6 +121,8 @@ public class QueryParseContext { this.lookup = null; this.parser = jp; this.namedFilters.clear(); + this.requireCustomQueryWrappingFilter = false; + this.propagateNoCache = false; } public Index index() { @@ -200,7 +205,7 @@ public class QueryParseContext { } public void addNamedQuery(String name, Query query) { - namedFilters.put(name, Queries.wrap(query)); + namedFilters.put(name, Queries.wrap(query, this)); } public ImmutableMap copyNamedFilters() { @@ -240,6 +245,13 @@ public class QueryParseContext { // if we are at END_OBJECT, move to the next one... parser.nextToken(); } + if (CustomQueryWrappingFilter.shouldUseCustomQueryWrappingFilter(result)) { + requireCustomQueryWrappingFilter = true; + // If later on, either directly or indirectly this query gets wrapped in a query filter it must never + // get cached even if a filter higher up the chain is configured to do this. This will happen, because + // the result filter will be instance of NoCacheFilter (CustomQueryWrappingFilter) which will in + // #executeFilterParser() set propagateNoCache to true. + } return result; } @@ -382,4 +394,8 @@ public class QueryParseContext { } return System.currentTimeMillis(); } + + public boolean requireCustomQueryWrappingFilter() { + return requireCustomQueryWrappingFilter; + } } diff --git a/src/test/java/org/elasticsearch/search/child/SimpleChildQuerySearchTests.java b/src/test/java/org/elasticsearch/search/child/SimpleChildQuerySearchTests.java index 474b3c79149..a3e0ef28e1c 100644 --- a/src/test/java/org/elasticsearch/search/child/SimpleChildQuerySearchTests.java +++ b/src/test/java/org/elasticsearch/search/child/SimpleChildQuerySearchTests.java @@ -66,6 +66,7 @@ import static org.elasticsearch.common.settings.ImmutableSettings.settingsBuilde import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.elasticsearch.index.query.FilterBuilders.*; import static org.elasticsearch.index.query.QueryBuilders.*; +import static org.elasticsearch.index.query.QueryBuilders.constantScoreQuery; import static org.elasticsearch.index.query.functionscore.ScoreFunctionBuilders.factorFunction; import static org.elasticsearch.index.query.functionscore.ScoreFunctionBuilders.scriptFunction; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.*; @@ -1065,6 +1066,79 @@ public class SimpleChildQuerySearchTests extends ElasticsearchIntegrationTest { assertThat(searchResponse.getHits().hits()[0].id(), equalTo("2")); } + @Test + public void testHasChildAndHasParentWrappedInAQueryFilter() throws Exception { + assertAcked(prepareCreate("test") + .addMapping("parent") + .addMapping("child", "_parent", "type=parent")); + ensureGreen(); + + // query filter in case for p/c shouldn't execute per segment, but rather + client().prepareIndex("test", "parent", "1").setSource("p_field", 1).get(); + client().admin().indices().prepareFlush("test").setForce(true).get(); + client().prepareIndex("test", "child", "2").setParent("1").setSource("c_field", 1).get(); + refresh(); + + SearchResponse searchResponse = client().prepareSearch("test") + .setQuery(filteredQuery(matchAllQuery(), queryFilter(hasChildQuery("child", matchQuery("c_field", 1))))).get(); + assertSearchHit(searchResponse, 1, hasId("1")); + + searchResponse = client().prepareSearch("test") + .setQuery(filteredQuery(matchAllQuery(), queryFilter(topChildrenQuery("child", matchQuery("c_field", 1))))).get(); + assertSearchHit(searchResponse, 1, hasId("1")); + + searchResponse = client().prepareSearch("test") + .setQuery(filteredQuery(matchAllQuery(), queryFilter(hasParentQuery("parent", matchQuery("p_field", 1))))).get(); + assertSearchHit(searchResponse, 1, hasId("2")); + + searchResponse = client().prepareSearch("test") + .setQuery(filteredQuery(matchAllQuery(), queryFilter(boolQuery().must(hasChildQuery("child", matchQuery("c_field", 1)))))).get(); + assertSearchHit(searchResponse, 1, hasId("1")); + + searchResponse = client().prepareSearch("test") + .setQuery(filteredQuery(matchAllQuery(), queryFilter(boolQuery().must(topChildrenQuery("child", matchQuery("c_field", 1)))))).get(); + assertSearchHit(searchResponse, 1, hasId("1")); + + searchResponse = client().prepareSearch("test") + .setQuery(filteredQuery(matchAllQuery(), queryFilter(boolQuery().must(hasParentQuery("parent", matchQuery("p_field", 1)))))).get(); + assertSearchHit(searchResponse, 1, hasId("2")); + } + + @Test + public void testHasChildAndHasParentWrappedInAQueryFilterShouldNeverGetCached() throws Exception { + assertAcked(prepareCreate("test") + .setSettings(ImmutableSettings.builder().put("index.cache.filter.type", "weighted")) + .addMapping("parent") + .addMapping("child", "_parent", "type=parent")); + ensureGreen(); + + client().prepareIndex("test", "parent", "1").setSource("p_field", 1).get(); + client().prepareIndex("test", "child", "2").setParent("1").setSource("c_field", 1).get(); + refresh(); + + for (int i = 0; i < 10; i++) { + SearchResponse searchResponse = client().prepareSearch("test") + .setExplain(true) + .setQuery(constantScoreQuery(boolFilter() + .must(queryFilter(hasChildQuery("child", matchQuery("c_field", 1)))) + .cache(true) + )).get(); + assertSearchHit(searchResponse, 1, hasId("1")); + // Can't start with ConstantScore(cache(BooleanFilter( + assertThat(searchResponse.getHits().getAt(0).explanation().getDescription(), startsWith("ConstantScore(BooleanFilter(")); + + searchResponse = client().prepareSearch("test") + .setExplain(true) + .setQuery(constantScoreQuery(boolFilter() + .must(queryFilter(boolQuery().must(matchAllQuery()).must(hasChildQuery("child", matchQuery("c_field", 1))))) + .cache(true) + )).get(); + assertSearchHit(searchResponse, 1, hasId("1")); + // Can't start with ConstantScore(cache(BooleanFilter( + assertThat(searchResponse.getHits().getAt(0).explanation().getDescription(), startsWith("ConstantScore(BooleanFilter(")); + } + } + @Test public void testSimpleQueryRewrite() throws Exception { assertAcked(prepareCreate("test")