Unit test for very large percentile aggs #36122
Conversation
cbuescher
added
>test
|
Pinging @elastic/es-analytics-geo |
polyfractal
left a comment
polyfractal
left a comment
There was a problem hiding this comment.
Left a few style comments but otherwise LGTM!
I didn't really review the actual test logic since it's a port and I'm assuming Colin knew what he was doing at the time :)
| @@ -0,0 +1,35 @@ | |||
| package org.elasticsearch.search.aggregations.metrics; | |||
There was a problem hiding this comment.
Missing license header, will make checkstyle unhappy :)
| double prev = Double.NEGATIVE_INFINITY; | ||
| for (double q : quantiles) { | ||
| final double v = digest.quantile(q); | ||
| System.out.println("q=" + q + ", v=" + v); |
There was a problem hiding this comment.
Probably better to use a logger instead of printing directly to console. Logged under debug or trace I would guess?
| import java.util.Arrays; | ||
|
|
||
| public class TDigestStateTests extends ESTestCase { | ||
| @Test |
There was a problem hiding this comment.
Doesn't really matter, but I don't think you need the test annotation here. I believe our tests are setup to just run anything that starts with "test". (Looks like this is just a carry-over from how the TDigest library tests things)
not-napoleon
deleted the
fix/19528-large-percentile-aggregation-anomaly
branch
5 participants
This ports in a unit test for #19528 (which is really just the unit test @colings86 submitted to the upstream library last year, retouched for our randomized testing framework). As far as I can tell, this issue has been fixed since we migrated to t-digest 3.2, and this test should just pass. My recommendation is to include the test in our suite for regression testing, and close #19528