Skip to content

Commit

Permalink
percolator: fix handling of nested documents
Browse files Browse the repository at this point in the history 
Nested documents were indexed as separate documents, but it was never checked
if the hits represent nested documents or not. Therefore, nested objects could
match not nested queries and nested queries could also match not nested documents.

Examples are in issue elastic#6540 .

closes elastic#6540
closes elastic#6544
  • Loading branch information
@brwe
brwe committed Jun 18, 2014
1 parent 69350dc commit fe89ea1
Show file tree
Hide file tree
Showing 3 changed files with 203 additions and 37 deletions.
41 changes: 25 additions & 16 deletions src/main/java/org/elasticsearch/percolator/PercolatorService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@
import org.elasticsearch.index.mapper.internal.IdFieldMapper;
import org.elasticsearch.index.percolator.stats.ShardPercolateService;
import org.elasticsearch.index.query.ParsedQuery;
import org.elasticsearch.index.search.nested.NonNestedDocsFilter;
import org.elasticsearch.index.service.IndexService;
import org.elasticsearch.index.shard.service.IndexShard;
import org.elasticsearch.indices.IndicesService;
Expand Down Expand Up @@ -210,8 +211,9 @@ public PercolateShardResponse percolate(PercolateShardRequest request) {

// parse the source either into one MemoryIndex, if it is a single document or index multiple docs if nested
PercolatorIndex percolatorIndex;
boolean isNested = indexShard.mapperService().documentMapper(request.documentType()).hasNestedObjects();
if (parsedDocument.docs().size() > 1) {
assert indexShard.mapperService().documentMapper(request.documentType()).hasNestedObjects();
assert isNested;
percolatorIndex = multi;
} else {
percolatorIndex = single;
Expand All @@ -232,7 +234,7 @@ public PercolateShardResponse percolate(PercolateShardRequest request) {
context.percolatorTypeId = action.id();

percolatorIndex.prepare(context, parsedDocument);
return action.doPercolate(request, context);
return action.doPercolate(request, context, isNested);
} finally {
context.close();
shardPercolateService.postPercolate(System.nanoTime() - startTime);
Expand Down Expand Up @@ -418,7 +420,7 @@ interface PercolatorType {

ReduceResult reduce(List<PercolateShardResponse> shardResults);

PercolateShardResponse doPercolate(PercolateShardRequest request, PercolateContext context);
PercolateShardResponse doPercolate(PercolateShardRequest request, PercolateContext context, boolean isNested);

}

Expand All @@ -443,13 +445,17 @@ public ReduceResult reduce(List<PercolateShardResponse> shardResults) {
}

@Override
public PercolateShardResponse doPercolate(PercolateShardRequest request, PercolateContext context) {
public PercolateShardResponse doPercolate(PercolateShardRequest request, PercolateContext context, boolean isNested) {
long count = 0;
Lucene.ExistsCollector collector = new Lucene.ExistsCollector();
for (Map.Entry<BytesRef, Query> entry : context.percolateQueries().entrySet()) {
collector.reset();
try {
context.docSearcher().search(entry.getValue(), collector);
if (isNested) {
context.docSearcher().search(entry.getValue(), NonNestedDocsFilter.INSTANCE, collector);
} else {
context.docSearcher().search(entry.getValue(), collector);
}
} catch (Throwable e) {
logger.debug("[" + entry.getKey() + "] failed to execute query", e);
throw new PercolateException(context.indexShard().shardId(), "failed to execute", e);
Expand Down Expand Up @@ -477,11 +483,11 @@ public ReduceResult reduce(List<PercolateShardResponse> shardResults) {
}

@Override
public PercolateShardResponse doPercolate(PercolateShardRequest request, PercolateContext context) {
public PercolateShardResponse doPercolate(PercolateShardRequest request, PercolateContext context, boolean isNested) {
long count = 0;
Engine.Searcher percolatorSearcher = context.indexShard().acquireSearcher("percolate");
try {
Count countCollector = count(logger, context);
Count countCollector = count(logger, context, isNested);
queryBasedPercolating(percolatorSearcher, context, countCollector);
count = countCollector.counter();
} catch (Throwable e) {
Expand Down Expand Up @@ -534,7 +540,7 @@ public ReduceResult reduce(List<PercolateShardResponse> shardResults) {
}

@Override
public PercolateShardResponse doPercolate(PercolateShardRequest request, PercolateContext context) {
public PercolateShardResponse doPercolate(PercolateShardRequest request, PercolateContext context, boolean isNested) {
long count = 0;
List<BytesRef> matches = new ArrayList<>();
List<Map<String, HighlightField>> hls = new ArrayList<>();
Expand All @@ -547,7 +553,11 @@ public PercolateShardResponse doPercolate(PercolateShardRequest request, Percola
context.hitContext().cache().clear();
}
try {
context.docSearcher().search(entry.getValue(), collector);
if (isNested) {
context.docSearcher().search(entry.getValue(), NonNestedDocsFilter.INSTANCE, collector);
} else {
context.docSearcher().search(entry.getValue(), collector);
}
} catch (Throwable e) {
logger.debug("[" + entry.getKey() + "] failed to execute query", e);
throw new PercolateException(context.indexShard().shardId(), "failed to execute", e);
Expand Down Expand Up @@ -583,10 +593,10 @@ public ReduceResult reduce(List<PercolateShardResponse> shardResults) {
}

@Override
public PercolateShardResponse doPercolate(PercolateShardRequest request, PercolateContext context) {
public PercolateShardResponse doPercolate(PercolateShardRequest request, PercolateContext context, boolean isNested) {
Engine.Searcher percolatorSearcher = context.indexShard().acquireSearcher("percolate");
try {
Match match = match(logger, context, highlightPhase);
Match match = match(logger, context, highlightPhase, isNested);
queryBasedPercolating(percolatorSearcher, context, match);
List<BytesRef> matches = match.matches();
List<Map<String, HighlightField>> hls = match.hls();
Expand Down Expand Up @@ -616,10 +626,10 @@ public ReduceResult reduce(List<PercolateShardResponse> shardResults) {
}

@Override
public PercolateShardResponse doPercolate(PercolateShardRequest request, PercolateContext context) {
public PercolateShardResponse doPercolate(PercolateShardRequest request, PercolateContext context, boolean isNested) {
Engine.Searcher percolatorSearcher = context.indexShard().acquireSearcher("percolate");
try {
MatchAndScore matchAndScore = matchAndScore(logger, context, highlightPhase);
MatchAndScore matchAndScore = matchAndScore(logger, context, highlightPhase, isNested);
queryBasedPercolating(percolatorSearcher, context, matchAndScore);
List<BytesRef> matches = matchAndScore.matches();
List<Map<String, HighlightField>> hls = matchAndScore.hls();
Expand Down Expand Up @@ -730,10 +740,10 @@ public ReduceResult reduce(List<PercolateShardResponse> shardResults) {
}

@Override
public PercolateShardResponse doPercolate(PercolateShardRequest request, PercolateContext context) {
public PercolateShardResponse doPercolate(PercolateShardRequest request, PercolateContext context, boolean isNested) {
Engine.Searcher percolatorSearcher = context.indexShard().acquireSearcher("percolate");
try {
MatchAndSort matchAndSort = QueryCollector.matchAndSort(logger, context);
MatchAndSort matchAndSort = QueryCollector.matchAndSort(logger, context, isNested);
queryBasedPercolating(percolatorSearcher, context, matchAndSort);
TopDocs topDocs = matchAndSort.topDocs();
long count = topDocs.totalHits;
Expand Down Expand Up @@ -785,7 +795,6 @@ private void queryBasedPercolating(Engine.Searcher percolatorSearcher, Percolate
percolatorTypeFilter = context.indexService().cache().filter().cache(percolatorTypeFilter);
XFilteredQuery query = new XFilteredQuery(context.percolateQuery(), percolatorTypeFilter);
percolatorSearcher.searcher().search(query, percolateCollector);

for (Collector queryCollector : percolateCollector.facetAndAggregatorCollector) {
if (queryCollector instanceof XCollector) {
((XCollector) queryCollector).postCollection();
Expand Down
61 changes: 40 additions & 21 deletions src/main/java/org/elasticsearch/percolator/QueryCollector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.elasticsearch.index.mapper.FieldMapper;
import org.elasticsearch.index.mapper.internal.IdFieldMapper;
import org.elasticsearch.index.query.ParsedQuery;
import org.elasticsearch.index.search.nested.NonNestedDocsFilter;
import org.elasticsearch.search.aggregations.AggregationPhase;
import org.elasticsearch.search.aggregations.Aggregator;
import org.elasticsearch.search.aggregations.bucket.global.GlobalAggregator;
Expand All @@ -55,6 +56,7 @@ abstract class QueryCollector extends Collector {
final IndexSearcher searcher;
final ConcurrentMap<BytesRef, Query> queries;
final ESLogger logger;
boolean isNestedDoc = false;

final Lucene.ExistsCollector collector = new Lucene.ExistsCollector();
BytesRef current;
Expand All @@ -63,12 +65,13 @@ abstract class QueryCollector extends Collector {

final List<Collector> facetAndAggregatorCollector;

QueryCollector(ESLogger logger, PercolateContext context) {
QueryCollector(ESLogger logger, PercolateContext context, boolean isNestedDoc) {
this.logger = logger;
this.queries = context.percolateQueries();
this.searcher = context.docSearcher();
final FieldMapper<?> idMapper = context.mapperService().smartNameFieldMapper(IdFieldMapper.NAME);
this.idFieldData = context.fieldData().getForField(idMapper);
this.isNestedDoc = isNestedDoc;

ImmutableList.Builder<Collector> facetAggCollectorBuilder = ImmutableList.builder();
if (context.facets() != null) {
Expand Down Expand Up @@ -139,20 +142,20 @@ public boolean acceptsDocsOutOfOrder() {
}


static Match match(ESLogger logger, PercolateContext context, HighlightPhase highlightPhase) {
return new Match(logger, context, highlightPhase);
static Match match(ESLogger logger, PercolateContext context, HighlightPhase highlightPhase, boolean isNestedDoc) {
return new Match(logger, context, highlightPhase, isNestedDoc);
}

static Count count(ESLogger logger, PercolateContext context) {
return new Count(logger, context);
static Count count(ESLogger logger, PercolateContext context, boolean isNestedDoc) {
return new Count(logger, context, isNestedDoc);
}

static MatchAndScore matchAndScore(ESLogger logger, PercolateContext context, HighlightPhase highlightPhase) {
return new MatchAndScore(logger, context, highlightPhase);
static MatchAndScore matchAndScore(ESLogger logger, PercolateContext context, HighlightPhase highlightPhase, boolean isNestedDoc) {
return new MatchAndScore(logger, context, highlightPhase, isNestedDoc);
}

static MatchAndSort matchAndSort(ESLogger logger, PercolateContext context) {
return new MatchAndSort(logger, context);
static MatchAndSort matchAndSort(ESLogger logger, PercolateContext context, boolean isNestedDoc) {
return new MatchAndSort(logger, context, isNestedDoc);
}


Expand All @@ -179,8 +182,8 @@ final static class Match extends QueryCollector {
final int size;
long counter = 0;

Match(ESLogger logger, PercolateContext context, HighlightPhase highlightPhase) {
super(logger, context);
Match(ESLogger logger, PercolateContext context, HighlightPhase highlightPhase, boolean isNestedDoc) {
super(logger, context, isNestedDoc);
this.limit = context.limit;
this.size = context.size();
this.context = context;
Expand All @@ -202,7 +205,11 @@ public void collect(int doc) throws IOException {
context.hitContext().cache().clear();
}

searcher.search(query, collector);
if (isNestedDoc) {
searcher.search(query, NonNestedDocsFilter.INSTANCE, collector);
} else {
searcher.search(query, collector);
}
if (collector.exists()) {
if (!limit || counter < size) {
matches.add(values.copyShared());
Expand Down Expand Up @@ -236,8 +243,8 @@ final static class MatchAndSort extends QueryCollector {

private final TopScoreDocCollector topDocsCollector;

MatchAndSort(ESLogger logger, PercolateContext context) {
super(logger, context);
MatchAndSort(ESLogger logger, PercolateContext context, boolean isNestedDoc) {
super(logger, context, isNestedDoc);
// TODO: Use TopFieldCollector.create(...) for ascending and decending scoring?
topDocsCollector = TopScoreDocCollector.create(context.size(), false);
}
Expand All @@ -252,7 +259,11 @@ public void collect(int doc) throws IOException {
// run the query
try {
collector.reset();
searcher.search(query, collector);
if (isNestedDoc) {
searcher.search(query, NonNestedDocsFilter.INSTANCE, collector);
} else {
searcher.search(query, collector);
}
if (collector.exists()) {
topDocsCollector.collect(doc);
postMatch(doc);
Expand Down Expand Up @@ -294,8 +305,8 @@ final static class MatchAndScore extends QueryCollector {

private Scorer scorer;

MatchAndScore(ESLogger logger, PercolateContext context, HighlightPhase highlightPhase) {
super(logger, context);
MatchAndScore(ESLogger logger, PercolateContext context, HighlightPhase highlightPhase, boolean isNestedDoc) {
super(logger, context, isNestedDoc);
this.limit = context.limit;
this.size = context.size();
this.context = context;
Expand All @@ -316,7 +327,11 @@ public void collect(int doc) throws IOException {
context.parsedQuery(new ParsedQuery(query, ImmutableMap.<String, Filter>of()));
context.hitContext().cache().clear();
}
searcher.search(query, collector);
if (isNestedDoc) {
searcher.search(query, NonNestedDocsFilter.INSTANCE, collector);
} else {
searcher.search(query, collector);
}
if (collector.exists()) {
if (!limit || counter < size) {
matches.add(values.copyShared());
Expand Down Expand Up @@ -360,8 +375,8 @@ final static class Count extends QueryCollector {

private long counter = 0;

Count(ESLogger logger, PercolateContext context) {
super(logger, context);
Count(ESLogger logger, PercolateContext context, boolean isNestedDoc) {
super(logger, context, isNestedDoc);
}

@Override
Expand All @@ -374,7 +389,11 @@ public void collect(int doc) throws IOException {
// run the query
try {
collector.reset();
searcher.search(query, collector);
if (isNestedDoc) {
searcher.search(query, NonNestedDocsFilter.INSTANCE, collector);
} else {
searcher.search(query, collector);
}
if (collector.exists()) {
counter++;
postMatch(doc);
Expand Down
Loading

0 comments on commit fe89ea1

Please sign in to comment.