java compareTo Comparison method violates its general contract

Question

I'm attempting to sort a collection and getting the following exception:

java.lang.IllegalArgumentException: Comparison method violates its general contract!
    at java.util.ComparableTimSort.mergeHi(ComparableTimSort.java:835)
    at java.util.ComparableTimSort.mergeAt(ComparableTimSort.java:453)
    at java.util.ComparableTimSort.mergeCollapse(ComparableTimSort.java:376)
    at java.util.ComparableTimSort.sort(ComparableTimSort.java:182)
    at java.util.ComparableTimSort.sort(ComparableTimSort.java:146)
    at java.util.Arrays.sort(Arrays.java:472)
    at java.util.Collections.sort(Collections.java:155)

I'm aware of the 3 contracts for compareTo:

x.compareTo(y)>0 && y.compareTo(z)>0) implies x.compareTo(z)>0
sgn(x.compareTo(y)) == -sgn(y.compareTo(x))
x.compareTo(y)==0 implies that sgn(x.compareTo(z)) == sgn(y.compareTo(z))

The code is below. I can see that there is potential for the Long.intValue() to be truncated, but as far as i can see this should not violate the contract.

public class Bar implements Comparable<Bar>{

private static final int LOWER = 1;
private static final int HIGHER = -1;
private boolean isNoPriority;
private int priority;
private String tradeId;
private long tradeVersion;



public Bar(boolean isNoPriority, int priority, String tradeId,long tradeVersion) {
    super();
    this.isNoPriority = isNoPriority;
    this.priority = priority;
    this.tradeId = tradeId;
    this.tradeVersion = tradeVersion;
}   
@Override
public int compareTo(Bar o) {

    if (isNoPriority && !o.isNoPriority){               
        return LOWER;
    }
    if (!isNoPriority && o.isNoPriority){
        return HIGHER;
    }

    if (priority == o.priority) {
        if (tradeId.compareToIgnoreCase(o.tradeId) == 0){                   
            return Long.valueOf(tradeVersion).intValue() - Long.valueOf(o.tradeVersion).intValue();
        }
        else {
            return tradeId.compareToIgnoreCase(o.tradeId);                              
        }
    }
    else if (priority < o.priority) {               
        return LOWER;
    }
    else if (priority > o.priority){                
        return HIGHER;          
    } else {
        return 0;
    }

}
}

I cannot understand what is incorrect about this compareTo implementation.

I'd add equals and hashCode as well. You need those, too. Follow the Joshua Bloch conventions. — duffymo, Jun 16 '14 at 14:08
@duffymo, adding stuff/puff/bloat just b/c you may use it is an anti-pattern in my book. It a key is comparably by nature it should not be used in hash-tables. — bestsss, Jun 16 '14 at 14:13
`return Long.valueOf(tradeVersion).intValue()- Long.valueOf(o.tradeVersion).intValue();` it's terrible code and also wrong. If you wish to compare long values just use `Long.compare(long, long)`. If you have to work pre java1.7. use an utility function: `static int compare(long x, long y) {return (x < y) ? -1 : ((x == y) ? 0 : 1);}` — bestsss, Jun 16 '14 at 14:16
To expand: you can't compare signed values by subtracting them. Same sign values can be compared via Long.sign(long1-long2), though. Yet converting to `int` is beyond a rooking mistake, and if you have to convert use (int), dont create a new object just to get an int. — bestsss, Jun 16 '14 at 14:17
Thanks for the help all. Part of the problem is that i've been unable to reproduce the failure in a unit test, presumably during to the various optimizations of the TimSort algorithm. I wanted to be certain of the cause and reproduce it, in the end i've just patched directly to a test environment and it's working with Long.compare(). The only case in which we can conceive of the compare being non transitive is when the difference in tradeVersions is exactly Integer.MIN_VALUE because -1 * Integer.MIN_VALUE == Integer.MIN_VALUE. The result is wrong in other cases, but still transitive. — Paul Creasey, Jun 16 '14 at 15:37

score 2 · Accepted Answer · answered Jun 16 '14 at 14:09

You have answered your own question. If one of the long values is "truncated", it potentially turns from a very large positive number to a negative number. This potentially results in returning a positive value when a negative value should be returned. It could also lead to returning 0 for 2 values which are not equal.

Instead of doing subtraction of the int values, use Long compareTo.

http://docs.oracle.com/javase/7/docs/api/java/lang/Long.html#compare(long,%20long)

java compareTo Comparison method violates its general contract

1 Answers1