Skip to content

Commit f08b45a

Browse files
author
Erik Froseth
committed
Bug#22165582 ST_*FROMGEOHASH ROUNDS INCORRECTLY
Problem ======= The current implementation of Geohash decoding may yield the wrong result, due to the rounding algorithm being too aggressive. To my knowledge, there is no official Geohash specification. However, the wikipedia page on Geohash is written by the Geohash author, and is thus used as the specification. The page states the following: Final rounding should be done carefully in a way that min <= round(value) <= max This is violated by the current implementation (funfact: it is also violated by the reference implementation http://geohash.org). To illustrate this, a decoding example is given: Decode "xkcd" into a latitude value: We begin by translating "xkcd" into its binary representation: x: 11101 k: 10010 c: 01011 d: 01100 Full binary representation: 11101100100101101100. To decode the latitude value, we only use the odd bits (given that the first bit is bit number zero). Removing all the even bits, we end up with the binary value 1010011010. A bit value of '1' indicates that we should discard the lower half of the latitude range. A value of '0' indicates that we should discard the upper half of the range. This gives us the following decoding steps: Start [-90.0 90.0 ] 1 [ 0.0 90.0 ] 0 [ 0.0 45.0 ] 1 [ 22.5 45.0 ] 0 [ 22.5 33.75 ] 0 [ 22.5 28.125 ] 1 [ 25.3125 28.125 ] 1 [ 26.71875 28.125 ] 0 [ 26.71875 27.421875 ] 1 [ 27.0703125 27.421875 ] 0 [ 27.0703125 27.24609375 ] From the specification, we see that the final value should be in the rage [27.0703125, 27.24609375]. However, the current implementation ends up with rounding the value to "27.0". Both "27.1" and "27.2" would be considered a correct result. Fix === Add a while-loop to Item_func_latlongfromgeohash::round_latlongitude which ensures that the returned result is within the valid range. We also add some asserts to ensure that the returned result satisfy the Geohash rounding condition mentioned in the specification. Note that the result returned from MySQL geohash functions may not be the same results returned from http://geohash.org, due to the fact that the "reference implementation" have the same bug/flaw. Also, the bug report mentiones the following case: For some positions, you can get results that differ wildly: SELECT ST_GeoHash(ST_PointFromGeoHash('ebrb', 0), 4); +-----------------------------------------------+ | ST_GeoHash(ST_PointFromGeoHash('ebrb', 0), 4) | +-----------------------------------------------+ | s00j | +-----------------------------------------------+ With this bugfix, the result is now the following: SELECT ST_GeoHash(ST_PointFromGeoHash('ebrb', 0), 4); +-----------------------------------------------+ | ST_GeoHash(ST_PointFromGeoHash('ebrb', 0), 4) | +-----------------------------------------------+ | s020 | +-----------------------------------------------+ This is however valid, as explained below: 'ebrb' represents the bounding box with latitude [1.40625, 1.58203125] and longitude [-0.3515625, 0.0]. ST_PointFromGeoHash('ebrb') results in POINT(0.0 1.5) (which is valid). 's020' represents the bounding box with latitude [1.40625, 1.58203125] and longitude [0.0, 0.3515625]. This is also a valid representation of POINT(0.0 1.5).
1 parent 01b8302 commit f08b45a

File tree

4 files changed

+131
-61
lines changed

4 files changed

+131
-61
lines changed

‎mysql-test/suite/gis/r/geohash_functions.result

+64-46
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,10 @@ ST_LONGFROMGEOHASH("s000")
1919
0
2020
SELECT ST_LONGFROMGEOHASH("0123456789");
2121
ST_LONGFROMGEOHASH("0123456789")
22-
-179.5574
22+
-179.55743
2323
SELECT ST_LONGFROMGEOHASH("9876543210");
2424
ST_LONGFROMGEOHASH("9876543210")
25-
-107.7961
25+
-107.79609
2626
SELECT ST_LONGFROMGEOHASH("bcdefghjkmnpqrstuvwxyz");
2727
ST_LONGFROMGEOHASH("bcdefghjkmnpqrstuvwxyz")
2828
-142.6078415216915
@@ -124,7 +124,7 @@ ST_LONGFROMGEOHASH("11111111111111111111")
124124
-133.548387096774
125125
SELECT ST_LONGFROMGEOHASH("99999999999999999999");
126126
ST_LONGFROMGEOHASH("99999999999999999999")
127-
-110.322580645161
127+
-110.3225806451612
128128
SELECT ST_LONGFROMGEOHASH(NULL);
129129
ST_LONGFROMGEOHASH(NULL)
130130
NULL
@@ -241,13 +241,13 @@ NULL
241241
-158
242242
158
243243
-105.343666081393
244-
75.425880752622
245-
113.250616968746
246-
4.425328277863
247-
-113.924180563539
248-
-132.972405182428
249-
102.741796243767
250-
60.78869827399
244+
75.4258807526218
245+
113.2506169687463
246+
4.4253282778628
247+
-113.9241805635394
248+
-132.9724051824284
249+
102.7417962437673
250+
60.7886982739903
251251
128.908488386749
252252
-37.292723368146
253253
# valid characters
@@ -271,10 +271,10 @@ ST_LATFROMGEOHASH("s000")
271271
0
272272
SELECT ST_LATFROMGEOHASH("0123456789");
273273
ST_LATFROMGEOHASH("0123456789")
274-
-82.77451
274+
-82.774507
275275
SELECT ST_LATFROMGEOHASH("9876543210");
276276
ST_LATFROMGEOHASH("9876543210")
277-
1.77017
277+
1.770175
278278
SELECT ST_LATFROMGEOHASH("bcdefghjkmnpqrstuvwxyz");
279279
ST_LATFROMGEOHASH("bcdefghjkmnpqrstuvwxyz")
280280
54.11408847964353
@@ -370,13 +370,13 @@ ST_LATFROMGEOHASH(CAST(100 AS CHAR))
370370
-89
371371
SELECT ST_LATFROMGEOHASH("10111000110001111001");
372372
ST_LATFROMGEOHASH("10111000110001111001")
373-
-89.824213380179
373+
-89.8242133801793
374374
SELECT ST_LATFROMGEOHASH("11111111111111111111");
375375
ST_LATFROMGEOHASH("11111111111111111111")
376-
-84.193548387097
376+
-84.1935483870967
377377
SELECT ST_LATFROMGEOHASH("99999999999999999999");
378378
ST_LATFROMGEOHASH("99999999999999999999")
379-
8.709677419355
379+
8.7096774193548
380380
SELECT ST_LATFROMGEOHASH(NULL);
381381
ST_LATFROMGEOHASH(NULL)
382382
NULL
@@ -469,19 +469,19 @@ ST_LATFROMGEOHASH(hash_value)
469469
NULL
470470
1
471471
-0.1
472-
0
472+
0.4
473473
-68
474474
68
475-
-27.337489936745
476-
37.434775233497
475+
-27.3374899367448
476+
37.4347752334972
477477
82.269670295412
478478
-74.168840669129
479-
23.969616151467
480-
-45.585271892424
479+
23.9696161514665
480+
-45.5852718924236
481481
29.763254995923
482-
83.760208251415
483-
31.870030535455
484-
19.707461873161
482+
83.7602082514146
483+
31.8700305354552
484+
19.7074618731612
485485
# valid characters
486486
SELECT ST_ASTEXT(ST_POINTFROMGEOHASH("0", 0));
487487
ST_ASTEXT(ST_POINTFROMGEOHASH("0", 0))
@@ -503,10 +503,10 @@ ST_ASTEXT(ST_POINTFROMGEOHASH("s000", 10000))
503503
POINT(0 0)
504504
SELECT ST_ASTEXT(ST_POINTFROMGEOHASH("0123456789", 100000));
505505
ST_ASTEXT(ST_POINTFROMGEOHASH("0123456789", 100000))
506-
POINT(-179.5574 -82.77451)
506+
POINT(-179.55743 -82.774507)
507507
SELECT ST_ASTEXT(ST_POINTFROMGEOHASH("9876543210", 1000000));
508508
ST_ASTEXT(ST_POINTFROMGEOHASH("9876543210", 1000000))
509-
POINT(-107.7961 1.77017)
509+
POINT(-107.79609 1.770175)
510510
SELECT ST_ASTEXT(ST_POINTFROMGEOHASH("bcdefghjkmnpqrstuvwxyz", 1));
511511
ST_ASTEXT(ST_POINTFROMGEOHASH("bcdefghjkmnpqrstuvwxyz", 1))
512512
POINT(-142.6078415216915 54.11408847964353)
@@ -606,13 +606,13 @@ ST_ASTEXT(ST_POINTFROMGEOHASH(CAST(100 AS CHAR), 1))
606606
POINT(-134 -89)
607607
SELECT ST_ASTEXT(ST_POINTFROMGEOHASH("10111000110001111001", "0"));
608608
ST_ASTEXT(ST_POINTFROMGEOHASH("10111000110001111001", "0"))
609-
POINT(-133.549761770805 -89.824213380179)
609+
POINT(-133.549761770805 -89.8242133801793)
610610
SELECT ST_ASTEXT(ST_POINTFROMGEOHASH("11111111111111111111", "1234567890"));
611611
ST_ASTEXT(ST_POINTFROMGEOHASH("11111111111111111111", "1234567890"))
612-
POINT(-133.548387096774 -84.193548387097)
612+
POINT(-133.548387096774 -84.1935483870967)
613613
SELECT ST_ASTEXT(ST_POINTFROMGEOHASH("99999999999999999999", "4294967295"));
614614
ST_ASTEXT(ST_POINTFROMGEOHASH("99999999999999999999", "4294967295"))
615-
POINT(-110.322580645161 8.709677419355)
615+
POINT(-110.3225806451612 8.7096774193548)
616616
SELECT ST_ASTEXT(ST_POINTFROMGEOHASH("00000000000000000000", " ***** "));
617617
ST_ASTEXT(ST_POINTFROMGEOHASH("00000000000000000000", " ***** "))
618618
POINT(-180 -90)
@@ -768,19 +768,19 @@ POINT(180 90)
768768
NULL
769769
POINT(1 1)
770770
POINT(-0.1 -0.1)
771-
POINT(1 0)
771+
POINT(1 0.4)
772772
POINT(-158 -68)
773773
POINT(158 68)
774-
POINT(-105.343666081393 -27.337489936745)
775-
POINT(75.425880752622 37.434775233497)
776-
POINT(113.250616968746 82.269670295412)
777-
POINT(4.425328277863 -74.168840669129)
778-
POINT(-113.924180563539 23.969616151467)
779-
POINT(-132.972405182428 -45.585271892424)
780-
POINT(102.741796243767 29.763254995923)
781-
POINT(60.78869827399 83.760208251415)
782-
POINT(128.908488386749 31.870030535455)
783-
POINT(-37.292723368146 19.707461873161)
774+
POINT(-105.343666081393 -27.3374899367448)
775+
POINT(75.4258807526218 37.4347752334972)
776+
POINT(113.2506169687463 82.269670295412)
777+
POINT(4.4253282778628 -74.168840669129)
778+
POINT(-113.9241805635394 23.9696161514665)
779+
POINT(-132.9724051824284 -45.5852718924236)
780+
POINT(102.7417962437673 29.763254995923)
781+
POINT(60.7886982739903 83.7602082514146)
782+
POINT(128.908488386749 31.8700305354552)
783+
POINT(-37.292723368146 19.7074618731612)
784784
# Test create table from SELECT statement
785785
CREATE TABLE t1 AS SELECT ST_POINTFROMGEOHASH("0123", 4326);
786786
EXPLAIN t1;
@@ -1353,7 +1353,7 @@ ST_ASTEXT(ST_POINTFROMGEOHASH(ST_GEOHASH(ST_GEOMFROMTEXT('POINT(-1.000 -1.1010)'
13531353
POINT(-1 -1.101)
13541354
SELECT ST_ASTEXT(ST_POINTFROMGEOHASH(ST_GEOHASH(ST_GEOMFROMTEXT('POINT(1.00101 1.000)'),10),1000000));
13551355
ST_ASTEXT(ST_POINTFROMGEOHASH(ST_GEOHASH(ST_GEOMFROMTEXT('POINT(1.00101 1.000)'),10),1000000))
1356-
POINT(1.001 1)
1356+
POINT(1.00101 1)
13571357
SELECT ST_ASTEXT(ST_POINTFROMGEOHASH(ST_GEOHASH(ST_GEOMFROMTEXT('POINT(-1.00101 -1.0000)'),20),10000000));
13581358
ST_ASTEXT(ST_POINTFROMGEOHASH(ST_GEOHASH(ST_GEOMFROMTEXT('POINT(-1.00101 -1.0000)'),20),10000000))
13591359
POINT(-1.00101 -1)
@@ -1598,33 +1598,33 @@ SET @geohash = "01bbgcee";
15981598
PREPARE stmt FROM "SELECT ST_LONGFROMGEOHASH(?)";
15991599
EXECUTE stmt USING @geohash;
16001600
ST_LONGFROMGEOHASH(?)
1601-
-178.776
1601+
-178.7755
16021602
DEALLOCATE PREPARE stmt;
16031603
SELECT ST_LONGFROMGEOHASH(@geohash);
16041604
ST_LONGFROMGEOHASH(@geohash)
1605-
-178.776
1605+
-178.7755
16061606
SELECT ST_LONGFROMGEOHASH(@null);
16071607
ST_LONGFROMGEOHASH(@null)
16081608
NULL
16091609
PREPARE stmt FROM "SELECT ST_LATFROMGEOHASH(?)";
16101610
EXECUTE stmt USING @geohash;
16111611
ST_LATFROMGEOHASH(?)
1612-
-80.016
1612+
-80.0156
16131613
DEALLOCATE PREPARE stmt;
16141614
SELECT ST_LATFROMGEOHASH(@geohash);
16151615
ST_LATFROMGEOHASH(@geohash)
1616-
-80.016
1616+
-80.0156
16171617
SELECT ST_LATFROMGEOHASH(@null);
16181618
ST_LATFROMGEOHASH(@null)
16191619
NULL
16201620
PREPARE stmt FROM "SELECT ST_ASTEXT(ST_POINTFROMGEOHASH(?, 1))";
16211621
EXECUTE stmt USING @geohash;
16221622
ST_ASTEXT(ST_POINTFROMGEOHASH(?, 1))
1623-
POINT(-178.776 -80.016)
1623+
POINT(-178.7755 -80.0156)
16241624
DEALLOCATE PREPARE stmt;
16251625
SELECT ST_ASTEXT(ST_POINTFROMGEOHASH(@geohash, 1));
16261626
ST_ASTEXT(ST_POINTFROMGEOHASH(@geohash, 1))
1627-
POINT(-178.776 -80.016)
1627+
POINT(-178.7755 -80.0156)
16281628
SET @srid = 4326;
16291629
PREPARE stmt FROM "SELECT ST_ASTEXT(ST_POINTFROMGEOHASH(\"00\", ?))";
16301630
EXECUTE stmt USING @srid;
@@ -1719,3 +1719,21 @@ ERROR HY000: Incorrect geohash value: 'guqtjvooguqtjvoo' for function st_pointfr
17191719
# Clean up
17201720
DROP TABLE geohashes;
17211721
DROP TABLE t1;
1722+
#
1723+
# Bug#22165582 ST_*FROMGEOHASH ROUNDS INCORRECTLY
1724+
#
1725+
SELECT ST_GeoHash(ST_PointFromGeoHash('xkcd', 0), 4);
1726+
ST_GeoHash(ST_PointFromGeoHash('xkcd', 0), 4)
1727+
xkcd
1728+
SELECT ST_LongFromGeoHash('xkcd');
1729+
ST_LongFromGeoHash('xkcd')
1730+
148.5
1731+
SELECT ST_LatFromGeoHash('xkcd');
1732+
ST_LatFromGeoHash('xkcd')
1733+
27.2
1734+
SELECT ST_GeoHash(ST_PointFromGeoHash('ebrb', 0), 4);
1735+
ST_GeoHash(ST_PointFromGeoHash('ebrb', 0), 4)
1736+
s020
1737+
SELECT ST_LatFromGeoHash('m7s9pyctu9bbwqkgbw5x6vutzkztd9szjh86gmz9w9nsz6792d');
1738+
ST_LatFromGeoHash('m7s9pyctu9bbwqkgbw5x6vutzkztd9szjh86gmz9w9nsz6792d')
1739+
-25.098643334722453

‎mysql-test/suite/gis/t/geohash_functions.test

+10
Original file line numberDiff line numberDiff line change
@@ -1453,3 +1453,13 @@ SELECT ST_POINTFROMGEOHASH(a, 1) FROM t1;
14531453
DROP TABLE geohashes;
14541454

14551455
DROP TABLE t1;
1456+
1457+
1458+
--echo #
1459+
--echo # Bug#22165582 ST_*FROMGEOHASH ROUNDS INCORRECTLY
1460+
--echo #
1461+
SELECT ST_GeoHash(ST_PointFromGeoHash('xkcd', 0), 4);
1462+
SELECT ST_LongFromGeoHash('xkcd');
1463+
SELECT ST_LatFromGeoHash('xkcd');
1464+
SELECT ST_GeoHash(ST_PointFromGeoHash('ebrb', 0), 4);
1465+
SELECT ST_LatFromGeoHash('m7s9pyctu9bbwqkgbw5x6vutzkztd9szjh86gmz9w9nsz6792d');

‎sql/item_func.cc

+55-14
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
#include "sql_time.h" // TIME_from_longlong_packed
4545
#include "strfunc.h" // find_type
4646
#include "item_json_func.h" // Item_func_json_quote
47+
#include <cfloat> // DBL_DIG
4748

4849
using std::min;
4950
using std::max;
@@ -2602,7 +2603,7 @@ Item_func_latlongfromgeohash::decode_geohash(String *geohash,
26022603
double *result_latitude,
26032604
double *result_longitude)
26042605
{
2605-
double latitiude_accuracy= (upper_latitude - lower_latitude) / 2.0;
2606+
double latitude_accuracy= (upper_latitude - lower_latitude) / 2.0;
26062607
double longitude_accuracy= (upper_longitude - lower_longitude) / 2.0;
26072608

26082609
double latitude_value= (upper_latitude + lower_latitude) / 2.0;
@@ -2612,7 +2613,7 @@ Item_func_latlongfromgeohash::decode_geohash(String *geohash,
26122613
uint input_length= geohash->length();
26132614

26142615
for (uint i= 0;
2615-
i < input_length && latitiude_accuracy > 0.0 && longitude_accuracy > 0.0;
2616+
i < input_length && latitude_accuracy > 0.0 && longitude_accuracy > 0.0;
26162617
i++)
26172618
{
26182619
char input_character= my_tolower(&my_charset_latin1, (*geohash)[i]);
@@ -2665,12 +2666,12 @@ Item_func_latlongfromgeohash::decode_geohash(String *geohash,
26652666
}
26662667
else
26672668
{
2668-
latitiude_accuracy/= 2.0;
2669+
latitude_accuracy/= 2.0;
26692670

26702671
if (converted_character & (1 << bit_number))
2671-
latitude_value+= latitiude_accuracy;
2672+
latitude_value+= latitude_accuracy;
26722673
else
2673-
latitude_value-= latitiude_accuracy;
2674+
latitude_value-= latitude_accuracy;
26742675
}
26752676

26762677
number_of_bits_used++;
@@ -2683,9 +2684,26 @@ Item_func_latlongfromgeohash::decode_geohash(String *geohash,
26832684
}
26842685

26852686
*result_latitude= round_latlongitude(latitude_value,
2686-
latitiude_accuracy * 2.0);
2687+
latitude_accuracy * 2.0,
2688+
latitude_value - latitude_accuracy,
2689+
latitude_value + latitude_accuracy);
26872690
*result_longitude= round_latlongitude(longitude_value,
2688-
longitude_accuracy * 2.0);
2691+
longitude_accuracy * 2.0,
2692+
longitude_value - longitude_accuracy,
2693+
longitude_value + longitude_accuracy);
2694+
2695+
/*
2696+
Ensure that the rounded results are not ouside of the valid range. As
2697+
written in the specification:
2698+
2699+
Final rounding should be done carefully in a way that
2700+
min <= round(value) <= max
2701+
*/
2702+
DBUG_ASSERT(latitude_value - latitude_accuracy <= *result_latitude);
2703+
DBUG_ASSERT(*result_latitude <= latitude_value + latitude_accuracy);
2704+
2705+
DBUG_ASSERT(longitude_value - longitude_accuracy <= *result_longitude);
2706+
DBUG_ASSERT(*result_longitude <= longitude_value + longitude_accuracy);
26892707

26902708
return false;
26912709
}
@@ -2699,33 +2717,56 @@ Item_func_latlongfromgeohash::decode_geohash(String *geohash,
26992717
(e.g upper value of 45.0 and a lower value of 22.5, gives an error range of
27002718
22.5).
27012719
2720+
The returned result will always be in the range [lower_limit, upper_limit]
2721+
27022722
@param latlongitude The latitude or longitude to round.
27032723
@param error_range The total error range of the calculated laglongitude.
2724+
@param lower_limit Lower limit of the returned result.
2725+
@param upper_limit Upper limit of the returned result.
27042726
27052727
@return A rounded latitude or longitude.
27062728
*/
27072729
double Item_func_latlongfromgeohash::round_latlongitude(double latlongitude,
2708-
double error_range)
2730+
double error_range,
2731+
double lower_limit,
2732+
double upper_limit)
27092733
{
2734+
// Ensure that we don't start with an impossible case to solve.
2735+
DBUG_ASSERT(lower_limit <= latlongitude);
2736+
DBUG_ASSERT(upper_limit >= latlongitude);
2737+
27102738
if (error_range == 0.0)
27112739
{
27122740
return latlongitude;
27132741
}
27142742
else
27152743
{
27162744
uint number_of_decimals= 0;
2717-
while (error_range < 0.1)
2745+
while (error_range <= 0.1 && number_of_decimals <= DBL_DIG)
27182746
{
27192747
number_of_decimals++;
27202748
error_range*= 10.0;
27212749
}
27222750

2723-
double rounded_result= my_double_round(latlongitude,
2724-
number_of_decimals,
2725-
false,
2726-
false);
2751+
double return_value;
2752+
do
2753+
{
2754+
return_value= my_double_round(latlongitude, number_of_decimals, false,
2755+
false);
2756+
number_of_decimals++;
2757+
} while ((lower_limit > return_value || return_value > upper_limit) &&
2758+
number_of_decimals <= DBL_DIG);
2759+
2760+
/*
2761+
We may in some cases still be outside of the allowed range. If this is the
2762+
case, return the input value (which we know for sure to be within the
2763+
allowed range).
2764+
*/
2765+
if (lower_limit > return_value || return_value > upper_limit)
2766+
return_value= latlongitude;
2767+
27272768
// Avoid printing signed zero.
2728-
return rounded_result + 0.0;
2769+
return return_value + 0.0;
27292770
}
27302771
}
27312772

‎sql/item_func.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -1023,7 +1023,8 @@ class Item_func_latlongfromgeohash :public Item_real_func
10231023
double lower_latitude, double upper_longitude,
10241024
double lower_longitude, double *result_latitude,
10251025
double *result_longitude);
1026-
static double round_latlongitude(double latlongitude, double error_range);
1026+
static double round_latlongitude(double latlongitude, double error_range,
1027+
double lower_limit, double upper_limit);
10271028
static bool check_geohash_argument_valid_type(Item *item);
10281029
};
10291030

0 commit comments

Comments
 (0)