Skip to content

Commit

Permalink
Move parent_id query to the parent-join module (elastic#25072)
Browse files Browse the repository at this point in the history
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 elastic#20257
  • Loading branch information
jimczi committed Jun 7, 2017
1 parent 468dc7a commit e69ca2e
Show file tree
Hide file tree
Showing 21 changed files with 361 additions and 98 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
2 changes: 0 additions & 2 deletions core/src/main/java/org/elasticsearch/search/SearchModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,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;
Expand Down Expand Up @@ -749,7 +748,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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,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()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ protected void initializeAdditionalMappings(MapperService mapperService) throws
String geoFieldMapping = (mapperService.getIndexSettings().getIndexVersionCreated()
.before(LatLonPointFieldMapper.LAT_LON_FIELD_VERSION)) ?
LEGACY_GEO_POINT_FIELD_MAPPING : "type=geo_point";
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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -894,19 +894,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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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()))
);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,6 @@ public List<PipelineAggregationSpec> getPipelineAggregations() {
"more_like_this",
"multi_match",
"nested",
"parent_id",
"prefix",
"query_string",
"range",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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)
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -156,6 +162,40 @@ public static Optional<ParentIdQueryBuilder> fromXContent(QueryParseContext pars

@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) {
Expand All @@ -170,11 +210,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -1261,27 +1261,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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -74,6 +75,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,
Expand Down
Loading

0 comments on commit e69ca2e

Please sign in to comment.