Range.compareTo() violates the contract of Comparable patch by jasobrown and blambov for CASSANDRA-11216
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/ecbeb084 Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/ecbeb084 Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/ecbeb084 Branch: refs/heads/trunk Commit: ecbeb0841e8627fa74a34346e0c380061588366a Parents: 941d13d Author: Jason Brown <jasedbr...@gmail.com> Authored: Fri Feb 26 11:53:03 2016 -0800 Committer: Jason Brown <jasedbr...@gmail.com> Committed: Fri Feb 26 14:47:57 2016 -0800 ---------------------------------------------------------------------- CHANGES.txt | 1 + src/java/org/apache/cassandra/dht/Range.java | 20 +++++++++++--------- .../org/apache/cassandra/dht/RangeTest.java | 15 +++++++++++++++ 3 files changed, 27 insertions(+), 9 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/ecbeb084/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index 9162165..aa3adf5 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 2.2.6 + * Range.compareTo() violates the contract of Comparable (CASSANDRA-11216) * Avoid NPE when serializing ErrorMessage with null message (CASSANDRA-11167) * Replacing an aggregate with a new version doesn't reset INITCOND (CASSANDRA-10840) * (cqlsh) cqlsh cannot be called through symlink (CASSANDRA-11037) http://git-wip-us.apache.org/repos/asf/cassandra/blob/ecbeb084/src/java/org/apache/cassandra/dht/Range.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/dht/Range.java b/src/java/org/apache/cassandra/dht/Range.java index f2c5996..34e91ea 100644 --- a/src/java/org/apache/cassandra/dht/Range.java +++ b/src/java/org/apache/cassandra/dht/Range.java @@ -32,6 +32,8 @@ import org.apache.cassandra.utils.Pair; * A Range is responsible for the tokens between (left, right]. * * Used by the partitioner and by map/reduce by-token range scans. + * + * Note: this class has a natural ordering that is inconsistent with equals */ public class Range<T extends RingPosition<T>> extends AbstractBounds<T> implements Comparable<Range<T>>, Serializable { @@ -254,18 +256,18 @@ public class Range<T extends RingPosition<T>> extends AbstractBounds<T> implemen return left.compareTo(right) >= 0; } + /** + * Note: this class has a natural ordering that is inconsistent with equals + */ public int compareTo(Range<T> rhs) { - /* - * If the range represented by the "this" pointer - * is a wrap around then it is the smaller one. - */ - if ( isWrapAround(left, right) ) - return -1; - - if ( isWrapAround(rhs.left, rhs.right) ) - return 1; + boolean lhsWrap = isWrapAround(left, right); + boolean rhsWrap = isWrapAround(rhs.left, rhs.right); + // if one of the two wraps, that's the smaller one. + if (lhsWrap != rhsWrap) + return Boolean.compare(!lhsWrap, !rhsWrap); + // otherwise compare by right. return right.compareTo(rhs.right); } http://git-wip-us.apache.org/repos/asf/cassandra/blob/ecbeb084/test/unit/org/apache/cassandra/dht/RangeTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/dht/RangeTest.java b/test/unit/org/apache/cassandra/dht/RangeTest.java index 4255487..9fb49cf 100644 --- a/test/unit/org/apache/cassandra/dht/RangeTest.java +++ b/test/unit/org/apache/cassandra/dht/RangeTest.java @@ -661,4 +661,19 @@ public class RangeTest { return new Murmur3Partitioner.LongToken(t); } + + @Test + public void testCompareTo_SameObject_WrapAround() + { + Range<Token> range = r(10, -10); + assertEquals(0, range.compareTo(range)); + } + + @Test + public void testCompareTo_BothWrapAround() + { + Range<Token> r0 = r(10, -10); + Range<Token> r1 = r(20, -5); + assertNotSame(r0.compareTo(r1), r1.compareTo(r0)); + } }
