From 164f7b981eecc6fe04cff612bd6364dacaa811c3 Mon Sep 17 00:00:00 2001 From: uboness Date: Fri, 21 Feb 2014 02:51:32 +0100 Subject: [PATCH] Fixed an issue where and IndexOutOfBoundsException was thrown when a date_/histogram aggregation was defined on unmapped field and also had a sub aggregation. The root cause there was that in such case, the estimated bucket count was 0, and the code was not designed to handle that well. Closes #5179 --- .../aggregations/AggregatorFactories.java | 8 +++- .../bucket/histogram/DateHistogramParser.java | 2 +- .../search/aggregations/CombiTests.java | 37 ++++++++++++++++++- 3 files changed, 42 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java b/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java index b51d2a24ad1..047de1f1a13 100644 --- a/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java +++ b/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java @@ -73,9 +73,13 @@ public class AggregatorFactories { ObjectArray aggregators; { - aggregators = BigArrays.newObjectArray(estimatedBucketsCount, context.pageCacheRecycler()); + // if estimated count is zero, we at least create a single aggregator. + // The estimated count is just an estimation and we can't rely on how it's estimated (that is, an + // estimation of 0 should not imply that we'll end up without any buckets) + long arraySize = estimatedBucketsCount > 0 ? estimatedBucketsCount : 1; + aggregators = BigArrays.newObjectArray(arraySize , context.pageCacheRecycler()); aggregators.set(0, first); - for (long i = 1; i < estimatedBucketsCount; ++i) { + for (long i = 1; i < arraySize; ++i) { aggregators.set(i, createAndRegisterContextAware(parent.context(), factory, parent, estimatedBucketsCount)); } } diff --git a/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramParser.java b/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramParser.java index d8f3f1666b1..85d615e22b9 100644 --- a/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramParser.java +++ b/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/DateHistogramParser.java @@ -223,7 +223,7 @@ public class DateHistogramParser implements Aggregator.Parser { } if (!(mapper instanceof DateFieldMapper)) { - throw new SearchParseException(context, "date histogram can only be aggregated on date fields but [" + field + "] is not a date field"); + throw new SearchParseException(context, "date histogram can only be aggregated on date fields but [" + field + "] is not a date field"); } IndexFieldData indexFieldData = context.fieldData().getForField(mapper); diff --git a/src/test/java/org/elasticsearch/search/aggregations/CombiTests.java b/src/test/java/org/elasticsearch/search/aggregations/CombiTests.java index 07622c30dc0..9633b857d80 100644 --- a/src/test/java/org/elasticsearch/search/aggregations/CombiTests.java +++ b/src/test/java/org/elasticsearch/search/aggregations/CombiTests.java @@ -25,18 +25,21 @@ import org.elasticsearch.action.index.IndexRequestBuilder; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.common.settings.ImmutableSettings; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.search.aggregations.bucket.histogram.Histogram; import org.elasticsearch.search.aggregations.bucket.missing.Missing; import org.elasticsearch.search.aggregations.bucket.terms.Terms; import org.elasticsearch.test.ElasticsearchIntegrationTest; +import org.hamcrest.Matchers; import org.junit.Test; import java.util.Collection; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; -import static org.elasticsearch.search.aggregations.AggregationBuilders.missing; -import static org.elasticsearch.search.aggregations.AggregationBuilders.terms; +import static org.elasticsearch.search.aggregations.AggregationBuilders.*; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse; import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.core.IsNull.notNullValue; /** * @@ -110,4 +113,34 @@ public class CombiTests extends ElasticsearchIntegrationTest { } assertTrue(values.isEmpty()); } + + + /** + * Some top aggs (eg. date_/histogram) that are executed on unmapped fields, will generate an estimate count of buckets - zero. + * when the sub aggregator is then created, it will take this estimation into account. This used to cause + * and an ArrayIndexOutOfBoundsException... + */ + @Test + public void subAggregationForTopAggregationOnUnmappedField() throws Exception { + + prepareCreate("idx").addMapping("type", jsonBuilder() + .startObject() + .startObject("type").startObject("properties") + .startObject("name").field("type", "string").endObject() + .startObject("value").field("type", "integer").endObject() + .endObject().endObject() + .endObject()).execute().actionGet(); + + ensureSearchable("idx"); + + SearchResponse searchResponse = client().prepareSearch("idx") + .addAggregation(histogram("values").field("value1").interval(1) + .subAggregation(terms("names").field("name"))) + .execute().actionGet(); + + assertThat(searchResponse.getHits().getTotalHits(), Matchers.equalTo(0l)); + Histogram values = searchResponse.getAggregations().get("values"); + assertThat(values, notNullValue()); + assertThat(values.getBuckets().isEmpty(), is(true)); + } }