Browse Source

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
Adrien Grand 9 years ago
parent
commit
e1bfe23c22

+ 1 - 1
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());
     }
 

+ 3 - 0
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);

+ 18 - 12
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)));

+ 16 - 0
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")