From 0c35a1756b3a030e867c792a428811504ad05b7d Mon Sep 17 00:00:00 2001 From: Vishnu Gt Date: Sat, 12 Jan 2019 17:39:28 +0530 Subject: [PATCH 1/2] Issue #37303 - Invalid variance fix --- .../search/aggregations/metrics/ExtendedStatsAggregator.java | 3 ++- .../search/aggregations/metrics/InternalExtendedStats.java | 3 ++- .../aggregations/metrics/ExtendedStatsAggregatorTests.java | 3 ++- .../search/aggregations/metrics/ExtendedStatsIT.java | 3 ++- .../search/aggregations/pipeline/ExtendedStatsBucketIT.java | 1 + 5 files changed, 9 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregator.java index 1d383a2ae1946..4774bec573e42 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregator.java @@ -202,7 +202,8 @@ public double metric(String name, long owningBucketOrd) { private double variance(long owningBucketOrd) { double sum = sums.get(owningBucketOrd); long count = counts.get(owningBucketOrd); - return (sumOfSqrs.get(owningBucketOrd) - ((sum * sum) / count)) / count; + double variance = (sumOfSqrs.get(owningBucketOrd) - ((sum * sum) / count)) / count; + return variance < 0 ? 0 : variance; } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalExtendedStats.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalExtendedStats.java index 608fd1de435c8..26a244c8ddfb8 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalExtendedStats.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalExtendedStats.java @@ -101,7 +101,8 @@ public double getSumOfSquares() { @Override public double getVariance() { - return (sumOfSqrs - ((sum * sum) / count)) / count; + double variance = (sumOfSqrs - ((sum * sum) / count)) / count; + return variance < 0 ? 0 : variance; } @Override diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorTests.java index e65d1269520bc..fb50b84e4497e 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorTests.java @@ -232,7 +232,8 @@ void add(double value) { } double variance() { - return (sumOfSqrs - ((sum * sum) / count)) / count; + double variance = (sumOfSqrs - ((sum * sum) / count)) / count; + return variance < 0 ? 0 : variance; } } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsIT.java index 4aa16d6f1d5ca..bdf678174967b 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsIT.java @@ -73,7 +73,8 @@ private static double variance(int... vals) { sum += val; sumOfSqrs += val * val; } - return (sumOfSqrs - ((sum * sum) / vals.length)) / vals.length; + double variance = (sumOfSqrs - ((sum * sum) / vals.length)) / vals.length; + return variance < 0 ? 0 : variance; } @Override diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/ExtendedStatsBucketIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/ExtendedStatsBucketIT.java index a8ebf687ad623..9155947e3b6de 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/ExtendedStatsBucketIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/ExtendedStatsBucketIT.java @@ -137,6 +137,7 @@ public void testGappyIndexWithSigma() { double sumOfSqrs = 1.0 + 1.0 + 1.0 + 4.0 + 0.0 + 1.0; double avg = sum / count; double var = (sumOfSqrs - ((sum * sum) / count)) / count; + var = var < 0 ? 0 : var; double stdDev = Math.sqrt(var); assertThat(extendedStatsBucketValue, notNullValue()); assertThat(extendedStatsBucketValue.getName(), equalTo("extended_stats_bucket")); From 36c886273da56cfb7872c65437b37af534da608a Mon Sep 17 00:00:00 2001 From: Vishnu Gt Date: Tue, 15 Jan 2019 00:34:38 +0530 Subject: [PATCH 2/2] Testcase added for #37303 --- .../metrics/ExtendedStatsAggregatorTests.java | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorTests.java index fb50b84e4497e..389cd936291f8 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorTests.java @@ -96,6 +96,34 @@ public void testRandomDoubles() throws IOException { ); } + /** + * Testcase for https://github.com/elastic/elasticsearch/issues/37303 + */ + public void testVarianceNonNegative() throws IOException { + MappedFieldType ft = + new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.DOUBLE); + ft.setName("field"); + final ExtendedSimpleStatsAggregator expected = new ExtendedSimpleStatsAggregator(); + testCase(ft, + iw -> { + int numDocs = 3; + for (int i = 0; i < numDocs; i++) { + Document doc = new Document(); + double value = 49.95d; + long valueAsLong = NumericUtils.doubleToSortableLong(value); + doc.add(new SortedNumericDocValuesField("field", valueAsLong)); + expected.add(value); + iw.addDocument(doc); + } + }, + stats -> { + //since the value(49.95) is a constant, variance should be 0 + assertEquals(0.0d, stats.getVariance(), TOLERANCE); + assertEquals(0.0d, stats.getStdDeviation(), TOLERANCE); + } + ); + } + public void testRandomLongs() throws IOException { MappedFieldType ft = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG);