From e1bfe23c2225da825f3fd1cb205c13e53d5989eb Mon Sep 17 00:00:00 2001 From: Adrien Grand Date: Tue, 29 Mar 2016 19:04:47 +0200 Subject: [PATCH] ExtendedStatsAggregator should also pass sigma to emtpy aggs. #17388 Because sigma is also used at reduce time, it should be passed to empty aggs. Otherwise it causes bugs when an empty aggregation is used to perform reduction is it would assume a sigma of zero. Closes #17362 --- .../extended/ExtendedStatsAggregator.java | 2 +- .../stats/extended/InternalExtendedStats.java | 3 ++ .../messy/tests/ExtendedStatsTests.java | 30 +++++++++++-------- .../elasticsearch/messy/tests/StatsTests.java | 16 ++++++++++ 4 files changed, 38 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/stats/extended/ExtendedStatsAggregator.java b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/stats/extended/ExtendedStatsAggregator.java index 104cd36367b..c9413e15d2a 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/stats/extended/ExtendedStatsAggregator.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/stats/extended/ExtendedStatsAggregator.java @@ -193,7 +193,7 @@ public class ExtendedStatsAggregator extends NumericMetricsAggregator.MultiValue @Override public InternalAggregation buildEmptyAggregation() { - return new InternalExtendedStats(name, 0, 0d, Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY, 0d, 0d, formatter, pipelineAggregators(), + return new InternalExtendedStats(name, 0, 0d, Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY, 0d, sigma, formatter, pipelineAggregators(), metaData()); } diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/stats/extended/InternalExtendedStats.java b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/stats/extended/InternalExtendedStats.java index 9fac5809cef..fe83e045952 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/stats/extended/InternalExtendedStats.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/stats/extended/InternalExtendedStats.java @@ -148,6 +148,9 @@ public class InternalExtendedStats extends InternalStats implements ExtendedStat double sumOfSqrs = 0; for (InternalAggregation aggregation : aggregations) { InternalExtendedStats stats = (InternalExtendedStats) aggregation; + if (stats.sigma != sigma) { + throw new IllegalStateException("Cannot reduce other stats aggregations that have a different sigma"); + } sumOfSqrs += stats.getSumOfSquares(); } final InternalStats stats = super.doReduce(aggregations, reduceContext); diff --git a/modules/lang-groovy/src/test/java/org/elasticsearch/messy/tests/ExtendedStatsTests.java b/modules/lang-groovy/src/test/java/org/elasticsearch/messy/tests/ExtendedStatsTests.java index c38e8f2a979..d7fc77c9ccb 100644 --- a/modules/lang-groovy/src/test/java/org/elasticsearch/messy/tests/ExtendedStatsTests.java +++ b/modules/lang-groovy/src/test/java/org/elasticsearch/messy/tests/ExtendedStatsTests.java @@ -19,7 +19,6 @@ package org.elasticsearch.messy.tests; import org.elasticsearch.action.search.SearchResponse; -import org.elasticsearch.action.search.ShardSearchFailure; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptService.ScriptType; @@ -129,6 +128,24 @@ public class ExtendedStatsTests extends AbstractNumericTestCase { assertThat(Double.isNaN(stats.getStdDeviationBound(ExtendedStats.Bounds.LOWER)), is(true)); } + public void testPartiallyUnmapped() { + double sigma = randomDouble() * 5; + ExtendedStats s1 = client().prepareSearch("idx") + .addAggregation(extendedStats("stats").field("value").sigma(sigma)).get() + .getAggregations().get("stats"); + ExtendedStats s2 = client().prepareSearch("idx", "idx_unmapped") + .addAggregation(extendedStats("stats").field("value").sigma(sigma)).get() + .getAggregations().get("stats"); + assertEquals(s1.getAvg(), s2.getAvg(), 1e-10); + assertEquals(s1.getCount(), s2.getCount()); + assertEquals(s1.getMin(), s2.getMin(), 0d); + assertEquals(s1.getMax(), s2.getMax(), 0d); + assertEquals(s1.getStdDeviation(), s2.getStdDeviation(), 1e-10); + assertEquals(s1.getSumOfSquares(), s2.getSumOfSquares(), 1e-10); + assertEquals(s1.getStdDeviationBound(Bounds.LOWER), s2.getStdDeviationBound(Bounds.LOWER), 1e-10); + assertEquals(s1.getStdDeviationBound(Bounds.UPPER), s2.getStdDeviationBound(Bounds.UPPER), 1e-10); + } + @Override public void testSingleValuedField() throws Exception { double sigma = randomDouble() * randomIntBetween(1, 10); @@ -584,17 +601,6 @@ public class ExtendedStatsTests extends AbstractNumericTestCase { } } - private void assertShardExecutionState(SearchResponse response, int expectedFailures) throws Exception { - ShardSearchFailure[] failures = response.getShardFailures(); - if (failures.length != expectedFailures) { - for (ShardSearchFailure failure : failures) { - logger.error("Shard Failure: {}", failure.getCause(), failure); - } - fail("Unexpected shard failures!"); - } - assertThat("Not all shards are initialized", response.getSuccessfulShards(), equalTo(response.getTotalShards())); - } - private void checkUpperLowerBounds(ExtendedStats stats, double sigma) { assertThat(stats.getStdDeviationBound(ExtendedStats.Bounds.UPPER), equalTo(stats.getAvg() + (stats.getStdDeviation() * sigma))); assertThat(stats.getStdDeviationBound(ExtendedStats.Bounds.LOWER), equalTo(stats.getAvg() - (stats.getStdDeviation() * sigma))); diff --git a/modules/lang-groovy/src/test/java/org/elasticsearch/messy/tests/StatsTests.java b/modules/lang-groovy/src/test/java/org/elasticsearch/messy/tests/StatsTests.java index 9af5e086a34..beb462fea00 100644 --- a/modules/lang-groovy/src/test/java/org/elasticsearch/messy/tests/StatsTests.java +++ b/modules/lang-groovy/src/test/java/org/elasticsearch/messy/tests/StatsTests.java @@ -31,6 +31,8 @@ import org.elasticsearch.search.aggregations.bucket.terms.Terms; import org.elasticsearch.search.aggregations.bucket.terms.Terms.Order; import org.elasticsearch.search.aggregations.metrics.AbstractNumericTestCase; import org.elasticsearch.search.aggregations.metrics.stats.Stats; +import org.elasticsearch.search.aggregations.metrics.stats.extended.ExtendedStats; +import org.elasticsearch.search.aggregations.metrics.stats.extended.ExtendedStats.Bounds; import java.util.Collection; import java.util.Collections; @@ -40,6 +42,7 @@ import java.util.Map; import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery; import static org.elasticsearch.index.query.QueryBuilders.termQuery; +import static org.elasticsearch.search.aggregations.AggregationBuilders.extendedStats; import static org.elasticsearch.search.aggregations.AggregationBuilders.filter; import static org.elasticsearch.search.aggregations.AggregationBuilders.global; import static org.elasticsearch.search.aggregations.AggregationBuilders.histogram; @@ -106,6 +109,19 @@ public class StatsTests extends AbstractNumericTestCase { assertThat(stats.getCount(), equalTo(0L)); } + public void testPartiallyUnmapped() { + Stats s1 = client().prepareSearch("idx") + .addAggregation(stats("stats").field("value")).get() + .getAggregations().get("stats"); + ExtendedStats s2 = client().prepareSearch("idx", "idx_unmapped") + .addAggregation(stats("stats").field("value")).get() + .getAggregations().get("stats"); + assertEquals(s1.getAvg(), s2.getAvg(), 1e-10); + assertEquals(s1.getCount(), s2.getCount()); + assertEquals(s1.getMin(), s2.getMin(), 0d); + assertEquals(s1.getMax(), s2.getMax(), 0d); + } + @Override public void testSingleValuedField() throws Exception { SearchResponse searchResponse = client().prepareSearch("idx")