Skip to content

Commit

Permalink
Delay the creation of SubSearchContext to the FetchSubPhase (elastic#…
Browse files Browse the repository at this point in the history 
…46598)

This change delays the creation of the SubSearchContext for nested and parent/child inner_hits
to the fetch sub phase in order to ensure that a SearchContext can built entirely from a
QueryShardContext. This commit also adds a validation step to the inner hits builder that ensures that we fail the request early if the inner hits path is invalid.

Relates elastic#46523
  • Loading branch information
@jimczi
jimczi authored Sep 12, 2019
1 parent a84908c commit 328fe44
Show file tree
Hide file tree
Showing 14 changed files with 141 additions and 118 deletions.
51 changes: 30 additions & 21 deletions ...nt-join/src/main/java/org/elasticsearch/join/query/ParentChildInnerHitContextBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,29 +67,38 @@ class ParentChildInnerHitContextBuilder extends InnerHitContextBuilder {
}

@Override
protected void doBuild(SearchContext context, InnerHitsContext innerHitsContext) throws IOException {
public void doValidate(QueryShardContext queryShardContext) {
if (ParentJoinFieldMapper.getMapper(queryShardContext.getMapperService()) == null
&& innerHitBuilder.isIgnoreUnmapped() == false) {
throw new IllegalStateException("no join field has been configured");
}
}

@Override
public void build(SearchContext context, InnerHitsContext innerHitsContext) throws IOException {
QueryShardContext queryShardContext = context.getQueryShardContext();
ParentJoinFieldMapper joinFieldMapper = ParentJoinFieldMapper.getMapper(context.mapperService());
if (joinFieldMapper != null) {
String name = innerHitBuilder.getName() != null ? innerHitBuilder.getName() : typeName;
JoinFieldInnerHitSubContext joinFieldInnerHits = new JoinFieldInnerHitSubContext(name, context, typeName,
fetchChildInnerHits, joinFieldMapper);
setupInnerHitsContext(queryShardContext, joinFieldInnerHits);
innerHitsContext.addInnerHitDefinition(joinFieldInnerHits);
} else {
if (innerHitBuilder.isIgnoreUnmapped() == false) {
throw new IllegalStateException("no join field has been configured");
}
if (joinFieldMapper == null) {
assert innerHitBuilder.isIgnoreUnmapped() : "should be validated first";
return;
}
String name = innerHitBuilder.getName() != null ? innerHitBuilder.getName() : typeName;
JoinFieldInnerHitSubContext joinFieldInnerHits =
new JoinFieldInnerHitSubContext(name, context, typeName, fetchChildInnerHits, joinFieldMapper);
setupInnerHitsContext(queryShardContext, joinFieldInnerHits);
innerHitsContext.addInnerHitDefinition(joinFieldInnerHits);
}

static final class JoinFieldInnerHitSubContext extends InnerHitsContext.InnerHitSubContext {
private final String typeName;
private final boolean fetchChildInnerHits;
private final ParentJoinFieldMapper joinFieldMapper;

JoinFieldInnerHitSubContext(String name, SearchContext context, String typeName, boolean fetchChildInnerHits,
ParentJoinFieldMapper joinFieldMapper) {
JoinFieldInnerHitSubContext(String name,
SearchContext context,
String typeName,
boolean fetchChildInnerHits,
ParentJoinFieldMapper joinFieldMapper) {
super(name, context);
this.typeName = typeName;
this.fetchChildInnerHits = fetchChildInnerHits;
Expand All @@ -102,13 +111,13 @@ public TopDocsAndMaxScore[] topDocs(SearchHit[] hits) throws IOException {
TopDocsAndMaxScore[] result = new TopDocsAndMaxScore[hits.length];
for (int i = 0; i < hits.length; i++) {
SearchHit hit = hits[i];
String joinName = getSortedDocValue(joinFieldMapper.name(), context, hit.docId());
String joinName = getSortedDocValue(joinFieldMapper.name(), this, hit.docId());
if (joinName == null) {
result[i] = new TopDocsAndMaxScore(Lucene.EMPTY_TOP_DOCS, Float.NaN);
continue;
}

QueryShardContext qsc = context.getQueryShardContext();
QueryShardContext qsc = getQueryShardContext();
ParentIdFieldMapper parentIdFieldMapper =
joinFieldMapper.getParentIdFieldMapper(typeName, fetchChildInnerHits == false);
if (parentIdFieldMapper == null) {
Expand All @@ -126,14 +135,14 @@ public TopDocsAndMaxScore[] topDocs(SearchHit[] hits) throws IOException {
.add(joinFieldMapper.fieldType().termQuery(typeName, qsc), BooleanClause.Occur.FILTER)
.build();
} else {
String parentId = getSortedDocValue(parentIdFieldMapper.name(), context, hit.docId());
q = context.mapperService().fullName(IdFieldMapper.NAME).termQuery(parentId, qsc);
String parentId = getSortedDocValue(parentIdFieldMapper.name(), this, hit.docId());
q = mapperService().fullName(IdFieldMapper.NAME).termQuery(parentId, qsc);
}

Weight weight = context.searcher().createWeight(context.searcher().rewrite(q), ScoreMode.COMPLETE_NO_SCORES, 1f);
Weight weight = searcher().createWeight(searcher().rewrite(q), ScoreMode.COMPLETE_NO_SCORES, 1f);
if (size() == 0) {
TotalHitCountCollector totalHitCountCollector = new TotalHitCountCollector();
for (LeafReaderContext ctx : context.searcher().getIndexReader().leaves()) {
for (LeafReaderContext ctx : searcher().getIndexReader().leaves()) {
intersect(weight, innerHitQueryWeight, totalHitCountCollector, ctx);
}
result[i] = new TopDocsAndMaxScore(
Expand All @@ -142,7 +151,7 @@ public TopDocsAndMaxScore[] topDocs(SearchHit[] hits) throws IOException {
Lucene.EMPTY_SCORE_DOCS
), Float.NaN);
} else {
int topN = Math.min(from() + size(), context.searcher().getIndexReader().maxDoc());
int topN = Math.min(from() + size(), searcher().getIndexReader().maxDoc());
TopDocsCollector<?> topDocsCollector;
MaxScoreCollector maxScoreCollector = null;
if (sort() != null) {
Expand All @@ -155,7 +164,7 @@ public TopDocsAndMaxScore[] topDocs(SearchHit[] hits) throws IOException {
maxScoreCollector = new MaxScoreCollector();
}
try {
for (LeafReaderContext ctx : context.searcher().getIndexReader().leaves()) {
for (LeafReaderContext ctx : searcher().getIndexReader().leaves()) {
intersect(weight, innerHitQueryWeight, MultiCollector.wrap(topDocsCollector, maxScoreCollector), ctx);
}
} finally {
Expand Down
11 changes: 5 additions & 6 deletions ...les/parent-join/src/test/java/org/elasticsearch/join/query/HasChildQueryBuilderTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,14 +177,13 @@ protected void doAssertLuceneQuery(HasChildQueryBuilder queryBuilder, Query quer
queryBuilder = (HasChildQueryBuilder) queryBuilder.rewrite(searchContext.getQueryShardContext());
Map<String, InnerHitContextBuilder> innerHitBuilders = new HashMap<>();
InnerHitContextBuilder.extractInnerHits(queryBuilder, innerHitBuilders);
final InnerHitsContext innerHitsContext = new InnerHitsContext();
for (InnerHitContextBuilder builder : innerHitBuilders.values()) {
builder.build(searchContext, searchContext.innerHits());
builder.build(searchContext, innerHitsContext);
}
assertNotNull(searchContext.innerHits());
assertEquals(1, searchContext.innerHits().getInnerHits().size());
assertTrue(searchContext.innerHits().getInnerHits().containsKey(queryBuilder.innerHit().getName()));
InnerHitsContext.InnerHitSubContext innerHits =
searchContext.innerHits().getInnerHits().get(queryBuilder.innerHit().getName());
assertEquals(1, innerHitsContext.getInnerHits().size());
assertTrue(innerHitsContext.getInnerHits().containsKey(queryBuilder.innerHit().getName()));
InnerHitsContext.InnerHitSubContext innerHits = innerHitsContext.getInnerHits().get(queryBuilder.innerHit().getName());
assertEquals(innerHits.size(), queryBuilder.innerHit().getSize());
assertEquals(innerHits.sort().sort.getSort().length, 1);
assertEquals(innerHits.sort().sort.getSort()[0].getField(), STRING_FIELD_NAME_2);
Expand Down
12 changes: 5 additions & 7 deletions ...es/parent-join/src/test/java/org/elasticsearch/join/query/HasParentQueryBuilderTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,17 +148,15 @@ protected void doAssertLuceneQuery(HasParentQueryBuilder queryBuilder, Query que
// doCreateTestQueryBuilder)
queryBuilder = (HasParentQueryBuilder) queryBuilder.rewrite(searchContext.getQueryShardContext());

assertNotNull(searchContext);
Map<String, InnerHitContextBuilder> innerHitBuilders = new HashMap<>();
InnerHitContextBuilder.extractInnerHits(queryBuilder, innerHitBuilders);
final InnerHitsContext innerHitsContext = new InnerHitsContext();
for (InnerHitContextBuilder builder : innerHitBuilders.values()) {
builder.build(searchContext, searchContext.innerHits());
builder.build(searchContext, innerHitsContext);
}
assertNotNull(searchContext.innerHits());
assertEquals(1, searchContext.innerHits().getInnerHits().size());
assertTrue(searchContext.innerHits().getInnerHits().containsKey(queryBuilder.innerHit().getName()));
InnerHitsContext.InnerHitSubContext innerHits = searchContext.innerHits()
.getInnerHits().get(queryBuilder.innerHit().getName());
assertEquals(1, innerHitsContext.getInnerHits().size());
assertTrue(innerHitsContext.getInnerHits().containsKey(queryBuilder.innerHit().getName()));
InnerHitsContext.InnerHitSubContext innerHits = innerHitsContext.getInnerHits().get(queryBuilder.innerHit().getName());
assertEquals(innerHits.size(), queryBuilder.innerHit().getSize());
assertEquals(innerHits.sort().sort.getSort().length, 1);
assertEquals(innerHits.sort().sort.getSort()[0].getField(), STRING_FIELD_NAME_2);
Expand Down
30 changes: 7 additions & 23 deletions server/src/main/java/org/elasticsearch/index/query/InnerHitContextBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import org.elasticsearch.search.sort.SortBuilder;

import java.io.IOException;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;

Expand All @@ -47,9 +46,9 @@ protected InnerHitContextBuilder(QueryBuilder query, InnerHitBuilder innerHitBui
this.query = query;
}

public final void build(SearchContext parentSearchContext, InnerHitsContext innerHitsContext) throws IOException {
public final void validate(QueryShardContext queryShardContext) {
long innerResultWindow = innerHitBuilder.getFrom() + innerHitBuilder.getSize();
int maxInnerResultWindow = parentSearchContext.mapperService().getIndexSettings().getMaxInnerResultWindow();
int maxInnerResultWindow = queryShardContext.getIndexSettings().getMaxInnerResultWindow();
if (innerResultWindow > maxInnerResultWindow) {
throw new IllegalArgumentException(
"Inner result window is too large, the inner hit definition's [" + innerHitBuilder.getName() +
Expand All @@ -58,10 +57,12 @@ public final void build(SearchContext parentSearchContext, InnerHitsContext inne
"] index level setting."
);
}
doBuild(parentSearchContext, innerHitsContext);
doValidate(queryShardContext);
}

protected abstract void doBuild(SearchContext parentSearchContext, InnerHitsContext innerHitsContext) throws IOException;
protected abstract void doValidate(QueryShardContext queryShardContext);

public abstract void build(SearchContext parentSearchContext, InnerHitsContext innerHitsContext) throws IOException;

public static void extractInnerHits(QueryBuilder query, Map<String, InnerHitContextBuilder> innerHitBuilders) {
if (query instanceof AbstractQueryBuilder) {
Expand Down Expand Up @@ -109,23 +110,6 @@ protected void setupInnerHitsContext(QueryShardContext queryShardContext,
}
ParsedQuery parsedQuery = new ParsedQuery(query.toQuery(queryShardContext), queryShardContext.copyNamedQueries());
innerHitsContext.parsedQuery(parsedQuery);
Map<String, InnerHitsContext.InnerHitSubContext> baseChildren =
buildChildInnerHits(innerHitsContext.parentSearchContext(), children);
innerHitsContext.setChildInnerHits(baseChildren);
}

private static Map<String, InnerHitsContext.InnerHitSubContext> buildChildInnerHits(SearchContext parentSearchContext,
Map<String, InnerHitContextBuilder> children) throws IOException {

Map<String, InnerHitsContext.InnerHitSubContext> childrenInnerHits = new HashMap<>();
for (Map.Entry<String, InnerHitContextBuilder> entry : children.entrySet()) {
InnerHitsContext childInnerHitsContext = new InnerHitsContext();
entry.getValue().build(
parentSearchContext, childInnerHitsContext);
if (childInnerHitsContext.getInnerHits() != null) {
childrenInnerHits.putAll(childInnerHitsContext.getInnerHits());
}
}
return childrenInnerHits;
innerHitsContext.innerHits(children);
}
}
34 changes: 20 additions & 14 deletions server/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -332,28 +332,34 @@ public void extractInnerHitBuilders(Map<String, InnerHitContextBuilder> innerHit
static class NestedInnerHitContextBuilder extends InnerHitContextBuilder {
private final String path;

NestedInnerHitContextBuilder(String path, QueryBuilder query, InnerHitBuilder innerHitBuilder,
Map<String, InnerHitContextBuilder> children) {
NestedInnerHitContextBuilder(String path,
QueryBuilder query,
InnerHitBuilder innerHitBuilder,
Map<String, InnerHitContextBuilder> children) {
super(query, innerHitBuilder, children);
this.path = path;
}

@Override
protected void doBuild(SearchContext parentSearchContext,
InnerHitsContext innerHitsContext) throws IOException {
QueryShardContext queryShardContext = parentSearchContext.getQueryShardContext();
public void doValidate(QueryShardContext queryShardContext) {
if (queryShardContext.getObjectMapper(path) == null
&& innerHitBuilder.isIgnoreUnmapped() == false) {
throw new IllegalStateException("[" + query.getName() + "] no mapping found for type [" + path + "]");
}
}

@Override
public void build(SearchContext searchContext, InnerHitsContext innerHitsContext) throws IOException {
QueryShardContext queryShardContext = searchContext.getQueryShardContext();
ObjectMapper nestedObjectMapper = queryShardContext.getObjectMapper(path);
if (nestedObjectMapper == null) {
if (innerHitBuilder.isIgnoreUnmapped() == false) {
throw new IllegalStateException("[" + query.getName() + "] no mapping found for type [" + path + "]");
} else {
return;
}
assert innerHitBuilder.isIgnoreUnmapped() : "should be validated first";
return;
}
String name = innerHitBuilder.getName() != null ? innerHitBuilder.getName() : nestedObjectMapper.fullPath();
ObjectMapper parentObjectMapper = queryShardContext.nestedScope().nextLevel(nestedObjectMapper);
NestedInnerHitSubContext nestedInnerHits = new NestedInnerHitSubContext(
name, parentSearchContext, parentObjectMapper, nestedObjectMapper
name, searchContext, parentObjectMapper, nestedObjectMapper
);
setupInnerHitsContext(queryShardContext, nestedInnerHits);
queryShardContext.nestedScope().previousLevel();
Expand Down Expand Up @@ -399,17 +405,17 @@ public TopDocsAndMaxScore[] topDocs(SearchHit[] hits) throws IOException {
LeafReaderContext ctx = searcher().getIndexReader().leaves().get(readerIndex);

Query childFilter = childObjectMapper.nestedTypeFilter();
BitSetProducer parentFilter = context.bitsetFilterCache().getBitSetProducer(rawParentFilter);
BitSetProducer parentFilter = bitsetFilterCache().getBitSetProducer(rawParentFilter);
Query q = new ParentChildrenBlockJoinQuery(parentFilter, childFilter, parentDocId);
Weight weight = context.searcher().createWeight(context.searcher().rewrite(q),
Weight weight = searcher().createWeight(searcher().rewrite(q),
org.apache.lucene.search.ScoreMode.COMPLETE_NO_SCORES, 1f);
if (size() == 0) {
TotalHitCountCollector totalHitCountCollector = new TotalHitCountCollector();
intersect(weight, innerHitQueryWeight, totalHitCountCollector, ctx);
result[i] = new TopDocsAndMaxScore(new TopDocs(new TotalHits(totalHitCountCollector.getTotalHits(),
TotalHits.Relation.EQUAL_TO), Lucene.EMPTY_SCORE_DOCS), Float.NaN);
} else {
int topN = Math.min(from() + size(), context.searcher().getIndexReader().maxDoc());
int topN = Math.min(from() + size(), searcher().getIndexReader().maxDoc());
TopDocsCollector<?> topDocsCollector;
MaxScoreCollector maxScoreCollector = null;
if (sort() != null) {
Expand Down
12 changes: 12 additions & 0 deletions server/src/main/java/org/elasticsearch/search/DefaultSearchContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.mapper.ObjectMapper;
import org.elasticsearch.index.query.AbstractQueryBuilder;
import org.elasticsearch.index.query.InnerHitContextBuilder;
import org.elasticsearch.index.query.ParsedQuery;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.index.query.QueryShardContext;
Expand Down Expand Up @@ -110,6 +111,7 @@ final class DefaultSearchContext extends SearchContext {
private ScriptFieldsContext scriptFields;
private FetchSourceContext fetchSourceContext;
private DocValueFieldsContext docValueFieldsContext;
private Map<String, InnerHitContextBuilder> innerHits = Collections.emptyMap();
private int from = -1;
private int size = -1;
private SortAndFormats sort;
Expand Down Expand Up @@ -377,6 +379,16 @@ public void highlight(SearchContextHighlight highlight) {
this.highlight = highlight;
}

@Override
public void innerHits(Map<String, InnerHitContextBuilder> innerHits) {
this.innerHits = innerHits;
}

@Override
public Map<String, InnerHitContextBuilder> innerHits() {
return innerHits;
}

@Override
public SuggestionSearchContext suggest() {
return suggest;
Expand Down
7 changes: 2 additions & 5 deletions server/src/main/java/org/elasticsearch/search/SearchService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,7 @@ private void parseSource(DefaultSearchContext context, SearchSourceBuilder sourc
context.from(source.from());
context.size(source.size());
Map<String, InnerHitContextBuilder> innerHitBuilders = new HashMap<>();
context.innerHits(innerHitBuilders);
if (source.query() != null) {
InnerHitContextBuilder.extractInnerHits(source.query(), innerHitBuilders);
context.parsedQuery(queryShardContext.toQuery(source.query()));
Expand All @@ -749,11 +750,7 @@ private void parseSource(DefaultSearchContext context, SearchSourceBuilder sourc
}
if (innerHitBuilders.size() > 0) {
for (Map.Entry<String, InnerHitContextBuilder> entry : innerHitBuilders.entrySet()) {
try {
entry.getValue().build(context, context.innerHits());
} catch (IOException e) {
throw new SearchContextException(context, "failed to build inner_hits", e);
}
entry.getValue().validate(queryShardContext);
}
}
if (source.sorts() != null) {
Expand Down
Loading

0 comments on commit 328fe44

Please sign in to comment.