Skip to content

Commit 517c4bf

Browse files
authored
[8.x] ESQL: Fix MvPercentileTests precision issues (elastic#114844) (elastic#114896)
* ESQL: Fix MvPercentileTests precision issues (elastic#114844) Fixes elastic#114588 Fixes elastic#114587 Fixes elastic#114586 Fixes elastic#114585 Fixes elastic#113008 Fixes elastic#113007 Fixes elastic#113006 Fixes elastic#113005 Fixed the long precision issue by allowing a +/-1 range. Also made a minor refactor to simplify using different matchers for different types. * Unmute MvPercentileTests
1 parent a703b20 commit 517c4bf

File tree

2 files changed

+30
-29
lines changed

2 files changed

+30
-29
lines changed

muted-tests.yml

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -315,18 +315,6 @@ tests:
315315
- class: org.elasticsearch.xpack.esql.qa.mixed.MixedClusterEsqlSpecIT
316316
method: test {categorize.Categorize ASYNC}
317317
issue: https://github.com/elastic/elasticsearch/issues/113721
318-
- class: org.elasticsearch.xpack.esql.expression.function.scalar.multivalue.MvPercentileTests
319-
method: "testEvaluateBlockWithoutNulls {TestCase=field: <positive mv UNORDERED longs>, percentile: <small positive double>}"
320-
issue: https://github.com/elastic/elasticsearch/issues/114585
321-
- class: org.elasticsearch.xpack.esql.expression.function.scalar.multivalue.MvPercentileTests
322-
method: "testEvaluateBlockWithoutNulls {TestCase=field: <positive mv SORTED_ASCENDING longs>, percentile: <small positive double>}"
323-
issue: https://github.com/elastic/elasticsearch/issues/114586
324-
- class: org.elasticsearch.xpack.esql.expression.function.scalar.multivalue.MvPercentileTests
325-
method: "testEvaluateBlockWithoutNulls {TestCase=field: <positive mv DEDUPLICATED_AND_SORTED_ASCENDING longs>, percentile: <small positive double>}"
326-
issue: https://github.com/elastic/elasticsearch/issues/114587
327-
- class: org.elasticsearch.xpack.esql.expression.function.scalar.multivalue.MvPercentileTests
328-
method: "testEvaluateBlockWithoutNulls {TestCase=field: <positive mv DEDUPLICATED_UNORDERD longs>, percentile: <small positive double>}"
329-
issue: https://github.com/elastic/elasticsearch/issues/114588
330318
- class: org.elasticsearch.action.search.SearchQueryThenFetchAsyncActionTests
331319
method: testMinimumVersionSameAsNewVersion
332320
issue: https://github.com/elastic/elasticsearch/issues/114593

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvPercentileTests.java

Lines changed: 30 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import org.elasticsearch.xpack.esql.expression.function.AbstractScalarFunctionTestCase;
1818
import org.elasticsearch.xpack.esql.expression.function.MultivalueTestCaseSupplier;
1919
import org.elasticsearch.xpack.esql.expression.function.TestCaseSupplier;
20+
import org.hamcrest.Matcher;
2021

2122
import java.math.BigDecimal;
2223
import java.util.ArrayList;
@@ -28,6 +29,7 @@
2829
import static org.elasticsearch.xpack.esql.core.type.DataType.DOUBLE;
2930
import static org.elasticsearch.xpack.esql.core.type.DataType.INTEGER;
3031
import static org.elasticsearch.xpack.esql.core.type.DataType.LONG;
32+
import static org.hamcrest.Matchers.anyOf;
3133
import static org.hamcrest.Matchers.closeTo;
3234
import static org.hamcrest.Matchers.equalTo;
3335
import static org.hamcrest.Matchers.nullValue;
@@ -375,27 +377,25 @@ private static TestCaseSupplier makeSupplier(
375377
var values = (List<Number>) fieldTypedData.data();
376378
var percentile = ((Number) percentileTypedData.data()).doubleValue();
377379

378-
var expected = calculatePercentile(values, percentile);
380+
var expectedMatcher = makePercentileMatcher(values, percentile);
379381

380382
return new TestCaseSupplier.TestCase(
381383
List.of(fieldTypedData, percentileTypedData),
382384
evaluatorString(fieldSupplier.type(), percentileSupplier.type()),
383385
fieldSupplier.type(),
384-
expected instanceof Double expectedDouble
385-
? closeTo(expectedDouble, Math.abs(expectedDouble * 0.0000001))
386-
: equalTo(expected)
386+
expectedMatcher
387387
);
388388
}
389389
);
390390
}
391391

392-
private static Number calculatePercentile(List<Number> rawValues, double percentile) {
392+
private static Matcher<?> makePercentileMatcher(List<Number> rawValues, double percentile) {
393393
if (rawValues.isEmpty() || percentile < 0 || percentile > 100) {
394-
return null;
394+
return nullValue();
395395
}
396396

397397
if (rawValues.size() == 1) {
398-
return rawValues.get(0);
398+
return equalTo(rawValues.get(0));
399399
}
400400

401401
int valueCount = rawValues.size();
@@ -407,49 +407,62 @@ private static Number calculatePercentile(List<Number> rawValues, double percent
407407

408408
if (rawValues.get(0) instanceof Integer) {
409409
var values = rawValues.stream().mapToInt(Number::intValue).sorted().toArray();
410+
int expected;
410411

411412
if (percentile == 0) {
412-
return values[0];
413+
expected = values[0];
413414
} else if (percentile == 100) {
414-
return values[valueCount - 1];
415+
expected = values[valueCount - 1];
415416
} else {
416417
assert lowerIndex >= 0 && upperIndex < valueCount;
417418
var difference = (long) values[upperIndex] - values[lowerIndex];
418-
return values[lowerIndex] + (int) (fraction * difference);
419+
expected = values[lowerIndex] + (int) (fraction * difference);
419420
}
421+
422+
return equalTo(expected);
420423
}
421424

422425
if (rawValues.get(0) instanceof Long) {
423426
var values = rawValues.stream().mapToLong(Number::longValue).sorted().toArray();
427+
long expected;
424428

425429
if (percentile == 0) {
426-
return values[0];
430+
expected = values[0];
427431
} else if (percentile == 100) {
428-
return values[valueCount - 1];
432+
expected = values[valueCount - 1];
429433
} else {
430434
assert lowerIndex >= 0 && upperIndex < valueCount;
431-
return calculatePercentile(fraction, new BigDecimal(values[lowerIndex]), new BigDecimal(values[upperIndex])).longValue();
435+
expected = calculatePercentile(fraction, BigDecimal.valueOf(values[lowerIndex]), BigDecimal.valueOf(values[upperIndex]))
436+
.longValue();
432437
}
438+
439+
// Double*bigLong may lose precision, we allow a small range
440+
return anyOf(equalTo(Math.min(expected, expected - 1)), equalTo(expected), equalTo(Math.max(expected, expected + 1)));
433441
}
434442

435443
if (rawValues.get(0) instanceof Double) {
436444
var values = rawValues.stream().mapToDouble(Number::doubleValue).sorted().toArray();
445+
double expected;
437446

438447
if (percentile == 0) {
439-
return values[0];
448+
expected = values[0];
440449
} else if (percentile == 100) {
441-
return values[valueCount - 1];
450+
expected = values[valueCount - 1];
442451
} else {
443452
assert lowerIndex >= 0 && upperIndex < valueCount;
444-
return calculatePercentile(fraction, new BigDecimal(values[lowerIndex]), new BigDecimal(values[upperIndex])).doubleValue();
453+
expected = calculatePercentile(fraction, new BigDecimal(values[lowerIndex]), new BigDecimal(values[upperIndex]))
454+
.doubleValue();
445455
}
456+
457+
return closeTo(expected, Math.abs(expected * 0.0000001));
446458
}
447459

448460
throw new IllegalArgumentException("Unsupported type: " + rawValues.get(0).getClass());
449461
}
450462

451463
private static BigDecimal calculatePercentile(double fraction, BigDecimal lowerValue, BigDecimal upperValue) {
452-
return lowerValue.add(new BigDecimal(fraction).multiply(upperValue.subtract(lowerValue)));
464+
var difference = upperValue.subtract(lowerValue);
465+
return lowerValue.add(new BigDecimal(fraction).multiply(difference));
453466
}
454467

455468
private static TestCaseSupplier.TypedData percentileWithType(Number value, DataType type) {

0 commit comments

Comments
 (0)