Closed lukaseder closed 3 years ago
This improvement breaks the following test:
assertEquals(Optional.of("b"), Stream.of("a", "b").collect(percentileBy(1.0, String::length)));
It now returns "a"
instead of "b"
. While this may be an unspecified implementation detail, users might have come to rely on the status quo. Let's be careful here.
It now returns "a" instead of "b".
Modifying the comparator can solve this problem: If two items are equal in value, force the output of the comparator to be minus. Then items that appear earlier in the input are more likely to appear earlier in the seq.
Modifying the comparator can solve this problem: If two items are equal in value, force the output of the comparator to be minus. Then items that appear earlier in the input are more likely to appear earlier in the seq.
As commented in the PR, this is an invalid technique. A Comparator
contract mandates that if a < b
, then b > a
. Your implementation violates this for a == b
cases, where a random operand is smaller than the other based on some observed fact (notably that this seems to have worked for our tests, but depending on the sort algorithm, it might as well not work).
Th
0.0
percentile corresponds tomin()
and the1.0
percentile corresponds tomax()
. We could implement this shortcut to improve the performance of these calculations, which can be performed inO(N)
(without a sort), rather thanO(N log N)
See also: https://github.com/jOOQ/jOOL/pull/372