This is an automated email from the ASF dual-hosted git repository.
binbin pushed a commit to branch unstable
in repository https://gitbox.apache.org/repos/asf/kvrocks.git
The following commit(s) were added to refs/heads/unstable by this push:
new 5efb8d3b Fix GEOSEARCHSTORE not overwrite the dst key (#1677)
5efb8d3b is described below
commit 5efb8d3b41f9db26bd0bfa3f707159c787c8ad43
Author: Binbin <[email protected]>
AuthorDate: Wed Aug 16 10:15:09 2023 +0800
Fix GEOSEARCHSTORE not overwrite the dst key (#1677)
When doing store, we should call overwrite instead of add, when
doing add, which leads us to perform zadd dst xxx in disguise.
And this cause the following issues.
The first case is more common, it is a normal user case.
We always adding the member results in a incorrect zset:
```
127.0.0.1:6666> flushall
OK
127.0.0.1:6666> geoadd src 10 10 Shenzhen
(integer) 1
127.0.0.1:6666> geoadd src2 10 10 Beijing
(integer) 1
127.0.0.1:6666> GEOSEARCHSTORE dst src FROMMEMBER Shenzhen BYBOX 88 88 m
(integer) 1
127.0.0.1:6666> GEOSEARCHSTORE dst src2 FROMMEMBER Beijing BYBOX 88 88 m
(integer) 1
127.0.0.1:6666> zcard dst
(integer) 2
127.0.0.1:6666> zrange dst 0 -1
1) "Beijing"
2) "Shenzhen"
127.0.0.1:6379> flushall
OK
127.0.0.1:6379> geoadd src 10 10 Shenzhen
(integer) 1
127.0.0.1:6379> geoadd src2 10 10 Beijing
(integer) 1
127.0.0.1:6379> GEOSEARCHSTORE dst src FROMMEMBER Shenzhen BYBOX 88 88 m
(integer) 1
127.0.0.1:6379> GEOSEARCHSTORE dst src2 FROMMEMBER Beijing BYBOX 88 88 m
(integer) 1
127.0.0.1:6379> zcard dst
(integer) 1
127.0.0.1:6379> zrange dst 0 -1
1) "Beijing"
```
The second one is a dst key with the wrong type, the add will
actually return an error (wrong type error) and take no effect
and the result is not actually written:
```
127.0.0.1:6666> flushall
OK
127.0.0.1:6666> geoadd src 10 10 Shenzhen
(integer) 1
127.0.0.1:6666> set dst string
OK
127.0.0.1:6666> GEOSEARCHSTORE dst src FROMMEMBER Shenzhen BYBOX 88 88 m
(integer) 1
127.0.0.1:6666> type dst
string
127.0.0.1:6379> flushall
OK
127.0.0.1:6379> geoadd src 10 10 Shenzhen
(integer) 1
127.0.0.1:6379> set dst string
OK
127.0.0.1:6379> GEOSEARCHSTORE dst src FROMMEMBER Shenzhen BYBOX 88 88 m
(integer) 1
127.0.0.1:6379> type dst
zset
```
---
src/types/redis_geo.cc | 3 +--
tests/gocase/unit/geo/geo_test.go | 20 ++++++++++++++++++++
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/src/types/redis_geo.cc b/src/types/redis_geo.cc
index ef842784..dd56f3eb 100644
--- a/src/types/redis_geo.cc
+++ b/src/types/redis_geo.cc
@@ -159,8 +159,7 @@ rocksdb::Status Geo::SearchStore(const Slice &user_key,
GeoShape geo_shape, Orig
double score = store_distance ? geo_point.dist / unit_conversion :
geo_point.score;
member_scores.emplace_back(MemberScore{geo_point.member, score});
}
- uint64_t ret = 0;
- ZSet::Add(store_key, ZAddFlags::Default(), &member_scores, &ret);
+ ZSet::Overwrite(store_key, member_scores);
}
}
return rocksdb::Status::OK();
diff --git a/tests/gocase/unit/geo/geo_test.go
b/tests/gocase/unit/geo/geo_test.go
index 527c6b6a..ee5f5d48 100644
--- a/tests/gocase/unit/geo/geo_test.go
+++ b/tests/gocase/unit/geo/geo_test.go
@@ -225,6 +225,26 @@ func TestGeo(t *testing.T) {
rdb.GeoSearchStore(ctx, "points", "points2",
&redis.GeoSearchStoreQuery{GeoSearchQuery: redis.GeoSearchQuery{BoxWidth: 200,
BoxHeight: 200, BoxUnit: "km", Longitude: -77.0368707, Latitude: 38.9071923,
Sort: "DESC"}, StoreDist: false}).Val())
})
+ t.Run("GEOSEARCHSTORE will overwrite the dst key", func(t *testing.T) {
+ // dst key wrong type
+ require.NoError(t, rdb.Do(ctx, "del", "src", "dst").Err())
+ require.NoError(t, rdb.Do(ctx, "geoadd", "src", "10", "10",
"Shenzhen").Err())
+ require.NoError(t, rdb.Do(ctx, "set", "dst", "string").Err())
+ require.NoError(t, rdb.Do(ctx, "geosearchstore", "dst", "src",
"frommember", "Shenzhen", "bybox", "88", "88", "m").Err())
+ require.Equal(t, "zset", rdb.Type(ctx, "dst").Val())
+ require.Equal(t, []string{"Shenzhen"}, rdb.ZRange(ctx, "dst",
0, -1).Val())
+
+ // normal case
+ require.NoError(t, rdb.Del(ctx, "dst").Err())
+ require.NoError(t, rdb.Do(ctx, "del", "src", "src2",
"dst").Err())
+ require.NoError(t, rdb.Do(ctx, "geoadd", "src", "10", "10",
"Shenzhen").Err())
+ require.NoError(t, rdb.Do(ctx, "geoadd", "src2", "10", "10",
"Beijing").Err())
+ require.NoError(t, rdb.Do(ctx, "geosearchstore", "dst", "src",
"frommember", "Shenzhen", "bybox", "88", "88", "m").Err())
+ require.NoError(t, rdb.Do(ctx, "geosearchstore", "dst", "src2",
"frommember", "Beijing", "bybox", "88", "88", "m").Err())
+ require.Equal(t, int64(1), rdb.ZCard(ctx, "dst").Val())
+ require.Equal(t, []string{"Beijing"}, rdb.ZRange(ctx, "dst", 0,
-1).Val())
+ })
+
t.Run("GEOHASH is able to return geohash strings", func(t *testing.T) {
require.NoError(t, rdb.Del(ctx, "points").Err())
require.NoError(t, rdb.GeoAdd(ctx, "points",
&redis.GeoLocation{Name: "test", Longitude: -5.6, Latitude: 42.6}).Err())
