Parent/child: If a p/c query is wrapped in a query filter then Custom…

…QueryWrappingFilter must always be used and any filter wrapping the query filter must never be cached. Closes #7685
elastic · Sep 12, 2014 · 0071276 · 0071276
1 parent 82db060
commit 0071276
Show file tree

Hide file tree

Showing 7 changed files with 107 additions and 11 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
@@ -21,6 +21,7 @@
 
 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 static int calculateMinShouldMatch(int optionalClauseCount, String spec)
 
     }
 
-    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 static Filter wrap(Query query) {
     // 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 != null && 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
@@ -25,7 +25,6 @@
 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 Filter parse(QueryParseContext parseContext) throws IOException, QueryPar
         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
@@ -153,9 +153,9 @@ public Filter parse(QueryParseContext parseContext) throws IOException, QueryPar
             Filter nestedFilter;
             if (join) {
                 ToParentBlockJoinQuery joinQuery = new ToParentBlockJoinQuery(query, parentFilter, ScoreMode.None);
-                nestedFilter = Queries.wrap(joinQuery);
+                nestedFilter = Queries.wrap(joinQuery, parseContext);
             } else {
-                nestedFilter = Queries.wrap(query);
+                nestedFilter = Queries.wrap(query, parseContext);
             }
 
             if (cache) {

diff --git 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 Filter parse(QueryParseContext parseContext) throws IOException, QueryPar
         if (query == null) {
             return null;
         }
-        return Queries.wrap(query);
+        return Queries.wrap(query, parseContext);
     }
 }
diff --git a/src/main/java/org/elasticsearch/index/query/QueryParseContext.java b/src/main/java/org/elasticsearch/index/query/QueryParseContext.java
@@ -43,6 +43,7 @@
 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;
@@ -80,6 +81,8 @@ public static void removeTypes() {
 
     private boolean propagateNoCache = false;
 
+    private boolean requireCustomQueryWrappingFilter = false;
+
     final IndexQueryParserService indexQueryParser;
 
     private final Map<String, Filter> namedFilters = Maps.newHashMap();
@@ -119,6 +122,8 @@ public void reset(XContentParser jp) {
         this.lookup = null;
         this.parser = jp;
         this.namedFilters.clear();
+        this.requireCustomQueryWrappingFilter = false;
+        this.propagateNoCache = false;
     }
 
     public Index index() {
@@ -205,7 +210,7 @@ public void addNamedFilter(String name, Filter filter) {
     }
 
     public void addNamedQuery(String name, Query query) {
-        namedFilters.put(name, Queries.wrap(query));
+        namedFilters.put(name, Queries.wrap(query, this));
     }
 
     public ImmutableMap<String, Filter> copyNamedFilters() {
@@ -245,6 +250,13 @@ public Query parseInnerQuery() throws IOException, QueryParsingException {
             // 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;
     }
 
@@ -387,4 +399,8 @@ public long nowInMillis() {
         }
         return System.currentTimeMillis();
     }
+
+    public boolean requireCustomQueryWrappingFilter() {
+        return requireCustomQueryWrappingFilter;
+    }
 }
diff --git a/src/main/java/org/elasticsearch/search/facet/query/QueryFacetExecutor.java b/src/main/java/org/elasticsearch/search/facet/query/QueryFacetExecutor.java
@@ -51,7 +51,7 @@ public QueryFacetExecutor(Query query) {
         if (possibleFilter != null) {
             this.filter = possibleFilter;
         } else {
-            this.filter = Queries.wrap(query);
+            this.filter = Queries.wrap(query, null);
         }
     }
 

diff --git a/src/test/java/org/elasticsearch/search/child/SimpleChildQuerySearchTests.java b/src/test/java/org/elasticsearch/search/child/SimpleChildQuerySearchTests.java
@@ -63,6 +63,7 @@
 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.search.facet.FacetBuilders.termsFacet;
@@ -1063,6 +1064,79 @@ public void testHasChildAndHasParentFilter_withFilter() throws Exception {
         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")