Can someone explain me in simple terms, why does this code throw an exception, "Comparison method violates its general contract!", and how do I fix it?
private int compareParents(Foo s1, Foo s2) {
if (s1.getParent() == s2) return -1;
if (s2.getParent() == s1) return 1;
return 0;
}
Your comparator is not transitive.
Let A be the parent of B, and B be the parent of C. Since A > B and B > C, then it must be the case that A > C. However, if your comparator is invoked on A and C, it would return zero, meaning A == C. This violates the contract and hence throws the exception.
It's rather nice of the library to detect this and let you know, rather than behave erratically.
One way to satisfy the transitivity requirement in compareParents() is to traverse the getParent() chain instead of only looking at the immediate ancestor.
java.util.Arrays.sort #7850039 –
Unspoiled Arrays.sort(files, (a, b) -> (int) (a.lastModified() - b.lastModified())) –
Babirusa Just because this is what I got when I Googled this error, my problem was that I had
if (value < other.value)
return -1;
else if (value >= other.value)
return 1;
else
return 0;
the value >= other.value should (obviously) actually be value > other.value so that you can actually return 0 with equal objects.
value is a NaN (if value is a double or float), it would fail as well. –
Histolysis The violation of the contract often means that the comparator is not providing the correct or consistent value when comparing objects. For example, you might want to perform a string compare and force empty strings to sort to the end with:
if ( one.length() == 0 ) {
return 1; // empty string sorts last
}
if ( two.length() == 0 ) {
return -1; // empty string sorts last
}
return one.compareToIgnoreCase( two );
But this overlooks the case where BOTH one and two are empty - and in that case, the wrong value is returned (1 instead of 0 to show a match), and the comparator reports that as a violation. It should have been written as:
if ( one.length() == 0 ) {
if ( two.length() == 0 ) {
return 0; // BOth empty - so indicate
}
return 1; // empty string sorts last
}
if ( two.length() == 0 ) {
return -1; // empty string sorts last
}
return one.compareToIgnoreCase( two );
Even if your compareTo is holds transitivity in theory, sometimes subtle bugs mess things up... such as floating point arithmetic error. It happened to me. this was my code:
public int compareTo(tfidfContainer compareTfidf) {
//descending order
if (this.tfidf > compareTfidf.tfidf)
return -1;
else if (this.tfidf < compareTfidf.tfidf)
return 1;
else
return 0;
}
The transitive property clearly holds, but for some reason I was getting the IllegalArgumentException. And it turns out that due to tiny errors in floating point arithmetic, the round-off errors where causing the transitive property to break where they shouldn't! So I rewrote the code to consider really tiny differences 0, and it worked:
public int compareTo(tfidfContainer compareTfidf) {
//descending order
if ((this.tfidf - compareTfidf.tfidf) < .000000001)
return 0;
if (this.tfidf > compareTfidf.tfidf)
return -1;
else if (this.tfidf < compareTfidf.tfidf)
return 1;
return 0;
}
Double.compare –
Satterfield In our case were were getting this error because we had accidentally flipped the order of comparison of s1 and s2. So watch out for that. It was obviously way more complicated than the following but this is an illustration:
s1 == s2
return 0;
s2 > s1
return 1;
s1 < s2
return -1;
Editing VM Configuration worked for me.
-Djava.util.Arrays.useLegacyMergeSort=true
- a the start of the proposed solution. Maybe you intended something like a one-item bullet point list instead. –
Valued In my case I was doing something like the following:
if (a.someField == null) {
return 1;
}
if (b.someField == null) {
return -1;
}
if (a.someField.equals(b.someField)) {
return a.someOtherField.compareTo(b.someOtherField);
}
return a.someField.compareTo(b.someField);
What I forgot to check was when both a.someField and b.someField are null.
I've seen this happen in a piece of code where the often recurring check for null values was performed:
if(( A==null ) && ( B==null )
return +1;//WRONG: two null values should return 0!!!
Java does not check consistency in a strict sense, only notifies you if it runs into serious trouble. Also it does not give you much information from the error.
I was puzzled with what's happening in my sorter and made a strict consistencyChecker, maybe this will help you:
/**
* @param dailyReports
* @param comparator
*/
public static <T> void checkConsitency(final List<T> dailyReports, final Comparator<T> comparator) {
final Map<T, List<T>> objectMapSmallerOnes = new HashMap<T, List<T>>();
iterateDistinctPairs(dailyReports.iterator(), new IPairIteratorCallback<T>() {
/**
* @param o1
* @param o2
*/
@Override
public void pair(T o1, T o2) {
final int diff = comparator.compare(o1, o2);
if (diff < Compare.EQUAL) {
checkConsistency(objectMapSmallerOnes, o1, o2);
getListSafely(objectMapSmallerOnes, o2).add(o1);
} else if (Compare.EQUAL < diff) {
checkConsistency(objectMapSmallerOnes, o2, o1);
getListSafely(objectMapSmallerOnes, o1).add(o2);
} else {
throw new IllegalStateException("Equals not expected?");
}
}
});
}
/**
* @param objectMapSmallerOnes
* @param o1
* @param o2
*/
static <T> void checkConsistency(final Map<T, List<T>> objectMapSmallerOnes, T o1, T o2) {
final List<T> smallerThan = objectMapSmallerOnes.get(o1);
if (smallerThan != null) {
for (final T o : smallerThan) {
if (o == o2) {
throw new IllegalStateException(o2 + " cannot be smaller than " + o1 + " if it's supposed to be vice versa.");
}
checkConsistency(objectMapSmallerOnes, o, o2);
}
}
}
/**
* @param keyMapValues
* @param key
* @param <Key>
* @param <Value>
* @return List<Value>
*/
public static <Key, Value> List<Value> getListSafely(Map<Key, List<Value>> keyMapValues, Key key) {
List<Value> values = keyMapValues.get(key);
if (values == null) {
keyMapValues.put(key, values = new LinkedList<Value>());
}
return values;
}
/**
* @author Oku
*
* @param <T>
*/
public interface IPairIteratorCallback<T> {
/**
* @param o1
* @param o2
*/
void pair(T o1, T o2);
}
/**
*
* Iterates through each distinct unordered pair formed by the elements of a given iterator
*
* @param it
* @param callback
*/
public static <T> void iterateDistinctPairs(final Iterator<T> it, IPairIteratorCallback<T> callback) {
List<T> list = Convert.toMinimumArrayList(new Iterable<T>() {
@Override
public Iterator<T> iterator() {
return it;
}
});
for (int outerIndex = 0; outerIndex < list.size() - 1; outerIndex++) {
for (int innerIndex = outerIndex + 1; innerIndex < list.size(); innerIndex++) {
callback.pair(list.get(outerIndex), list.get(innerIndex));
}
}
}
Compare, Convert (and potentially others) are not defined. Please update the code sniplet with a self-contained example. –
Leavitt checkConsi(s)tency and remove all redundant @param declarations to make the code more readable. –
Contemporary If compareParents(s1, s2) == -1 then compareParents(s2, s1) == 1 is expected. With your code it's not always true.
Specifically if s1.getParent() == s2 && s2.getParent() == s1.
It's just one of the possible problems.
In my case, it was an infinite sort. That is, at first the line moved up according to the condition, and then the same line moved down to the same place. I added one more condition at the end that unambiguously established the order of the lines.
You can't compare object data like this:s1.getParent() == s2 - this will compare the object references. You should override equals function for Foo class and then compare them like this s1.getParent().equals(s2)
So The main reason this happens is when the compare or compareTo method is not following the basic contract of comparison which is transitivity.
`If A>B And B>C then A has to be greater than A has to greater than C`
Please don't directly jump at using
` -Djava.util.Arrays.useLegacyMergeSort=true `
It will make your sorting logic buggy.
`
Foo s1 = new Foo();
Foo s2 = new Foo();
Foo s3 = new Foo();
s1.setParent(s2);
s2.setParent(s3);
s3.setParent(s1);
compareParents(s1, s2); // returns -1
compareParents(s2, s3); // returns 1
compareParents(s1, s3); // returns 1
`
In these cases your compareParents method will break the transitive property.
The short explanation for the error is: A comparator suitable for sorting has to return whether one element is greater, smaller or equal to the second.
Your comparator, however, returns whether one is parent of, child of or totally unrelated to the other. Which is something completely different. Especially 'unrelated' is something completely different than 'equal' and a 4th comparison result (which is not supported by the comparator interface or the sorting algorithms)
Now for a simple 'bubblesort' sorting algorithm, it may lead to a result, as 'unrelated' can be assumed to mean 'equal' in the sense of 'no need to switch the two entries'. However, different runs on the same list with a different starting order may lead to a different sorting result. Because while two elements may be unrelated, a third one may very well be related to one or even both of them and slip between them, or below one or behind the other, depending on the order the comparisons are done. For other sorting algorithms, this may lead to endless recursion (and if this is detected, an exception may be thrown or the program may crash or whatever.)
For sorting by parent/child relation, you'll have to implement your own sorting algorithm.
I faced the same issue and I solved it.
//This this your code
private int compareParents(Foo s1, Foo s2) {
if (s1.getParent() == s2) return -1;
if (s2.getParent() == s1) return 1;
return 0;
}
The violation is comparing different things with each other.
//acceptable
compare between s1.getParent() and s2.getParent()
//acceptable
compare between s1 and s2
//NOT acceptable
compare between s1 and s2.getParent()
//NOT acceptable
compare between s1.getParent() and s2
In my code, I wanted to sort addresses by their coordination. In the comparator, I compared between X and Y (by mistake), instead of X and X.
//My code:
private void sortBasedOnX(){
//addresses is a list of addresses where each address has X and Y
addresses.sort((o1, o2) -> {
String a = o1.getAddress().getX();
String b = o2.getAddress().getY(); //<-- this is supposed to be getX
return Integer.parseInt(a)-Integer.parseInt(b);
});
}
//acceptable
compare between o1.getAddress().getX() and o1.getAddress().getX()
//acceptable
compare between o1.getAddress().getY() and o1.getAddress().getY()
//NOT acceptable
compare between o1.getAddress().getX() and o1.getAddress().getY()
//NOT acceptable
compare between o1.getAddress().getX() and o1.getAddress()
//NOT acceptable
compare between o1.getAddress().getX() and o1
© 2022 - 2025 — McMap. All rights reserved.
s1.getParent().equals(s2)instead ofs1.getParent() == s2. – Probabilisms1is the parent ofs2, ands2is not the parent ofs1. ThencompareParents(s1, s2)is0, butcompareParents(s2, s1)is1. That doesn't make sense. (In addition, it's not transitive, like aix mentioned below.) – Microelectronics