Skip to content

Commit b124d65

Browse files
vishnugtpolyfractal
authored andcommitted
Do not allow negative variances (#37384)
Due to floating point error, it was possible for variances to become negative which should never happen. This bugfix sets variance to zero if it becomes negative as a result of fp error.
1 parent e653508 commit b124d65

File tree

5 files changed

+37
-4
lines changed

5 files changed

+37
-4
lines changed

server/src/main/java/org/elasticsearch/search/aggregations/metrics/stats/extended/ExtendedStatsAggregator.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,8 @@ public double metric(String name, long owningBucketOrd) {
202202
private double variance(long owningBucketOrd) {
203203
double sum = sums.get(owningBucketOrd);
204204
long count = counts.get(owningBucketOrd);
205-
return (sumOfSqrs.get(owningBucketOrd) - ((sum * sum) / count)) / count;
205+
double variance = (sumOfSqrs.get(owningBucketOrd) - ((sum * sum) / count)) / count;
206+
return variance < 0 ? 0 : variance;
206207
}
207208

208209
@Override

server/src/main/java/org/elasticsearch/search/aggregations/metrics/stats/extended/InternalExtendedStats.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,8 @@ public double getSumOfSquares() {
102102

103103
@Override
104104
public double getVariance() {
105-
return (sumOfSqrs - ((sum * sum) / count)) / count;
105+
double variance = (sumOfSqrs - ((sum * sum) / count)) / count;
106+
return variance < 0 ? 0 : variance;
106107
}
107108

108109
@Override

server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorTests.java

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,34 @@ public void testRandomDoubles() throws IOException {
9999
);
100100
}
101101

102+
/**
103+
* Testcase for https://github.com/elastic/elasticsearch/issues/37303
104+
*/
105+
public void testVarianceNonNegative() throws IOException {
106+
MappedFieldType ft =
107+
new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.DOUBLE);
108+
ft.setName("field");
109+
final ExtendedSimpleStatsAggregator expected = new ExtendedSimpleStatsAggregator();
110+
testCase(ft,
111+
iw -> {
112+
int numDocs = 3;
113+
for (int i = 0; i < numDocs; i++) {
114+
Document doc = new Document();
115+
double value = 49.95d;
116+
long valueAsLong = NumericUtils.doubleToSortableLong(value);
117+
doc.add(new SortedNumericDocValuesField("field", valueAsLong));
118+
expected.add(value);
119+
iw.addDocument(doc);
120+
}
121+
},
122+
stats -> {
123+
//since the value(49.95) is a constant, variance should be 0
124+
assertEquals(0.0d, stats.getVariance(), TOLERANCE);
125+
assertEquals(0.0d, stats.getStdDeviation(), TOLERANCE);
126+
}
127+
);
128+
}
129+
102130
public void testRandomLongs() throws IOException {
103131
MappedFieldType ft =
104132
new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG);
@@ -235,7 +263,8 @@ void add(double value) {
235263
}
236264

237265
double variance() {
238-
return (sumOfSqrs - ((sum * sum) / count)) / count;
266+
double variance = (sumOfSqrs - ((sum * sum) / count)) / count;
267+
return variance < 0 ? 0 : variance;
239268
}
240269
}
241270
}

server/src/test/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsIT.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,8 @@ private static double variance(int... vals) {
7474
sum += val;
7575
sumOfSqrs += val * val;
7676
}
77-
return (sumOfSqrs - ((sum * sum) / vals.length)) / vals.length;
77+
double variance = (sumOfSqrs - ((sum * sum) / vals.length)) / vals.length;
78+
return variance < 0 ? 0 : variance;
7879
}
7980

8081
@Override

server/src/test/java/org/elasticsearch/search/aggregations/pipeline/ExtendedStatsBucketIT.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,7 @@ public void testGappyIndexWithSigma() {
138138
double sumOfSqrs = 1.0 + 1.0 + 1.0 + 4.0 + 0.0 + 1.0;
139139
double avg = sum / count;
140140
double var = (sumOfSqrs - ((sum * sum) / count)) / count;
141+
var = var < 0 ? 0 : var;
141142
double stdDev = Math.sqrt(var);
142143
assertThat(extendedStatsBucketValue, notNullValue());
143144
assertThat(extendedStatsBucketValue.getName(), equalTo("extended_stats_bucket"));

0 commit comments

Comments
 (0)