summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorantirez <antirez@gmail.com>2016-12-05 14:02:32 +0100
committerantirez <antirez@gmail.com>2016-12-05 14:18:59 +0100
commit8c22086c34a3d6538387d0f08ea1db61884d03be (patch)
tree8bc6ff7ea7c16da3071d017d8bde22090a830fab
parent92958df3b1306fb787d3eeb21889ba91d6fb7168 (diff)
downloadredis-8c22086c34a3d6538387d0f08ea1db61884d03be.tar.gz
Geo: fix computation of bounding box.
A bug was reported in the context in issue #3631. The root cause of the bug was that certain neighbor boxes were zeroed after the "inside the bounding box or not" check, simply because the bounding box computation function was wrong. A few debugging infos where enhanced and moved in other parts of the code. A check to avoid steps=0 was added, but is unrelated to this issue and I did not verified it was an actual bug in practice.
-rw-r--r--deps/geohash-int/geohash_helper.c35
-rw-r--r--src/geo.c25
2 files changed, 33 insertions, 27 deletions
diff --git a/deps/geohash-int/geohash_helper.c b/deps/geohash-int/geohash_helper.c
index e7bc27055..56c6cc802 100644
--- a/deps/geohash-int/geohash_helper.c
+++ b/deps/geohash-int/geohash_helper.c
@@ -80,35 +80,18 @@ uint8_t geohashEstimateStepsByRadius(double range_meters, double lat) {
return step;
}
-int geohashBitsComparator(const GeoHashBits *a, const GeoHashBits *b) {
- /* If step not equal, compare on step. Else, compare on bits. */
- return a->step != b->step ? a->step - b->step : a->bits - b->bits;
-}
-
+/* Return the bounding box of the search area centered at latitude,longitude
+ * having a radius of radius_meter. bounds[0] - bounds[2] is the minimum
+ * and maxium longitude, while bounds[1] - bounds[3] is the minimum and
+ * maximum latitude. */
int geohashBoundingBox(double longitude, double latitude, double radius_meters,
double *bounds) {
if (!bounds) return 0;
- double lonr, latr;
- lonr = deg_rad(longitude);
- latr = deg_rad(latitude);
-
- if (radius_meters > EARTH_RADIUS_IN_METERS)
- radius_meters = EARTH_RADIUS_IN_METERS;
- double distance = radius_meters / EARTH_RADIUS_IN_METERS;
- double min_latitude = latr - distance;
- double max_latitude = latr + distance;
-
- /* Note: we're being lazy and not accounting for coordinates near poles */
- double min_longitude, max_longitude;
- double difference_longitude = asin(sin(distance) / cos(latr));
- min_longitude = lonr - difference_longitude;
- max_longitude = lonr + difference_longitude;
-
- bounds[0] = rad_deg(min_longitude);
- bounds[1] = rad_deg(min_latitude);
- bounds[2] = rad_deg(max_longitude);
- bounds[3] = rad_deg(max_latitude);
+ bounds[0] = longitude - rad_deg(radius_meters/EARTH_RADIUS_IN_METERS/cos(deg_rad(latitude)));
+ bounds[2] = longitude + rad_deg(radius_meters/EARTH_RADIUS_IN_METERS/cos(deg_rad(latitude)));
+ bounds[1] = latitude - rad_deg(radius_meters/EARTH_RADIUS_IN_METERS);
+ bounds[3] = latitude + rad_deg(radius_meters/EARTH_RADIUS_IN_METERS);
return 1;
}
@@ -161,7 +144,7 @@ GeoHashRadius geohashGetAreasByRadius(double longitude, double latitude, double
< radius_meters) decrease_step = 1;
}
- if (decrease_step) {
+ if (steps > 1 && decrease_step) {
steps--;
geohashEncode(&long_range,&lat_range,longitude,latitude,steps,&hash);
geohashNeighbors(&hash,&neighbors);
diff --git a/src/geo.c b/src/geo.c
index d997bc7e4..f2ae8c827 100644
--- a/src/geo.c
+++ b/src/geo.c
@@ -328,6 +328,7 @@ int membersOfGeoHashBox(robj *zobj, GeoHashBits hash, geoArray *ga, double lon,
int membersOfAllNeighbors(robj *zobj, GeoHashRadius n, double lon, double lat, double radius, geoArray *ga) {
GeoHashBits neighbors[9];
unsigned int i, count = 0, last_processed = 0;
+ int debugmsg = 0;
neighbors[0] = n.hash;
neighbors[1] = n.neighbors.north;
@@ -342,8 +343,26 @@ int membersOfAllNeighbors(robj *zobj, GeoHashRadius n, double lon, double lat, d
/* For each neighbor (*and* our own hashbox), get all the matching
* members and add them to the potential result list. */
for (i = 0; i < sizeof(neighbors) / sizeof(*neighbors); i++) {
- if (HASHISZERO(neighbors[i]))
+ if (HASHISZERO(neighbors[i])) {
+ if (debugmsg) D("neighbors[%d] is zero",i);
continue;
+ }
+
+ /* Debugging info. */
+ if (debugmsg) {
+ GeoHashRange long_range, lat_range;
+ geohashGetCoordRange(&long_range,&lat_range);
+ GeoHashArea myarea = {{0}};
+ geohashDecode(long_range, lat_range, neighbors[i], &myarea);
+
+ /* Dump center square. */
+ D("neighbors[%d]:\n",i);
+ D("area.longitude.min: %f\n", myarea.longitude.min);
+ D("area.longitude.max: %f\n", myarea.longitude.max);
+ D("area.latitude.min: %f\n", myarea.latitude.min);
+ D("area.latitude.max: %f\n", myarea.latitude.max);
+ D("\n");
+ }
/* When a huge Radius (in the 5000 km range or more) is used,
* adjacent neighbors can be the same, leading to duplicated
@@ -352,7 +371,11 @@ int membersOfAllNeighbors(robj *zobj, GeoHashRadius n, double lon, double lat, d
if (last_processed &&
neighbors[i].bits == neighbors[last_processed].bits &&
neighbors[i].step == neighbors[last_processed].step)
+ {
+ if (debugmsg)
+ D("Skipping processing of %d, same as previous\n",i);
continue;
+ }
count += membersOfGeoHashBox(zobj, neighbors[i], ga, lon, lat, radius);
last_processed = i;
}