diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java index f4103d87fbd..f67bec631bc 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java @@ -40,10 +40,10 @@ public class AbstractRangeAggregatorFactory { private final InternalRange.Factory rangeFactory; - private final List ranges; + private final R[] ranges; private final boolean keyed; - public AbstractRangeAggregatorFactory(String name, Type type, ValuesSourceConfig config, List ranges, boolean keyed, + public AbstractRangeAggregatorFactory(String name, Type type, ValuesSourceConfig config, R[] ranges, boolean keyed, InternalRange.Factory rangeFactory, AggregationContext context, AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder, Map metaData) throws IOException { super(name, type, config, context, parent, subFactoriesBuilder, metaData); @@ -55,7 +55,7 @@ public class AbstractRangeAggregatorFactory pipelineAggregators, Map metaData) throws IOException { - return new Unmapped(name, ranges, keyed, config.format(), context, parent, rangeFactory, pipelineAggregators, metaData); + return new Unmapped<>(name, ranges, keyed, config.format(), context, parent, rangeFactory, pipelineAggregators, metaData); } @Override diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeBuilder.java index 13d10bd0a0c..0692b0ed304 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeBuilder.java @@ -19,13 +19,17 @@ package org.elasticsearch.search.aggregations.bucket.range; +import org.apache.lucene.util.InPlaceMergeSorter; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.search.aggregations.bucket.range.RangeAggregator.Range; +import org.elasticsearch.search.aggregations.support.AggregationContext; import org.elasticsearch.search.aggregations.support.ValuesSource; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder; +import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; +import org.elasticsearch.search.aggregations.support.ValuesSource.Numeric; import java.io.IOException; import java.util.ArrayList; @@ -55,6 +59,40 @@ public abstract class AbstractRangeBuilder config) { + Range[] ranges = new Range[this.ranges.size()]; + for (int i = 0; i < ranges.length; i++) { + ranges[i] = this.ranges.get(i).process(config.format(), context.searchContext()); + } + sortRanges(ranges); + return ranges; + } + + private static void sortRanges(final Range[] ranges) { + new InPlaceMergeSorter() { + + @Override + protected void swap(int i, int j) { + final Range tmp = ranges[i]; + ranges[i] = ranges[j]; + ranges[j] = tmp; + } + + @Override + protected int compare(int i, int j) { + int cmp = Double.compare(ranges[i].from, ranges[j].from); + if (cmp == 0) { + cmp = Double.compare(ranges[i].to, ranges[j].to); + } + return cmp; + } + }.sort(0, ranges.length); + } + @Override protected void innerWriteTo(StreamOutput out) throws IOException { out.writeVInt(ranges.size()); diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java index c815ae9d3cf..73a4d86819a 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java @@ -114,6 +114,8 @@ public class RangeAggregationBuilder extends AbstractRangeBuilder config, AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException { + // We need to call processRanges here so they are parsed before we make the decision of whether to cache the request + Range[] ranges = processRanges(context, config); return new RangeAggregatorFactory(name, type, config, ranges, keyed, rangeFactory, context, parent, subFactoriesBuilder, metaData); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java index c83e2d2c721..27d826a644a 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregator.java @@ -19,7 +19,6 @@ package org.elasticsearch.search.aggregations.bucket.range; import org.apache.lucene.index.LeafReaderContext; -import org.apache.lucene.util.InPlaceMergeSorter; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.ParseFieldMatcher; import org.elasticsearch.common.io.stream.StreamInput; @@ -210,7 +209,7 @@ public class RangeAggregator extends BucketsAggregator { final double[] maxTo; public RangeAggregator(String name, AggregatorFactories factories, ValuesSource.Numeric valuesSource, DocValueFormat format, - InternalRange.Factory rangeFactory, List ranges, boolean keyed, AggregationContext aggregationContext, + InternalRange.Factory rangeFactory, Range[] ranges, boolean keyed, AggregationContext aggregationContext, Aggregator parent, List pipelineAggregators, Map metaData) throws IOException { super(name, factories, aggregationContext, parent, pipelineAggregators, metaData); @@ -220,11 +219,7 @@ public class RangeAggregator extends BucketsAggregator { this.keyed = keyed; this.rangeFactory = rangeFactory; - this.ranges = new Range[ranges.size()]; - for (int i = 0; i < this.ranges.length; i++) { - this.ranges[i] = ranges.get(i).process(format, context.searchContext()); - } - sortRanges(this.ranges); + this.ranges = ranges; maxTo = new double[this.ranges.length]; maxTo[0] = this.ranges[0].to; @@ -337,45 +332,21 @@ public class RangeAggregator extends BucketsAggregator { return rangeFactory.create(name, buckets, format, keyed, pipelineAggregators(), metaData()); } - private static void sortRanges(final Range[] ranges) { - new InPlaceMergeSorter() { - - @Override - protected void swap(int i, int j) { - final Range tmp = ranges[i]; - ranges[i] = ranges[j]; - ranges[j] = tmp; - } - - @Override - protected int compare(int i, int j) { - int cmp = Double.compare(ranges[i].from, ranges[j].from); - if (cmp == 0) { - cmp = Double.compare(ranges[i].to, ranges[j].to); - } - return cmp; - } - }.sort(0, ranges.length); - } - public static class Unmapped extends NonCollectingAggregator { - private final List ranges; + private final R[] ranges; private final boolean keyed; private final InternalRange.Factory factory; private final DocValueFormat format; @SuppressWarnings("unchecked") - public Unmapped(String name, List ranges, boolean keyed, DocValueFormat format, + public Unmapped(String name, R[] ranges, boolean keyed, DocValueFormat format, AggregationContext context, Aggregator parent, InternalRange.Factory factory, List pipelineAggregators, Map metaData) throws IOException { super(name, context, parent, pipelineAggregators, metaData); - this.ranges = new ArrayList<>(); - for (R range : ranges) { - this.ranges.add((R) range.process(format, context.searchContext())); - } + this.ranges = ranges; this.keyed = keyed; this.format = format; this.factory = factory; @@ -384,7 +355,7 @@ public class RangeAggregator extends BucketsAggregator { @Override public InternalAggregation buildEmptyAggregation() { InternalAggregations subAggs = buildEmptySubAggregations(); - List buckets = new ArrayList<>(ranges.size()); + List buckets = new ArrayList<>(ranges.length); for (RangeAggregator.Range range : ranges) { buckets.add(factory.createBucket(range.key, range.from, range.to, 0, subAggs, keyed, format)); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorFactory.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorFactory.java index 5dec4c40c45..b3297401457 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorFactory.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorFactory.java @@ -29,12 +29,11 @@ import org.elasticsearch.search.aggregations.support.AggregationContext; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; import java.io.IOException; -import java.util.List; import java.util.Map; public class RangeAggregatorFactory extends AbstractRangeAggregatorFactory { - public RangeAggregatorFactory(String name, Type type, ValuesSourceConfig config, List ranges, boolean keyed, + public RangeAggregatorFactory(String name, Type type, ValuesSourceConfig config, Range[] ranges, boolean keyed, Factory rangeFactory, AggregationContext context, AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder, Map metaData) throws IOException { super(name, type, config, ranges, keyed, rangeFactory, context, parent, subFactoriesBuilder, metaData); diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/date/DateRangeAggregationBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/date/DateRangeAggregationBuilder.java index a75b071569c..c8a8e16640b 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/date/DateRangeAggregationBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/date/DateRangeAggregationBuilder.java @@ -259,6 +259,9 @@ public class DateRangeAggregationBuilder extends AbstractRangeBuilder config, AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException { + // We need to call processRanges here so they are parsed and we know whether `now` has been used before we make + // the decision of whether to cache the request + Range[] ranges = processRanges(context, config); return new DateRangeAggregatorFactory(name, type, config, ranges, keyed, rangeFactory, context, parent, subFactoriesBuilder, metaData); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/date/DateRangeAggregatorFactory.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/date/DateRangeAggregatorFactory.java index d3bb7ac6238..d5d16123ec3 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/date/DateRangeAggregatorFactory.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/date/DateRangeAggregatorFactory.java @@ -30,12 +30,11 @@ import org.elasticsearch.search.aggregations.support.AggregationContext; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; import java.io.IOException; -import java.util.List; import java.util.Map; public class DateRangeAggregatorFactory extends AbstractRangeAggregatorFactory { - public DateRangeAggregatorFactory(String name, Type type, ValuesSourceConfig config, List ranges, boolean keyed, + public DateRangeAggregatorFactory(String name, Type type, ValuesSourceConfig config, Range[] ranges, boolean keyed, Factory rangeFactory, AggregationContext context, AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder, Map metaData) throws IOException { super(name, type, config, ranges, keyed, rangeFactory, context, parent, subFactoriesBuilder, metaData); diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/GeoDistanceAggregationBuilder.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/GeoDistanceAggregationBuilder.java index 4a4cab2affa..583bc83feb4 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/GeoDistanceAggregationBuilder.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/GeoDistanceAggregationBuilder.java @@ -215,6 +215,7 @@ public class GeoDistanceAggregationBuilder extends ValuesSourceAggregationBuilde protected ValuesSourceAggregatorFactory innerBuild(AggregationContext context, ValuesSourceConfig config, AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException { + Range[] ranges = this.ranges.toArray(new Range[this.range().size()]); return new GeoDistanceRangeAggregatorFactory(name, type, config, origin, ranges, unit, distanceType, keyed, context, parent, subFactoriesBuilder, metaData); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/GeoDistanceRangeAggregatorFactory.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/GeoDistanceRangeAggregatorFactory.java index 32c3592a8fc..62aa18b168a 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/GeoDistanceRangeAggregatorFactory.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/range/geodistance/GeoDistanceRangeAggregatorFactory.java @@ -51,13 +51,13 @@ public class GeoDistanceRangeAggregatorFactory private final InternalRange.Factory rangeFactory = InternalGeoDistance.FACTORY; private final GeoPoint origin; - private final List ranges; + private final Range[] ranges; private final DistanceUnit unit; private final GeoDistance distanceType; private final boolean keyed; public GeoDistanceRangeAggregatorFactory(String name, Type type, ValuesSourceConfig config, GeoPoint origin, - List ranges, DistanceUnit unit, GeoDistance distanceType, boolean keyed, AggregationContext context, + Range[] ranges, DistanceUnit unit, GeoDistance distanceType, boolean keyed, AggregationContext context, AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder, Map metaData) throws IOException { super(name, type, config, context, parent, subFactoriesBuilder, metaData); this.origin = origin; @@ -70,7 +70,7 @@ public class GeoDistanceRangeAggregatorFactory @Override protected Aggregator createUnmapped(Aggregator parent, List pipelineAggregators, Map metaData) throws IOException { - return new Unmapped(name, ranges, keyed, config.format(), context, parent, rangeFactory, pipelineAggregators, metaData); + return new Unmapped<>(name, ranges, keyed, config.format(), context, parent, rangeFactory, pipelineAggregators, metaData); } @Override diff --git a/core/src/test/java/org/elasticsearch/indices/IndicesRequestCacheIT.java b/core/src/test/java/org/elasticsearch/indices/IndicesRequestCacheIT.java index 7b9f2dbb7cf..078bf499ff4 100644 --- a/core/src/test/java/org/elasticsearch/indices/IndicesRequestCacheIT.java +++ b/core/src/test/java/org/elasticsearch/indices/IndicesRequestCacheIT.java @@ -34,6 +34,8 @@ import org.joda.time.chrono.ISOChronology; import java.util.List; import static org.elasticsearch.search.aggregations.AggregationBuilders.dateHistogram; +import static org.elasticsearch.search.aggregations.AggregationBuilders.filter; +import static org.elasticsearch.search.aggregations.AggregationBuilders.dateRange; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse; import static org.hamcrest.Matchers.equalTo; @@ -406,11 +408,33 @@ public class IndicesRequestCacheIT extends ESIntegTestCase { assertThat(client().admin().indices().prepareStats("index").setRequestCache(true).get().getTotal().getRequestCache().getMissCount(), equalTo(0L)); - // If size > 1 and cache flag is set on the request we should cache - final SearchResponse r4 = client().prepareSearch("index").setSearchType(SearchType.QUERY_THEN_FETCH).setSize(1) - .setRequestCache(true).setQuery(QueryBuilders.rangeQuery("s").gte("2016-03-21").lte("2016-03-27")).get(); + // If the request has an aggregation containng now we should not cache + final SearchResponse r4 = client().prepareSearch("index").setSearchType(SearchType.QUERY_THEN_FETCH).setSize(0) + .setRequestCache(true).setQuery(QueryBuilders.rangeQuery("s").gte("2016-03-20").lte("2016-03-26")) + .addAggregation(filter("foo", QueryBuilders.rangeQuery("s").from("now-10y").to("now"))).get(); assertSearchResponse(r4); assertThat(r4.getHits().getTotalHits(), equalTo(7L)); + assertThat(client().admin().indices().prepareStats("index").setRequestCache(true).get().getTotal().getRequestCache().getHitCount(), + equalTo(0L)); + assertThat(client().admin().indices().prepareStats("index").setRequestCache(true).get().getTotal().getRequestCache().getMissCount(), + equalTo(0L)); + + // If the request has an aggregation containng now we should not cache + final SearchResponse r5 = client().prepareSearch("index").setSearchType(SearchType.QUERY_THEN_FETCH).setSize(0) + .setRequestCache(true).setQuery(QueryBuilders.rangeQuery("s").gte("2016-03-20").lte("2016-03-26")) + .addAggregation(dateRange("foo").field("s").addRange("now-10y", "now")).get(); + assertSearchResponse(r5); + assertThat(r5.getHits().getTotalHits(), equalTo(7L)); + assertThat(client().admin().indices().prepareStats("index").setRequestCache(true).get().getTotal().getRequestCache().getHitCount(), + equalTo(0L)); + assertThat(client().admin().indices().prepareStats("index").setRequestCache(true).get().getTotal().getRequestCache().getMissCount(), + equalTo(0L)); + + // If size > 1 and cache flag is set on the request we should cache + final SearchResponse r6 = client().prepareSearch("index").setSearchType(SearchType.QUERY_THEN_FETCH).setSize(1) + .setRequestCache(true).setQuery(QueryBuilders.rangeQuery("s").gte("2016-03-21").lte("2016-03-27")).get(); + assertSearchResponse(r6); + assertThat(r6.getHits().getTotalHits(), equalTo(7L)); assertThat(client().admin().indices().prepareStats("index").setRequestCache(true).get().getTotal().getRequestCache().getHitCount(), equalTo(0L)); assertThat(client().admin().indices().prepareStats("index").setRequestCache(true).get().getTotal().getRequestCache().getMissCount(),