Caching in nested aggregations?

henryptung · June 12, 2019, 2:23pm

Hi devs,

I recently made a change to enable caching for nested filters in sorts. It seems like there may be a similar lack of caching in nested aggregations?

As seen here (click the link with the diff-line, and let it load for a bit):

Nested aggregations used to cache its filters in the distant past, and a conscious decision was made at the time to use Lucene's cache. However as seen here (click the link with the diff-line):

That choice to lose Lucene's cache seems to have been lost, and no cache at all is used for this computation. At this point, the NestedAggregator has access to the full SearchContext:

github.com

elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/search/aggregations/bucket/nested/NestedAggregator.java#L63




static final ParseField PATH_FIELD = new ParseField("path");


private final BitSetProducer parentFilter;
private final Query childFilter;
private final boolean collectsFromSingleBucket;


private BufferingNestedLeafBucketCollector bufferingNestedLeafBucketCollector;


NestedAggregator(String name, AggregatorFactories factories, ObjectMapper parentObjectMapper, ObjectMapper childObjectMapper,
                 SearchContext context, Aggregator parentAggregator,
                 List<PipelineAggregator> pipelineAggregators, Map<String, Object> metaData,
                 boolean collectsFromSingleBucket) throws IOException {
    super(name, factories, context, parentAggregator, pipelineAggregators, metaData);


    Query parentFilter = parentObjectMapper != null ? parentObjectMapper.nestedTypeFilter()
        : Queries.newNonNestedFilter();
    this.parentFilter = context.bitsetFilterCache().getBitSetProducer(parentFilter);
    this.childFilter = childObjectMapper.nestedTypeFilter();
    this.collectsFromSingleBucket = collectsFromSingleBucket;
}

so would it be appropriate to delegate back to the ContextIndexSearcher here and leverage query-filter caching again?

This is wholly speculative and I've done no practical testing of this - just something I stumbled upon looking through the code that looks remarkably similar to:

jimczi · June 18, 2019, 9:30am

so would it be appropriate to delegate back to the ContextIndexSearcher here and leverage query-filter caching again?

I don't think this would change anything because the child filter is always a simple TermQuery that matches all children of the current nested path. We don't cache TermQuery since https://github.com/elastic/elasticsearch/pull/21566 so it seems ok to me to disable caching entirely when we run this filter.