Resync Geopoint hashCode/equals method

Geopoint's equals method was modified to consider two points equal if they are within a threshold. This change was done to accept round-off error introduced from GeoHash encoding methods. This commit removes this trappy leniency from the GeoPoint equals method and instead forces round-off error to be handled at the encoding source.

core/src/main/java/org/elasticsearch/common/geo/GeoPoint.java

@@ -30,8 +30,7 @@ public final class GeoPoint {
 
     private double lat;
     private double lon;
-    private final static double TOLERANCE = XGeoUtils.TOLERANCE;
-    
+
     public GeoPoint() {
     }
 
@@ -126,14 +125,10 @@ public final class GeoPoint {
         if (this == o) return true;
         if (o == null || getClass() != o.getClass()) return false;
 
-        final GeoPoint geoPoint = (GeoPoint) o;
-        final double lonCompare = geoPoint.lon - lon;
-        final double latCompare = geoPoint.lat - lat;
+        GeoPoint geoPoint = (GeoPoint) o;
 
-        if ((lonCompare < -TOLERANCE || lonCompare > TOLERANCE)
-                || (latCompare < -TOLERANCE || latCompare > TOLERANCE)) {
-            return false;
-        }
+        if (Double.compare(geoPoint.lat, lat) != 0) return false;
+        if (Double.compare(geoPoint.lon, lon) != 0) return false;
 
         return true;
     }

core/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java

@@ -19,7 +19,6 @@
 
 package org.elasticsearch.index.search.geo;
 
-
 import org.apache.lucene.util.XGeoHashUtils;
 import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.geo.GeoPoint;
@@ -28,6 +27,7 @@ import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
 import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.test.geo.RandomGeoGenerator;
 import org.junit.Test;
 
 import java.io.IOException;
@@ -36,9 +36,7 @@ import static org.hamcrest.Matchers.closeTo;
 
 
 public class GeoPointParsingTests  extends ESTestCase {
-
-    // mind geohash precision and error
-    private static final double ERROR = 0.00001d;
+    static double TOLERANCE = 1E-5;
 
     @Test
     public void testGeoPointReset() throws IOException {
@@ -46,36 +44,66 @@ public class GeoPointParsingTests  extends ESTestCase {
         double lon = 1 + randomDouble() * 179;
 
         GeoPoint point = new GeoPoint(0, 0);
-        assertCloseTo(point, 0, 0);
-
-        assertCloseTo(point.reset(lat, lon), lat, lon);
-        assertCloseTo(point.reset(0, 0), 0, 0);
-        assertCloseTo(point.resetLat(lat), lat, 0);
-        assertCloseTo(point.resetLat(0), 0, 0);
-        assertCloseTo(point.resetLon(lon), 0, lon);
-        assertCloseTo(point.resetLon(0), 0, 0);
+        GeoPoint point2 = new GeoPoint(0, 0);
+        assertPointsEqual(point, point2);
+
+        assertPointsEqual(point.reset(lat, lon), point2.reset(lat, lon));
+        assertPointsEqual(point.reset(0, 0), point2.reset(0, 0));
+        assertPointsEqual(point.resetLat(lat), point2.reset(lat, 0));
+        assertPointsEqual(point.resetLat(0), point2.reset(0, 0));
+        assertPointsEqual(point.resetLon(lon), point2.reset(0, lon));
+        assertPointsEqual(point.resetLon(0), point2.reset(0, 0));
         assertCloseTo(point.resetFromGeoHash(XGeoHashUtils.stringEncode(lon, lat)), lat, lon);
-        assertCloseTo(point.reset(0, 0), 0, 0);
-        assertCloseTo(point.resetFromString(Double.toString(lat) + ", " + Double.toHexString(lon)), lat, lon);
-        assertCloseTo(point.reset(0, 0), 0, 0);
+        assertPointsEqual(point.reset(0, 0), point2.reset(0, 0));
+        assertPointsEqual(point.resetFromString(Double.toString(lat) + ", " + Double.toHexString(lon)), point2.reset(lat, lon));
+        assertPointsEqual(point.reset(0, 0), point2.reset(0, 0));
     }
-    
+
+    @Test
+    public void testEqualsHashCodeContract() {
+        // generate a random geopoint
+        final GeoPoint x = RandomGeoGenerator.randomPoint(random());
+        final GeoPoint y = new GeoPoint(x.lat(), x.lon());
+        final GeoPoint z = new GeoPoint(y.lat(), y.lon());
+        // GeoPoint doesn't care about coordinate system bounds, this simply validates inequality
+        final GeoPoint a = new GeoPoint(x.lat() + randomIntBetween(1, 5), x.lon() + randomIntBetween(1, 5));
+
+        /** equality test */
+        // reflexive
+        assertTrue(x.equals(x));
+        // symmetry
+        assertTrue(x.equals(y));
+        // transitivity
+        assertTrue(y.equals(z));
+        assertTrue(x.equals(z));
+        // inequality
+        assertFalse(x.equals(a));
+
+        /** hashCode test */
+        // symmetry
+        assertTrue(x.hashCode() == y.hashCode());
+        // transitivity
+        assertTrue(y.hashCode() == z.hashCode());
+        assertTrue(x.hashCode() == z.hashCode());
+        // inequality
+        assertFalse(x.hashCode() == a.hashCode());
+    }
+
     @Test
     public void testGeoPointParsing() throws IOException {
-        double lat = randomDouble() * 180 - 90;
-        double lon = randomDouble() * 360 - 180;
-        
-        GeoPoint point = GeoUtils.parseGeoPoint(objectLatLon(lat, lon));
-        assertCloseTo(point, lat, lon);
-        
-        GeoUtils.parseGeoPoint(arrayLatLon(lat, lon), point);
-        assertCloseTo(point, lat, lon);
-
-        GeoUtils.parseGeoPoint(geohash(lat, lon), point);
-        assertCloseTo(point, lat, lon);
-
-        GeoUtils.parseGeoPoint(stringLatLon(lat, lon), point);
-        assertCloseTo(point, lat, lon);
+        GeoPoint randomPt = RandomGeoGenerator.randomPoint(random());
+
+        GeoPoint point = GeoUtils.parseGeoPoint(objectLatLon(randomPt.lat(), randomPt.lon()));
+        assertPointsEqual(point, randomPt);
+
+        GeoUtils.parseGeoPoint(arrayLatLon(randomPt.lat(), randomPt.lon()), point);
+        assertPointsEqual(point, randomPt);
+
+        GeoUtils.parseGeoPoint(geohash(randomPt.lat(), randomPt.lon()), point);
+        assertCloseTo(point, randomPt.lat(), randomPt.lon());
+
+        GeoUtils.parseGeoPoint(stringLatLon(randomPt.lat(), randomPt.lon()), point);
+        assertCloseTo(point, randomPt.lat(), randomPt.lon());
     }
 
     // Based on issue5390
@@ -98,7 +126,7 @@ public class GeoPointParsingTests  extends ESTestCase {
     public void testInvalidPointLatHashMix() throws IOException {
         XContentBuilder content = JsonXContent.contentBuilder();
         content.startObject();
-        content.field("lat", 0).field("geohash", XGeoHashUtils.stringEncode(0, 0));
+        content.field("lat", 0).field("geohash", XGeoHashUtils.stringEncode(0d, 0d));
         content.endObject();
 
         XContentParser parser = JsonXContent.jsonXContent.createParser(content.bytes());
@@ -111,7 +139,7 @@ public class GeoPointParsingTests  extends ESTestCase {
     public void testInvalidPointLonHashMix() throws IOException {
         XContentBuilder content = JsonXContent.contentBuilder();
         content.startObject();
-        content.field("lon", 0).field("geohash", XGeoHashUtils.stringEncode(0, 0));
+        content.field("lon", 0).field("geohash", XGeoHashUtils.stringEncode(0d, 0d));
         content.endObject();
 
         XContentParser parser = JsonXContent.jsonXContent.createParser(content.bytes());
@@ -166,10 +194,15 @@ public class GeoPointParsingTests  extends ESTestCase {
         parser.nextToken();
         return parser;
     }
-    
-    public static void assertCloseTo(GeoPoint point, double lat, double lon) {
-        assertThat(point.lat(), closeTo(lat, ERROR));
-        assertThat(point.lon(), closeTo(lon, ERROR));
+
+    public static void assertPointsEqual(final GeoPoint point1, final GeoPoint point2) {
+        assertEquals(point1, point2);
+        assertEquals(point1.hashCode(), point2.hashCode());
+    }
+
+    public static void assertCloseTo(final GeoPoint point, final double lat, final double lon) {
+        assertEquals(point.lat(), lat, TOLERANCE);
+        assertEquals(point.lon(), lon, TOLERANCE);
     }
 
 }

core/src/test/java/org/elasticsearch/search/aggregations/MissingValueIT.java

@@ -41,6 +41,7 @@ import static org.elasticsearch.search.aggregations.AggregationBuilders.stats;
 import static org.elasticsearch.search.aggregations.AggregationBuilders.terms;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse;
+import static org.hamcrest.Matchers.closeTo;
 
 @ESIntegTestCase.SuiteScopeTestCase
 public class MissingValueIT extends ESIntegTestCase {
@@ -198,7 +199,8 @@ public class MissingValueIT extends ESIntegTestCase {
         SearchResponse response = client().prepareSearch("idx").addAggregation(geoCentroid("centroid").field("location").missing("2,1")).get();
         assertSearchResponse(response);
         GeoCentroid centroid = response.getAggregations().get("centroid");
-        assertEquals(new GeoPoint(1.5, 1.5), centroid.centroid());
+        GeoPoint point = new GeoPoint(1.5, 1.5);
+        assertThat(point.lat(), closeTo(centroid.centroid().lat(), 1E-5));
+        assertThat(point.lon(), closeTo(centroid.centroid().lon(), 1E-5));
     }
-
 }

core/src/test/java/org/elasticsearch/search/aggregations/metrics/AbstractGeoTestCase.java

@@ -68,6 +68,7 @@ public abstract class AbstractGeoTestCase extends ESIntegTestCase {
     protected static GeoPoint singleTopLeft, singleBottomRight, multiTopLeft, multiBottomRight, singleCentroid, multiCentroid, unmappedCentroid;
     protected static ObjectIntMap<String> expectedDocCountsForGeoHash = null;
     protected static ObjectObjectMap<String, GeoPoint> expectedCentroidsForGeoHash = null;
+    protected static final double GEOHASH_TOLERANCE = 1E-5D;
 
     @Override
     public void setupSuiteScopeCluster() throws Exception {

core/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidIT.java

@@ -84,7 +84,8 @@ public class GeoCentroidIT extends AbstractGeoTestCase {
         assertThat(geoCentroid, notNullValue());
         assertThat(geoCentroid.getName(), equalTo(aggName));
         GeoPoint centroid = geoCentroid.centroid();
-        assertThat(centroid, equalTo(singleCentroid));
+        assertThat(centroid.lat(), closeTo(singleCentroid.lat(), GEOHASH_TOLERANCE));
+        assertThat(centroid.lon(), closeTo(singleCentroid.lon(), GEOHASH_TOLERANCE));
     }
 
     @Test
@@ -99,7 +100,8 @@ public class GeoCentroidIT extends AbstractGeoTestCase {
         assertThat(geoCentroid, notNullValue());
         assertThat(geoCentroid.getName(), equalTo(aggName));
         GeoPoint centroid = geoCentroid.centroid();
-        assertThat(centroid, equalTo(singleCentroid));
+        assertThat(centroid.lat(), closeTo(singleCentroid.lat(), GEOHASH_TOLERANCE));
+        assertThat(centroid.lon(), closeTo(singleCentroid.lon(), GEOHASH_TOLERANCE));
     }
 
     @Test
@@ -122,10 +124,12 @@ public class GeoCentroidIT extends AbstractGeoTestCase {
         assertThat(geoCentroid.getName(), equalTo(aggName));
         assertThat((GeoCentroid) global.getProperty(aggName), sameInstance(geoCentroid));
         GeoPoint centroid = geoCentroid.centroid();
-        assertThat(centroid, equalTo(singleCentroid));
-        assertThat((GeoPoint) global.getProperty(aggName + ".value"), equalTo(singleCentroid));
-        assertThat((double) global.getProperty(aggName + ".lat"), closeTo(singleCentroid.lat(), 1e-5));
-        assertThat((double) global.getProperty(aggName + ".lon"), closeTo(singleCentroid.lon(), 1e-5));
+        assertThat(centroid.lat(), closeTo(singleCentroid.lat(), GEOHASH_TOLERANCE));
+        assertThat(centroid.lon(), closeTo(singleCentroid.lon(), GEOHASH_TOLERANCE));
+        assertThat(((GeoPoint) global.getProperty(aggName + ".value")).lat(), closeTo(singleCentroid.lat(), GEOHASH_TOLERANCE));
+        assertThat(((GeoPoint) global.getProperty(aggName + ".value")).lon(), closeTo(singleCentroid.lon(), GEOHASH_TOLERANCE));
+        assertThat((double) global.getProperty(aggName + ".lat"), closeTo(singleCentroid.lat(), GEOHASH_TOLERANCE));
+        assertThat((double) global.getProperty(aggName + ".lon"), closeTo(singleCentroid.lon(), GEOHASH_TOLERANCE));
     }
 
     @Test
@@ -140,7 +144,8 @@ public class GeoCentroidIT extends AbstractGeoTestCase {
         assertThat(geoCentroid, notNullValue());
         assertThat(geoCentroid.getName(), equalTo(aggName));
         GeoPoint centroid = geoCentroid.centroid();
-        assertThat(centroid, equalTo(multiCentroid));
+        assertThat(centroid.lat(), closeTo(multiCentroid.lat(), GEOHASH_TOLERANCE));
+        assertThat(centroid.lon(), closeTo(multiCentroid.lon(), GEOHASH_TOLERANCE));
     }
 
     @Test
@@ -160,7 +165,10 @@ public class GeoCentroidIT extends AbstractGeoTestCase {
             String geohash = cell.getKeyAsString();
             GeoPoint expectedCentroid = expectedCentroidsForGeoHash.get(geohash);
             GeoCentroid centroidAgg = cell.getAggregations().get(aggName);
-            assertEquals("Geohash " + geohash + " has wrong centroid ", expectedCentroid, centroidAgg.centroid());
+            assertThat("Geohash " + geohash + " has wrong centroid latitude ", expectedCentroid.lat(),
+                    closeTo(centroidAgg.centroid().lat(), GEOHASH_TOLERANCE));
+            assertThat("Geohash " + geohash + " has wrong centroid longitude", expectedCentroid.lon(),
+                    closeTo(centroidAgg.centroid().lon(), GEOHASH_TOLERANCE));
         }
     }
 }