Move parent_id query to the parent-join module (#25072)

jimczi · web-flow · commit 7e60cf3e544f · 2017-06-06T19:35:14.000+02:00
This change moves the parent_id query to the parent-join module and handles the case when only the parent-join field can be declared on an index (index with single type on). If single type is off it uses the legacy parent join field mapper and switch to the new one otherwise (default in 6). Relates #20257
diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java b/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java
@@ -471,14 +471,6 @@ public static MoreLikeThisQueryBuilder moreLikeThisQuery(Item[] likeItems) {
         return moreLikeThisQuery(null, null, likeItems);
     }
 
-    /**
-     * Constructs a new parent id query that returns all child documents of the specified type that
-     * point to the specified id.
-     */
-    public static ParentIdQueryBuilder parentId(String type, String id) {
-        return new ParentIdQueryBuilder(type, id);
-    }
-
     public static NestedQueryBuilder nestedQuery(String path, QueryBuilder query, ScoreMode scoreMode) {
         return new NestedQueryBuilder(path, query, scoreMode);
     }
diff --git a/core/src/main/java/org/elasticsearch/search/SearchModule.java b/core/src/main/java/org/elasticsearch/search/SearchModule.java
@@ -52,7 +52,6 @@
 import org.elasticsearch.index.query.MoreLikeThisQueryBuilder;
 import org.elasticsearch.index.query.MultiMatchQueryBuilder;
 import org.elasticsearch.index.query.NestedQueryBuilder;
-import org.elasticsearch.index.query.ParentIdQueryBuilder;
 import org.elasticsearch.index.query.PrefixQueryBuilder;
 import org.elasticsearch.index.query.QueryBuilder;
 import org.elasticsearch.index.query.QueryParseContext;
@@ -743,7 +742,6 @@ private void registerQueryParsers(List<SearchPlugin> plugins) {
         registerQuery(new QuerySpec<>(GeoPolygonQueryBuilder.NAME, GeoPolygonQueryBuilder::new, GeoPolygonQueryBuilder::fromXContent));
         registerQuery(new QuerySpec<>(ExistsQueryBuilder.NAME, ExistsQueryBuilder::new, ExistsQueryBuilder::fromXContent));
         registerQuery(new QuerySpec<>(MatchNoneQueryBuilder.NAME, MatchNoneQueryBuilder::new, MatchNoneQueryBuilder::fromXContent));
-        registerQuery(new QuerySpec<>(ParentIdQueryBuilder.NAME, ParentIdQueryBuilder::new, ParentIdQueryBuilder::fromXContent));
 
         if (ShapesAvailability.JTS_AVAILABLE && ShapesAvailability.SPATIAL4J_AVAILABLE) {
             registerQuery(new QuerySpec<>(GeoShapeQueryBuilder.NAME, GeoShapeQueryBuilder::new, GeoShapeQueryBuilder::fromXContent));
diff --git a/core/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/MatchQueryBuilderTests.java
@@ -430,7 +430,7 @@ public void testExceptionUsingAnalyzerOnNumericField() {
 
     @Override
     protected void initializeAdditionalMappings(MapperService mapperService) throws IOException {
-        mapperService.merge("t_boost", new CompressedXContent(PutMappingRequest.buildFromSimplifiedDef("t_boost",
+        mapperService.merge("doc", new CompressedXContent(PutMappingRequest.buildFromSimplifiedDef("doc",
             "string_boost", "type=text,boost=4").string()), MapperService.MergeReason.MAPPING_UPDATE, false);
     }
 
diff --git a/core/src/test/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilderTests.java
@@ -97,7 +97,7 @@ private static String[] randomStringFields() {
 
     private Item generateRandomItem() {
         String index = randomBoolean() ? getIndex().getName() : null;
-        String type = getRandomType();  // set to one type to avoid ambiguous types
+        String type = "doc";
         // indexed item or artificial document
         Item item;
         if (randomBoolean()) {
diff --git a/core/src/test/java/org/elasticsearch/index/query/NestedQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/NestedQueryBuilderTests.java
@@ -55,7 +55,7 @@ public class NestedQueryBuilderTests extends AbstractQueryTestCase<NestedQueryBu
 
     @Override
     protected void initializeAdditionalMappings(MapperService mapperService) throws IOException {
-        mapperService.merge("nested_doc", new CompressedXContent(PutMappingRequest.buildFromSimplifiedDef("nested_doc",
+        mapperService.merge("doc", new CompressedXContent(PutMappingRequest.buildFromSimplifiedDef("doc",
                 STRING_FIELD_NAME, "type=text",
                 INT_FIELD_NAME, "type=integer",
                 DOUBLE_FIELD_NAME, "type=double",
diff --git a/core/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/QueryStringQueryBuilderTests.java
@@ -858,19 +858,19 @@ public void testExistsFieldQuery() throws Exception {
 
     public void testDisabledFieldNamesField() throws Exception {
         QueryShardContext context = createShardContext();
-        context.getMapperService().merge("new_type",
+        context.getMapperService().merge("doc",
             new CompressedXContent(
-                PutMappingRequest.buildFromSimplifiedDef("new_type",
+                PutMappingRequest.buildFromSimplifiedDef("doc",
                     "foo", "type=text",
                     "_field_names", "enabled=false").string()),
             MapperService.MergeReason.MAPPING_UPDATE, true);
         QueryStringQueryBuilder queryBuilder = new QueryStringQueryBuilder("foo:*");
         Query query = queryBuilder.toQuery(context);
         Query expected = new WildcardQuery(new Term("foo", "*"));
         assertThat(query, equalTo(expected));
-        context.getMapperService().merge("new_type",
+        context.getMapperService().merge("doc",
             new CompressedXContent(
-                PutMappingRequest.buildFromSimplifiedDef("new_type",
+                PutMappingRequest.buildFromSimplifiedDef("doc",
                     "foo", "type=text",
                     "_field_names", "enabled=true").string()),
             MapperService.MergeReason.MAPPING_UPDATE, true);
diff --git a/core/src/test/java/org/elasticsearch/index/query/SpanTermQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/SpanTermQueryBuilderTests.java
@@ -133,7 +133,7 @@ public void testParseFailsWithMultipleFields() throws IOException {
 
     public void testWithMetaDataField() throws IOException {
         QueryShardContext context = createShardContext();
-        for (String field : new String[]{"_type", "_all"}) {
+        for (String field : new String[]{"field1", "field2"}) {
             SpanTermQueryBuilder spanTermQueryBuilder = new SpanTermQueryBuilder(field, "toto");
             Query query = spanTermQueryBuilder.toQuery(context);
             Query expected = new SpanTermQuery(new Term(field, "toto"));
diff --git a/core/src/test/java/org/elasticsearch/index/query/TypeQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/TypeQueryBuilderTests.java
@@ -19,6 +19,7 @@
 
 package org.elasticsearch.index.query;
 
+import org.apache.lucene.search.MatchAllDocsQuery;
 import org.apache.lucene.search.MatchNoDocsQuery;
 import org.apache.lucene.search.Query;
 import org.apache.lucene.util.BytesRef;
@@ -28,19 +29,27 @@
 
 import java.io.IOException;
 
+import static org.hamcrest.Matchers.anyOf;
+import static org.hamcrest.Matchers.equalTo;
+
+
 public class TypeQueryBuilderTests extends AbstractQueryTestCase<TypeQueryBuilder> {
 
     @Override
     protected TypeQueryBuilder doCreateTestQueryBuilder() {
-        return new TypeQueryBuilder(getRandomType());
+        return new TypeQueryBuilder("doc");
     }
 
     @Override
     protected void doAssertLuceneQuery(TypeQueryBuilder queryBuilder, Query query, SearchContext context) throws IOException {
         if (createShardContext().getMapperService().documentMapper(queryBuilder.type()) == null) {
             assertEquals(new MatchNoDocsQuery(), query);
         } else {
-            assertEquals(new TypeFieldMapper.TypesQuery(new BytesRef(queryBuilder.type())), query);
+            assertThat(query,
+                anyOf(
+                    equalTo(new TypeFieldMapper.TypesQuery(new BytesRef(queryBuilder.type()))),
+                    equalTo(new MatchAllDocsQuery()))
+            );
         }
     }
 
diff --git a/core/src/test/java/org/elasticsearch/index/query/WildcardQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/WildcardQueryBuilderTests.java
@@ -89,7 +89,7 @@ public void testIllegalArguments() {
     public void testEmptyValue() throws IOException {
         QueryShardContext context = createShardContext();
         context.setAllowUnmappedFields(true);
-        WildcardQueryBuilder wildcardQueryBuilder = new WildcardQueryBuilder(getRandomType(), "");
+        WildcardQueryBuilder wildcardQueryBuilder = new WildcardQueryBuilder("doc", "");
         assertEquals(wildcardQueryBuilder.toQuery(context).getClass(), WildcardQuery.class);
     }
 
@@ -129,7 +129,7 @@ public void testParseFailsWithMultipleFields() throws IOException {
 
     public void testWithMetaDataField() throws IOException {
         QueryShardContext context = createShardContext();
-        for (String field : new String[]{"_type", "_all"}) {
+        for (String field : new String[]{"field1", "field2"}) {
             WildcardQueryBuilder wildcardQueryBuilder = new WildcardQueryBuilder(field, "toto");
             Query query = wildcardQueryBuilder.toQuery(context);
             Query expected = new WildcardQuery(new Term(field, "toto"));
diff --git a/core/src/test/java/org/elasticsearch/search/SearchModuleTests.java b/core/src/test/java/org/elasticsearch/search/SearchModuleTests.java
@@ -279,7 +279,6 @@ public List<PipelineAggregationSpec> getPipelineAggregations() {
             "more_like_this",
             "multi_match",
             "nested",
-            "parent_id",
             "prefix",
             "query_string",
             "range",
diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/ParentJoinPlugin.java b/modules/parent-join/src/main/java/org/elasticsearch/join/ParentJoinPlugin.java
@@ -27,6 +27,7 @@
 import org.elasticsearch.join.mapper.ParentJoinFieldMapper;
 import org.elasticsearch.join.query.HasChildQueryBuilder;
 import org.elasticsearch.join.query.HasParentQueryBuilder;
+import org.elasticsearch.join.query.ParentIdQueryBuilder;
 import org.elasticsearch.plugins.MapperPlugin;
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.plugins.SearchPlugin;
@@ -44,7 +45,8 @@ public ParentJoinPlugin(Settings settings) {}
     public List<QuerySpec<?>> getQueries() {
         return Arrays.asList(
             new QuerySpec<>(HasChildQueryBuilder.NAME, HasChildQueryBuilder::new, HasChildQueryBuilder::fromXContent),
-            new QuerySpec<>(HasParentQueryBuilder.NAME, HasParentQueryBuilder::new, HasParentQueryBuilder::fromXContent)
+            new QuerySpec<>(HasParentQueryBuilder.NAME, HasParentQueryBuilder::new, HasParentQueryBuilder::fromXContent),
+            new QuerySpec<>(ParentIdQueryBuilder.NAME, ParentIdQueryBuilder::new, ParentIdQueryBuilder::fromXContent)
         );
     }
 
diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/query/JoinQueryBuilders.java b/modules/parent-join/src/main/java/org/elasticsearch/join/query/JoinQueryBuilders.java
@@ -47,4 +47,11 @@ public static HasParentQueryBuilder hasParentQuery(String type, QueryBuilder que
         return new HasParentQueryBuilder(type, query, score);
     }
 
+    /**
+     * Constructs a new parent id query that returns all child documents of the specified type that
+     * point to the specified id.
+     */
+    public static ParentIdQueryBuilder parentId(String type, String id) {
+        return new ParentIdQueryBuilder(type, id);
+    }
 }
diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/query/ParentIdQueryBuilder.java b/modules/parent-join/src/main/java/org/elasticsearch/join/query/ParentIdQueryBuilder.java
@@ -17,7 +17,7 @@
  * under the License.
  */
 
-package org.elasticsearch.index.query;
+package org.elasticsearch.join.query;
 
 import org.apache.lucene.index.Term;
 import org.apache.lucene.search.BooleanClause;
@@ -35,6 +35,12 @@
 import org.elasticsearch.index.mapper.DocumentMapper;
 import org.elasticsearch.index.mapper.ParentFieldMapper;
 import org.elasticsearch.index.mapper.TypeFieldMapper;
+import org.elasticsearch.index.query.AbstractQueryBuilder;
+import org.elasticsearch.index.query.QueryParseContext;
+import org.elasticsearch.index.query.QueryShardContext;
+import org.elasticsearch.index.query.QueryShardException;
+import org.elasticsearch.join.mapper.ParentIdFieldMapper;
+import org.elasticsearch.join.mapper.ParentJoinFieldMapper;
 
 import java.io.IOException;
 import java.util.Objects;
@@ -155,6 +161,40 @@ public static ParentIdQueryBuilder fromXContent(QueryParseContext parseContext)
 
     @Override
     protected Query doToQuery(QueryShardContext context) throws IOException {
+        if (context.getIndexSettings().isSingleType() == false) {
+            // BWC for indices with multiple types
+            return doToQueryBWC(context);
+        }
+
+        ParentJoinFieldMapper joinFieldMapper = ParentJoinFieldMapper.getMapper(context.getMapperService());
+        if (joinFieldMapper == null) {
+            if (ignoreUnmapped) {
+                return new MatchNoDocsQuery();
+            } else {
+                final String indexName = context.getIndexSettings().getIndex().getName();
+                throw new QueryShardException(context, "[" + NAME + "] no join field found for index [" + indexName  + "]");
+            }
+        }
+        final ParentIdFieldMapper childMapper = joinFieldMapper.getParentIdFieldMapper(type, false);
+        if (childMapper == null) {
+            if (ignoreUnmapped) {
+                return new MatchNoDocsQuery();
+            } else {
+                throw new QueryShardException(context, "[" + NAME + "] no relation found for child [" + type + "]");
+            }
+        }
+        return new BooleanQuery.Builder()
+            .add(childMapper.fieldType().termQuery(id, context), BooleanClause.Occur.MUST)
+            // Need to take child type into account, otherwise a child doc of different type with the same id could match
+            .add(joinFieldMapper.fieldType().termQuery(type, context), BooleanClause.Occur.FILTER)
+            .build();
+    }
+
+    /**
+     * Creates parent_id query from a {@link ParentFieldMapper}
+     * Only used for BWC with multi-types indices
+     */
+    private Query doToQueryBWC(QueryShardContext context) throws IOException {
         DocumentMapper childDocMapper = context.getMapperService().documentMapper(type);
         if (childDocMapper == null) {
             if (ignoreUnmapped) {
@@ -169,11 +209,11 @@ protected Query doToQuery(QueryShardContext context) throws IOException {
         }
         String fieldName = ParentFieldMapper.joinField(parentFieldMapper.type());
 
-        BooleanQuery.Builder query = new BooleanQuery.Builder();
-        query.add(new DocValuesTermsQuery(fieldName, id), BooleanClause.Occur.MUST);
-        // Need to take child type into account, otherwise a child doc of different type with the same id could match
-        query.add(new TermQuery(new Term(TypeFieldMapper.NAME, type)), BooleanClause.Occur.FILTER);
-        return query.build();
+        return new BooleanQuery.Builder()
+            .add(new DocValuesTermsQuery(fieldName, id), BooleanClause.Occur.MUST)
+            // Need to take child type into account, otherwise a child doc of different type with the same id could match
+            .add(new TermQuery(new Term(TypeFieldMapper.NAME, type)), BooleanClause.Occur.FILTER)
+            .build();
     }
 
     @Override
diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/query/ChildQuerySearchIT.java b/modules/parent-join/src/test/java/org/elasticsearch/join/query/ChildQuerySearchIT.java
@@ -77,7 +77,6 @@
 import static org.elasticsearch.index.query.QueryBuilders.idsQuery;
 import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
 import static org.elasticsearch.index.query.QueryBuilders.matchQuery;
-import static org.elasticsearch.index.query.QueryBuilders.parentId;
 import static org.elasticsearch.index.query.QueryBuilders.prefixQuery;
 import static org.elasticsearch.index.query.QueryBuilders.queryStringQuery;
 import static org.elasticsearch.index.query.QueryBuilders.termQuery;
@@ -86,6 +85,7 @@
 import static org.elasticsearch.index.query.functionscore.ScoreFunctionBuilders.weightFactorFunction;
 import static org.elasticsearch.join.query.JoinQueryBuilders.hasChildQuery;
 import static org.elasticsearch.join.query.JoinQueryBuilders.hasParentQuery;
+import static org.elasticsearch.join.query.JoinQueryBuilders.parentId;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures;
@@ -1262,27 +1262,31 @@ public void testParentFieldQuery() throws Exception {
     }
 
     public void testParentIdQuery() throws Exception {
-        if (legacy() == false) {
-            // Fix parent_id query
-            return;
+        if (legacy()) {
+            assertAcked(prepareCreate("test")
+                .setSettings(Settings.builder()
+                    .put(indexSettings())
+                    .put("index.refresh_interval", -1)
+                )
+                .addMapping("parent")
+                .addMapping("child", "_parent", "type=parent"));
+        } else {
+            assertAcked(prepareCreate("test")
+                .setSettings(Settings.builder()
+                    .put(indexSettings())
+                    .put("index.refresh_interval", -1)
+                )
+                .addMapping("doc", "join_field", "type=join,parent=child"));
         }
-
-        assertAcked(prepareCreate("test")
-            .setSettings(Settings.builder()
-                .put(indexSettings())
-                .put("index.refresh_interval", -1)
-            )
-            .addMapping("parent")
-            .addMapping("child", "_parent", "type=parent"));
         ensureGreen();
 
-        client().prepareIndex("test", "child", "c1").setSource("{}", XContentType.JSON).setParent("p1").get();
+        createIndexRequest("test", "child", "c1", "p1").get();
         refresh();
 
         SearchResponse response = client().prepareSearch("test").setQuery(parentId("child", "p1")).get();
         assertHitCount(response, 1L);
 
-        client().prepareIndex("test", "child", "c2").setSource("{}", XContentType.JSON).setParent("p2").get();
+        createIndexRequest("test", "child", "c2", "p2").get();
         refresh();
 
         response = client().prepareSearch("test")
diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/query/HasChildQueryBuilderTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/query/HasChildQueryBuilderTests.java
@@ -84,6 +84,14 @@ protected Collection<Class<? extends Plugin>> getPlugins() {
         return Collections.singletonList(ParentJoinPlugin.class);
     }
 
+    @Override
+    protected Settings indexSettings() {
+        return Settings.builder()
+            .put(super.indexSettings())
+            .put("index.mapping.single_type", false)
+            .build();
+    }
+
     @Override
     protected void initializeAdditionalMappings(MapperService mapperService) throws IOException {
         similarity = randomFrom("classic", "BM25");
diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/query/HasParentQueryBuilderTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/query/HasParentQueryBuilderTests.java
@@ -25,6 +25,7 @@
 import org.elasticsearch.Version;
 import org.elasticsearch.action.admin.indices.mapping.put.PutMappingRequest;
 import org.elasticsearch.common.compress.CompressedXContent;
+import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
@@ -70,6 +71,14 @@ protected Collection<Class<? extends Plugin>> getPlugins() {
         return Collections.singletonList(ParentJoinPlugin.class);
     }
 
+    @Override
+    protected Settings indexSettings() {
+        return Settings.builder()
+            .put(super.indexSettings())
+            .put("index.mapping.single_type", false)
+            .build();
+    }
+
     @Override
     protected void initializeAdditionalMappings(MapperService mapperService) throws IOException {
         // TODO: use a single type when inner hits have been changed to work with join field,
diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/query/LegacyParentIdQueryBuilderTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/query/LegacyParentIdQueryBuilderTests.java
diff --git a/modules/parent-join/src/test/java/org/elasticsearch/join/query/ParentIdQueryBuilderTests.java b/modules/parent-join/src/test/java/org/elasticsearch/join/query/ParentIdQueryBuilderTests.java
diff --git a/modules/parent-join/src/test/resources/rest-api-spec/test/20_parent_join.yml b/modules/parent-join/src/test/resources/rest-api-spec/test/20_parent_join.yml
diff --git a/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolateQueryBuilderTests.java b/modules/percolator/src/test/java/org/elasticsearch/percolator/PercolateQueryBuilderTests.java
diff --git a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java

Original file line number	Diff line number	Diff line change
`@@ -471,14 +471,6 @@ public static MoreLikeThisQueryBuilder moreLikeThisQuery(Item[] likeItems) {`
`471`	`471`	`return moreLikeThisQuery(null, null, likeItems);`
`472`	`472`	`}`
`473`	`473`
`474`		`- /**`
`475`		`- * Constructs a new parent id query that returns all child documents of the specified type that`
`476`		`- * point to the specified id.`
`477`		`- */`
`478`		`- public static ParentIdQueryBuilder parentId(String type, String id) {`
`479`		`- return new ParentIdQueryBuilder(type, id);`
`480`		`- }`
`481`		`-`
`482`	`474`	`public static NestedQueryBuilder nestedQuery(String path, QueryBuilder query, ScoreMode scoreMode) {`
`483`	`475`	`return new NestedQueryBuilder(path, query, scoreMode);`
`484`	`476`	`}`
Original file line number	Diff line number	Diff line change
`@@ -430,7 +430,7 @@ public void testExceptionUsingAnalyzerOnNumericField() {`
`430`	`430`
`431`	`431`	`@Override`
`432`	`432`	`protected void initializeAdditionalMappings(MapperService mapperService) throws IOException {`
`433`		`- mapperService.merge("t_boost", new CompressedXContent(PutMappingRequest.buildFromSimplifiedDef("t_boost",`
	`433`	`+ mapperService.merge("doc", new CompressedXContent(PutMappingRequest.buildFromSimplifiedDef("doc",`
`434`	`434`	`"string_boost", "type=text,boost=4").string()), MapperService.MergeReason.MAPPING_UPDATE, false);`
`435`	`435`	`}`
`436`	`436`