You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "uds5501 (via GitHub)" <gi...@apache.org> on 2023/06/28 17:45:09 UTC
[GitHub] [kvrocks] uds5501 opened a new pull request, #1533: Draft: Introduce GeoShape and GeoSearch command parsing
uds5501 opened a new pull request, #1533:
URL: https://github.com/apache/kvrocks/pull/1533
[WIP] Currently completed basic command parsing. Now working on BYBOX implementations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] PragmaTwice commented on a diff in pull request #1533: Draft: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1257677887
##########
src/types/redis_geo.cc:
##########
@@ -85,11 +85,18 @@ rocksdb::Status Geo::Radius(const Slice &user_key, double longitude, double lati
rocksdb::Status s = ZSet::GetMetadata(ns_key, &metadata);
if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ GeoShape geo_shape;
+ geo_shape.type = CIRCULAR;
+ geo_shape.xy[0] = longitude;
+ geo_shape.xy[1] = latitude;
+ geo_shape.radius = radius_meters;
+ geo_shape.conversion = 1;
+
/* Get all neighbor geohash boxes for our radius search */
- GeoHashRadius georadius = GeoHashHelper::GetAreasByRadiusWGS84(longitude, latitude, radius_meters);
+ GeoHashRadius georadius = GeoHashHelper::GetAreasByShapeWGS84(&geo_shape);
Review Comment:
I think it is ok to refactor them if it is convenient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] PragmaTwice commented on pull request #1533: Draft: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#issuecomment-1634610592
> @git-hulk @PragmaTwice @infdahai , this MR is ready for review, thanks for your patience :D
Congrats! It seems you need firstly resolve the conflict in geo_test.go.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on pull request #1533: Draft: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#issuecomment-1634603834
@git-hulk @PragmaTwice @infdahai , this MR is ready for review, thanks for your patience :D
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] torwig commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1271326529
##########
src/commands/cmd_geo.cc:
##########
@@ -355,6 +364,249 @@ class CommandGeoRadius : public CommandGeoBase {
double latitude_ = 0;
};
+class CommandGeoSearch : public CommandGeoBase {
+ public:
+ CommandGeoSearch() : CommandGeoBase() {}
+
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ key_ = GET_OR_RET(parser.TakeStr());
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("frommember")) {
+ auto s = setOriginType(kMember);
+ if (!s.IsOK()) return s;
+
+ member_ = GET_OR_RET(parser.TakeStr());
+ } else if (parser.EatEqICase("fromlonlat")) {
+ auto s = setOriginType(kLongLat);
+ if (!s.IsOK()) return s;
+
+ longitude_ = GET_OR_RET(parser.TakeFloat());
+ latitude_ = GET_OR_RET(parser.TakeFloat());
+ s = ValidateLongLat(&longitude_, &latitude_);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("byradius")) {
+ auto s = setShapeType(CIRCULAR);
+ if (!s.IsOK()) return s;
+ radius_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("bybox")) {
+ auto s = setShapeType(RECTANGULAR);
+ if (!s.IsOK()) return s;
+ width_ = GET_OR_RET(parser.TakeFloat());
+ height_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("asc") && sort_ == kSortNone) {
+ sort_ = kSortASC;
+ } else if (parser.EatEqICase("desc") && sort_ == kSortNone) {
+ sort_ = kSortDESC;
+ } else if (parser.EatEqICase("count")) {
+ count_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ } else if (parser.EatEqICase("withcoord")) {
+ with_coord_ = true;
+ } else if (parser.EatEqICase("withdist")) {
+ with_dist_ = true;
+ } else if (parser.EatEqICase("withhash")) {
+ with_hash_ = true;
+ } else {
+ return {Status::RedisParseErr, "Invalid argument given"};
+ }
+ }
+
+ if (member_ != "" && longitude_ != 0 && latitude_ != 0) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+
+ auto s = createGeoShape();
+ if (!s.IsOK()) {
+ return s;
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::vector<GeoPoint> geo_points;
+ redis::Geo geo_db(svr->storage, conn->GetNamespace());
+
+ auto s = geo_db.Search(args_[1], geo_shape_, origin_point_type_, member_, count_, sort_, false, GetUnitConversion(),
+ &geo_points);
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ *output = generateOutput(geo_points);
+
+ return Status::OK();
+ }
+
+ protected:
+ double radius_ = 0;
+ double height_ = 0;
+ double width_ = 0;
+ int count_ = 0;
+ double longitude_ = 0;
+ double latitude_ = 0;
+ std::string member_;
+ std::string key_;
+ DistanceSort sort_ = kSortNone;
+ GeoShapeType shape_type_ = NONE;
+ OriginPointType origin_point_type_ = kNone;
+ GeoShape geo_shape_;
+
+ Status setShapeType(GeoShapeType shape_type) {
+ if (shape_type_ != NONE) {
+ return {Status::RedisParseErr, "please use only one of BYBOX or BYRADIUS"};
+ }
+ shape_type_ = shape_type;
+ return Status::OK();
+ }
+
+ Status setOriginType(OriginPointType origin_point_type) {
+ if (origin_point_type_ != kNone) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+ origin_point_type_ = origin_point_type;
+ return Status::OK();
+ }
+
+ Status createGeoShape() {
+ if (shape_type_ == NONE) {
+ return {Status::RedisParseErr, "please use BYBOX or BYRADIUS"};
+ }
+ geo_shape_.type = shape_type_;
+ geo_shape_.conversion = GetUnitConversion();
+
+ if (shape_type_ == CIRCULAR) {
+ geo_shape_.radius = radius_;
+ } else {
+ geo_shape_.width = width_;
+ geo_shape_.height = height_;
+ }
+
+ if (origin_point_type_ == kLongLat) {
+ geo_shape_.xy[0] = longitude_;
+ geo_shape_.xy[1] = latitude_;
+ }
+ return Status::OK();
+ }
+
+ std::string generateOutput(const std::vector<GeoPoint> &geo_points) {
+ int result_length = static_cast<int>(geo_points.size());
+ int returned_items_count = (count_ == 0 || result_length < count_) ? result_length : count_;
+ std::vector<std::string> output;
+ output.reserve(returned_items_count);
+ for (int i = 0; i < returned_items_count; i++) {
+ auto geo_point = geo_points[i];
+ if (!with_coord_ && !with_hash_ && !with_dist_) {
+ output[i] = redis::BulkString(geo_point.member);
Review Comment:
I think you can leave `push_back/emplace_back` here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on a diff in pull request #1533: Draft: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1257470896
##########
src/types/redis_geo.cc:
##########
@@ -85,11 +85,18 @@ rocksdb::Status Geo::Radius(const Slice &user_key, double longitude, double lati
rocksdb::Status s = ZSet::GetMetadata(ns_key, &metadata);
if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ GeoShape geo_shape;
+ geo_shape.type = CIRCULAR;
+ geo_shape.xy[0] = longitude;
+ geo_shape.xy[1] = latitude;
+ geo_shape.radius = radius_meters;
+ geo_shape.conversion = 1;
+
/* Get all neighbor geohash boxes for our radius search */
- GeoHashRadius georadius = GeoHashHelper::GetAreasByRadiusWGS84(longitude, latitude, radius_meters);
+ GeoHashRadius georadius = GeoHashHelper::GetAreasByShapeWGS84(&geo_shape);
Review Comment:
Ps: This new implementation is different, hence breaking some tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1265673219
##########
src/types/geohash.cc:
##########
@@ -329,47 +329,47 @@ uint8_t GeoHashHelper::EstimateStepsByRadius(double range_meters, double lat) {
/* 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.
- *
- * This function does not behave correctly with very large radius values, for
- * instance for the coordinates 81.634948934258375 30.561509253718668 and a
- * radius of 7083 kilometers, it reports as bounding boxes:
- *
- * min_lon 7.680495, min_lat -33.119473, max_lon 155.589402, max_lat 94.242491
- *
- * However, for instance, a min_lon of 7.680495 is not correct, because the
- * point -1.27579540014266968 61.33421815228281559 is at less than 7000
- * kilometers away.
- *
- * Since this function is currently only used as an optimization, the
- * optimization is not used for very big radiuses, however the function
- * should be fixed. */
-int GeoHashHelper::BoundingBox(double longitude, double latitude, double radius_meters, double *bounds) {
+ * maximum latitude. */
+int GeoHashHelper::BoundingBox(GeoShape *geo_shape, double *bounds) {
Review Comment:
The `*bounds` is Geoshape's bound itself, hence geo_shape is being changed.
I should probably refactor this function to reflect this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1271320916
##########
src/commands/cmd_geo.cc:
##########
@@ -355,6 +363,249 @@ class CommandGeoRadius : public CommandGeoBase {
double latitude_ = 0;
};
+class CommandGeoSearch : public CommandGeoBase {
+ public:
+ CommandGeoSearch() : CommandGeoBase() {}
+
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ key_ = GET_OR_RET(parser.TakeStr());
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("frommember")) {
+ auto s = setOriginType(kMember);
+ if (!s.IsOK()) return s;
+
+ member_ = GET_OR_RET(parser.TakeStr());
+ } else if (parser.EatEqICase("fromlonlat")) {
+ auto s = setOriginType(kLongLat);
+ if (!s.IsOK()) return s;
+
+ longitude_ = GET_OR_RET(parser.TakeFloat());
+ latitude_ = GET_OR_RET(parser.TakeFloat());
+ s = ValdiateLongLat(&longitude_, &latitude_);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("byradius")) {
+ auto s = setShapeType(CIRCULAR);
+ if (!s.IsOK()) return s;
+ radius_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("bybox")) {
+ auto s = setShapeType(RECTANGULAR);
+ if (!s.IsOK()) return s;
+ width_ = GET_OR_RET(parser.TakeFloat());
+ height_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("asc") && sort_ == kSortNone) {
+ sort_ = kSortASC;
+ } else if (parser.EatEqICase("desc") && sort_ == kSortNone) {
+ sort_ = kSortDESC;
+ } else if (parser.EatEqICase("count")) {
+ count_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ } else if (parser.EatEqICase("withcoord")) {
+ with_coord_ = true;
+ } else if (parser.EatEqICase("withdist")) {
+ with_dist_ = true;
+ } else if (parser.EatEqICase("withhash")) {
+ with_hash_ = true;
+ } else {
+ return {Status::RedisParseErr, "Invalid argument given"};
+ }
+ }
+
+ if (member_ != "" && longitude_ != 0 && latitude_ != 0) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+
+ auto s = createGeoShape();
+ if (!s.IsOK()) {
+ return s;
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::vector<GeoPoint> geo_points;
+ redis::Geo geo_db(svr->storage, conn->GetNamespace());
+
+ auto s = geo_db.Search(args_[1], geo_shape_, origin_point_type_, member_, count_, sort_, store_key_, false,
+ GetUnitConversion(), &geo_points);
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ *output = generateOutput(geo_points);
+ // storing comes later.
+ return Status::OK();
+ }
+
+ protected:
+ double radius_ = 0;
+ double height_ = 0;
+ double width_ = 0;
+ int count_ = 0;
+ double longitude_ = 0;
+ double latitude_ = 0;
+ std::string member_;
+ std::string key_;
+ DistanceSort sort_ = kSortNone;
+ GeoShapeType shape_type_ = NONE;
+ OriginPointType origin_point_type_ = kNone;
+ GeoShape geo_shape_;
+ std::string store_key_;
+ bool store_distance_ = false;
Review Comment:
Agreed, removing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] git-hulk commented on pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#issuecomment-1647339654
Thanks all, merging...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on pull request #1533: Draft: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#issuecomment-1616422745
[WIP] Completed the BYBOX implementation.
TODO
- [ ] Deprecate old code which uses RADIUS-specific functions, instead use by shape alternatives and verify that tests don't break
- [ ] Perform a lint.
- [ ] Write tests for GeoSearch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1265677223
##########
src/types/geohash.cc:
##########
@@ -329,47 +329,47 @@ uint8_t GeoHashHelper::EstimateStepsByRadius(double range_meters, double lat) {
/* 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.
- *
- * This function does not behave correctly with very large radius values, for
- * instance for the coordinates 81.634948934258375 30.561509253718668 and a
- * radius of 7083 kilometers, it reports as bounding boxes:
- *
- * min_lon 7.680495, min_lat -33.119473, max_lon 155.589402, max_lat 94.242491
- *
- * However, for instance, a min_lon of 7.680495 is not correct, because the
- * point -1.27579540014266968 61.33421815228281559 is at less than 7000
- * kilometers away.
- *
- * Since this function is currently only used as an optimization, the
- * optimization is not used for very big radiuses, however the function
- * should be fixed. */
-int GeoHashHelper::BoundingBox(double longitude, double latitude, double radius_meters, double *bounds) {
+ * maximum latitude. */
+int GeoHashHelper::BoundingBox(GeoShape *geo_shape, double *bounds) {
if (!bounds) return 0;
-
- bounds[0] = longitude - RadDeg(radius_meters / EARTH_RADIUS_IN_METERS / cos(DegRad(latitude)));
- bounds[2] = longitude + RadDeg(radius_meters / EARTH_RADIUS_IN_METERS / cos(DegRad(latitude)));
- bounds[1] = latitude - RadDeg(radius_meters / EARTH_RADIUS_IN_METERS);
- bounds[3] = latitude + RadDeg(radius_meters / EARTH_RADIUS_IN_METERS);
+ double longitude = geo_shape->xy[0];
+ double latitude = geo_shape->xy[1];
+ double height = geo_shape->conversion * (geo_shape->type == CIRCULAR ? geo_shape->radius : geo_shape->height / 2);
+ double width = geo_shape->conversion * (geo_shape->type == CIRCULAR ? geo_shape->radius : geo_shape->width / 2);
+
+ const double lat_delta = RadDeg(height / EARTH_RADIUS_IN_METERS);
+ const double long_delta_top = RadDeg(width / EARTH_RADIUS_IN_METERS / cos(DegRad(latitude + lat_delta)));
+ const double long_delta_bottom = RadDeg(width / EARTH_RADIUS_IN_METERS / cos(DegRad(latitude - lat_delta)));
+
+ bool is_in_southern_hemisphere = latitude < 0;
+ bounds[0] = is_in_southern_hemisphere ? longitude - long_delta_bottom : longitude - long_delta_top;
+ bounds[2] = is_in_southern_hemisphere ? longitude + long_delta_bottom : longitude + long_delta_top;
+ bounds[1] = latitude - lat_delta;
+ bounds[3] = latitude + lat_delta;
return 1;
}
-/* Return a set of areas (center + 8) that are able to cover a range query
- * for the specified position and radius. */
-GeoHashRadius GeoHashHelper::GetAreasByRadius(double longitude, double latitude, double radius_meters) {
+GeoHashRadius GeoHashHelper::GetAreasByShapeWGS84(GeoShape *geo_shape) {
Review Comment:
ack, addressing this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1271322824
##########
src/commands/cmd_geo.cc:
##########
@@ -355,6 +363,249 @@ class CommandGeoRadius : public CommandGeoBase {
double latitude_ = 0;
};
+class CommandGeoSearch : public CommandGeoBase {
+ public:
+ CommandGeoSearch() : CommandGeoBase() {}
+
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ key_ = GET_OR_RET(parser.TakeStr());
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("frommember")) {
+ auto s = setOriginType(kMember);
+ if (!s.IsOK()) return s;
+
+ member_ = GET_OR_RET(parser.TakeStr());
+ } else if (parser.EatEqICase("fromlonlat")) {
+ auto s = setOriginType(kLongLat);
+ if (!s.IsOK()) return s;
+
+ longitude_ = GET_OR_RET(parser.TakeFloat());
+ latitude_ = GET_OR_RET(parser.TakeFloat());
+ s = ValdiateLongLat(&longitude_, &latitude_);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("byradius")) {
+ auto s = setShapeType(CIRCULAR);
+ if (!s.IsOK()) return s;
+ radius_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("bybox")) {
+ auto s = setShapeType(RECTANGULAR);
+ if (!s.IsOK()) return s;
+ width_ = GET_OR_RET(parser.TakeFloat());
+ height_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("asc") && sort_ == kSortNone) {
+ sort_ = kSortASC;
+ } else if (parser.EatEqICase("desc") && sort_ == kSortNone) {
+ sort_ = kSortDESC;
+ } else if (parser.EatEqICase("count")) {
+ count_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ } else if (parser.EatEqICase("withcoord")) {
+ with_coord_ = true;
+ } else if (parser.EatEqICase("withdist")) {
+ with_dist_ = true;
+ } else if (parser.EatEqICase("withhash")) {
+ with_hash_ = true;
+ } else {
+ return {Status::RedisParseErr, "Invalid argument given"};
+ }
+ }
+
+ if (member_ != "" && longitude_ != 0 && latitude_ != 0) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+
+ auto s = createGeoShape();
+ if (!s.IsOK()) {
+ return s;
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::vector<GeoPoint> geo_points;
+ redis::Geo geo_db(svr->storage, conn->GetNamespace());
+
+ auto s = geo_db.Search(args_[1], geo_shape_, origin_point_type_, member_, count_, sort_, store_key_, false,
+ GetUnitConversion(), &geo_points);
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ *output = generateOutput(geo_points);
+ // storing comes later.
+ return Status::OK();
+ }
+
+ protected:
+ double radius_ = 0;
+ double height_ = 0;
+ double width_ = 0;
+ int count_ = 0;
+ double longitude_ = 0;
+ double latitude_ = 0;
+ std::string member_;
+ std::string key_;
+ DistanceSort sort_ = kSortNone;
+ GeoShapeType shape_type_ = NONE;
+ OriginPointType origin_point_type_ = kNone;
+ GeoShape geo_shape_;
+ std::string store_key_;
+ bool store_distance_ = false;
Review Comment:
Hmmm, agreed. One could be a simple search and one could be search store. Refactoring this.
##########
src/commands/cmd_geo.cc:
##########
@@ -355,6 +363,249 @@ class CommandGeoRadius : public CommandGeoBase {
double latitude_ = 0;
};
+class CommandGeoSearch : public CommandGeoBase {
+ public:
+ CommandGeoSearch() : CommandGeoBase() {}
+
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ key_ = GET_OR_RET(parser.TakeStr());
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("frommember")) {
+ auto s = setOriginType(kMember);
+ if (!s.IsOK()) return s;
+
+ member_ = GET_OR_RET(parser.TakeStr());
+ } else if (parser.EatEqICase("fromlonlat")) {
+ auto s = setOriginType(kLongLat);
+ if (!s.IsOK()) return s;
+
+ longitude_ = GET_OR_RET(parser.TakeFloat());
+ latitude_ = GET_OR_RET(parser.TakeFloat());
+ s = ValdiateLongLat(&longitude_, &latitude_);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("byradius")) {
+ auto s = setShapeType(CIRCULAR);
+ if (!s.IsOK()) return s;
+ radius_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("bybox")) {
+ auto s = setShapeType(RECTANGULAR);
+ if (!s.IsOK()) return s;
+ width_ = GET_OR_RET(parser.TakeFloat());
+ height_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("asc") && sort_ == kSortNone) {
+ sort_ = kSortASC;
+ } else if (parser.EatEqICase("desc") && sort_ == kSortNone) {
+ sort_ = kSortDESC;
+ } else if (parser.EatEqICase("count")) {
+ count_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ } else if (parser.EatEqICase("withcoord")) {
+ with_coord_ = true;
+ } else if (parser.EatEqICase("withdist")) {
+ with_dist_ = true;
+ } else if (parser.EatEqICase("withhash")) {
+ with_hash_ = true;
+ } else {
+ return {Status::RedisParseErr, "Invalid argument given"};
+ }
+ }
+
+ if (member_ != "" && longitude_ != 0 && latitude_ != 0) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+
+ auto s = createGeoShape();
+ if (!s.IsOK()) {
+ return s;
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::vector<GeoPoint> geo_points;
+ redis::Geo geo_db(svr->storage, conn->GetNamespace());
+
+ auto s = geo_db.Search(args_[1], geo_shape_, origin_point_type_, member_, count_, sort_, store_key_, false,
+ GetUnitConversion(), &geo_points);
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ *output = generateOutput(geo_points);
+ // storing comes later.
+ return Status::OK();
+ }
+
+ protected:
+ double radius_ = 0;
+ double height_ = 0;
+ double width_ = 0;
+ int count_ = 0;
+ double longitude_ = 0;
+ double latitude_ = 0;
+ std::string member_;
+ std::string key_;
+ DistanceSort sort_ = kSortNone;
+ GeoShapeType shape_type_ = NONE;
+ OriginPointType origin_point_type_ = kNone;
+ GeoShape geo_shape_;
+ std::string store_key_;
+ bool store_distance_ = false;
Review Comment:
Hmmm, agreed. One could be a simple search and one could be search store. Refactoring this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1271321762
##########
src/commands/cmd_geo.cc:
##########
@@ -355,6 +363,249 @@ class CommandGeoRadius : public CommandGeoBase {
double latitude_ = 0;
};
+class CommandGeoSearch : public CommandGeoBase {
+ public:
+ CommandGeoSearch() : CommandGeoBase() {}
+
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ key_ = GET_OR_RET(parser.TakeStr());
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("frommember")) {
+ auto s = setOriginType(kMember);
+ if (!s.IsOK()) return s;
+
+ member_ = GET_OR_RET(parser.TakeStr());
+ } else if (parser.EatEqICase("fromlonlat")) {
+ auto s = setOriginType(kLongLat);
+ if (!s.IsOK()) return s;
+
+ longitude_ = GET_OR_RET(parser.TakeFloat());
+ latitude_ = GET_OR_RET(parser.TakeFloat());
+ s = ValdiateLongLat(&longitude_, &latitude_);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("byradius")) {
+ auto s = setShapeType(CIRCULAR);
+ if (!s.IsOK()) return s;
+ radius_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("bybox")) {
+ auto s = setShapeType(RECTANGULAR);
+ if (!s.IsOK()) return s;
+ width_ = GET_OR_RET(parser.TakeFloat());
+ height_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("asc") && sort_ == kSortNone) {
+ sort_ = kSortASC;
+ } else if (parser.EatEqICase("desc") && sort_ == kSortNone) {
+ sort_ = kSortDESC;
+ } else if (parser.EatEqICase("count")) {
+ count_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ } else if (parser.EatEqICase("withcoord")) {
+ with_coord_ = true;
+ } else if (parser.EatEqICase("withdist")) {
+ with_dist_ = true;
+ } else if (parser.EatEqICase("withhash")) {
+ with_hash_ = true;
+ } else {
+ return {Status::RedisParseErr, "Invalid argument given"};
+ }
+ }
+
+ if (member_ != "" && longitude_ != 0 && latitude_ != 0) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+
+ auto s = createGeoShape();
+ if (!s.IsOK()) {
+ return s;
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::vector<GeoPoint> geo_points;
+ redis::Geo geo_db(svr->storage, conn->GetNamespace());
+
+ auto s = geo_db.Search(args_[1], geo_shape_, origin_point_type_, member_, count_, sort_, store_key_, false,
+ GetUnitConversion(), &geo_points);
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ *output = generateOutput(geo_points);
+ // storing comes later.
+ return Status::OK();
+ }
+
+ protected:
+ double radius_ = 0;
+ double height_ = 0;
+ double width_ = 0;
+ int count_ = 0;
+ double longitude_ = 0;
+ double latitude_ = 0;
+ std::string member_;
+ std::string key_;
+ DistanceSort sort_ = kSortNone;
+ GeoShapeType shape_type_ = NONE;
+ OriginPointType origin_point_type_ = kNone;
+ GeoShape geo_shape_;
+ std::string store_key_;
+ bool store_distance_ = false;
+
+ Status setShapeType(GeoShapeType shape_type) {
+ if (shape_type_ != NONE) {
+ return {Status::RedisParseErr, "please use only one of BYBOX or BYRADIUS"};
+ }
+ shape_type_ = shape_type;
+ return Status::OK();
+ }
+
+ Status setOriginType(OriginPointType origin_point_type) {
+ if (origin_point_type_ != kNone) {
Review Comment:
Similar thought process to differentiate between a longlat type vs member type instruction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#issuecomment-1646744752
@torwig updated the MR with suggestions, please check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] git-hulk commented on pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#issuecomment-1647340967
@uds5501 Thanks for your support again!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] torwig commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1271326209
##########
src/commands/cmd_geo.cc:
##########
@@ -355,6 +363,249 @@ class CommandGeoRadius : public CommandGeoBase {
double latitude_ = 0;
};
+class CommandGeoSearch : public CommandGeoBase {
+ public:
+ CommandGeoSearch() : CommandGeoBase() {}
+
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ key_ = GET_OR_RET(parser.TakeStr());
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("frommember")) {
+ auto s = setOriginType(kMember);
+ if (!s.IsOK()) return s;
+
+ member_ = GET_OR_RET(parser.TakeStr());
+ } else if (parser.EatEqICase("fromlonlat")) {
+ auto s = setOriginType(kLongLat);
+ if (!s.IsOK()) return s;
+
+ longitude_ = GET_OR_RET(parser.TakeFloat());
+ latitude_ = GET_OR_RET(parser.TakeFloat());
+ s = ValdiateLongLat(&longitude_, &latitude_);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("byradius")) {
+ auto s = setShapeType(CIRCULAR);
+ if (!s.IsOK()) return s;
+ radius_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("bybox")) {
+ auto s = setShapeType(RECTANGULAR);
+ if (!s.IsOK()) return s;
+ width_ = GET_OR_RET(parser.TakeFloat());
+ height_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("asc") && sort_ == kSortNone) {
+ sort_ = kSortASC;
+ } else if (parser.EatEqICase("desc") && sort_ == kSortNone) {
+ sort_ = kSortDESC;
+ } else if (parser.EatEqICase("count")) {
+ count_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ } else if (parser.EatEqICase("withcoord")) {
+ with_coord_ = true;
+ } else if (parser.EatEqICase("withdist")) {
+ with_dist_ = true;
+ } else if (parser.EatEqICase("withhash")) {
+ with_hash_ = true;
+ } else {
+ return {Status::RedisParseErr, "Invalid argument given"};
+ }
+ }
+
+ if (member_ != "" && longitude_ != 0 && latitude_ != 0) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+
+ auto s = createGeoShape();
+ if (!s.IsOK()) {
+ return s;
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::vector<GeoPoint> geo_points;
+ redis::Geo geo_db(svr->storage, conn->GetNamespace());
+
+ auto s = geo_db.Search(args_[1], geo_shape_, origin_point_type_, member_, count_, sort_, store_key_, false,
+ GetUnitConversion(), &geo_points);
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ *output = generateOutput(geo_points);
+ // storing comes later.
+ return Status::OK();
+ }
+
+ protected:
+ double radius_ = 0;
+ double height_ = 0;
+ double width_ = 0;
+ int count_ = 0;
+ double longitude_ = 0;
+ double latitude_ = 0;
+ std::string member_;
+ std::string key_;
+ DistanceSort sort_ = kSortNone;
+ GeoShapeType shape_type_ = NONE;
+ OriginPointType origin_point_type_ = kNone;
+ GeoShape geo_shape_;
+ std::string store_key_;
+ bool store_distance_ = false;
+
+ Status setShapeType(GeoShapeType shape_type) {
+ if (shape_type_ != NONE) {
Review Comment:
Now I got your point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#issuecomment-1646635451
@torwig thanks for taking out time to review, I have made changes and replied to a couple of comments, kindly check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] PragmaTwice commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1263471114
##########
src/types/redis_geo.h:
##########
@@ -68,24 +70,27 @@ class Geo : public ZSet {
rocksdb::Status RadiusByMember(const Slice &user_key, const Slice &member, double radius_meters, int count,
DistanceSort sort, const std::string &store_key, bool store_distance,
double unit_conversion, std::vector<GeoPoint> *geo_points);
-
+ rocksdb::Status Search(const Slice &user_key, GeoShape geo_shape, OriginPointType point_type, std::string &member,
+ int count, DistanceSort sort, const std::string &store_key, bool store_distance,
+ double unit_conversion, std::vector<GeoPoint> *geo_points);
rocksdb::Status Get(const Slice &user_key, const Slice &member, GeoPoint *geo_point);
rocksdb::Status MGet(const Slice &user_key, const std::vector<Slice> &members,
std::map<std::string, GeoPoint> *geo_points);
static std::string EncodeGeoHash(double longitude, double latitude);
private:
static int decodeGeoHash(double bits, double *xy);
- int membersOfAllNeighbors(const Slice &user_key, GeoHashRadius n, double lon, double lat, double radius,
+ int membersOfAllNeighbors(const Slice &user_key, GeoHashRadius n, GeoShape &geo_shape,
std::vector<GeoPoint> *geo_points);
- int membersOfGeoHashBox(const Slice &user_key, GeoHashBits hash, std::vector<GeoPoint> *geo_points, double lon,
- double lat, double radius);
+ int membersOfGeoHashBox(const Slice &user_key, GeoHashBits hash, std::vector<GeoPoint> *geo_points,
+ GeoShape &geo_shape);
static void scoresOfGeoHashBox(GeoHashBits hash, GeoHashFix52Bits *min, GeoHashFix52Bits *max);
- int getPointsInRange(const Slice &user_key, double min, double max, double lon, double lat, double radius,
+ int getPointsInRange(const Slice &user_key, double min, double max, GeoShape &geo_shape,
std::vector<GeoPoint> *geo_points);
static bool appendIfWithinRadius(std::vector<GeoPoint> *geo_points, double lon, double lat, double radius,
double score, const std::string &member);
-
+ static bool appendIfWithinShape(std::vector<GeoPoint> *geo_points, GeoShape &geo_shape, double score,
+ const std::string &member);
Review Comment:
ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1271378933
##########
src/types/geohash.h:
##########
@@ -59,6 +59,8 @@ enum GeoDirection {
GEOHASH_NORT_EAST
};
+enum GeoShapeType { NONE = 0, CIRCULAR, RECTANGULAR };
Review Comment:
ack, renaming it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1271321725
##########
src/commands/cmd_geo.cc:
##########
@@ -355,6 +363,249 @@ class CommandGeoRadius : public CommandGeoBase {
double latitude_ = 0;
};
+class CommandGeoSearch : public CommandGeoBase {
+ public:
+ CommandGeoSearch() : CommandGeoBase() {}
+
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ key_ = GET_OR_RET(parser.TakeStr());
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("frommember")) {
+ auto s = setOriginType(kMember);
+ if (!s.IsOK()) return s;
+
+ member_ = GET_OR_RET(parser.TakeStr());
+ } else if (parser.EatEqICase("fromlonlat")) {
+ auto s = setOriginType(kLongLat);
+ if (!s.IsOK()) return s;
+
+ longitude_ = GET_OR_RET(parser.TakeFloat());
+ latitude_ = GET_OR_RET(parser.TakeFloat());
+ s = ValdiateLongLat(&longitude_, &latitude_);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("byradius")) {
+ auto s = setShapeType(CIRCULAR);
+ if (!s.IsOK()) return s;
+ radius_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("bybox")) {
+ auto s = setShapeType(RECTANGULAR);
+ if (!s.IsOK()) return s;
+ width_ = GET_OR_RET(parser.TakeFloat());
+ height_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("asc") && sort_ == kSortNone) {
+ sort_ = kSortASC;
+ } else if (parser.EatEqICase("desc") && sort_ == kSortNone) {
+ sort_ = kSortDESC;
+ } else if (parser.EatEqICase("count")) {
+ count_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ } else if (parser.EatEqICase("withcoord")) {
+ with_coord_ = true;
+ } else if (parser.EatEqICase("withdist")) {
+ with_dist_ = true;
+ } else if (parser.EatEqICase("withhash")) {
+ with_hash_ = true;
+ } else {
+ return {Status::RedisParseErr, "Invalid argument given"};
+ }
+ }
+
+ if (member_ != "" && longitude_ != 0 && latitude_ != 0) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+
+ auto s = createGeoShape();
+ if (!s.IsOK()) {
+ return s;
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::vector<GeoPoint> geo_points;
+ redis::Geo geo_db(svr->storage, conn->GetNamespace());
+
+ auto s = geo_db.Search(args_[1], geo_shape_, origin_point_type_, member_, count_, sort_, store_key_, false,
+ GetUnitConversion(), &geo_points);
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ *output = generateOutput(geo_points);
+ // storing comes later.
+ return Status::OK();
+ }
+
+ protected:
+ double radius_ = 0;
+ double height_ = 0;
+ double width_ = 0;
+ int count_ = 0;
+ double longitude_ = 0;
+ double latitude_ = 0;
+ std::string member_;
+ std::string key_;
+ DistanceSort sort_ = kSortNone;
+ GeoShapeType shape_type_ = NONE;
+ OriginPointType origin_point_type_ = kNone;
+ GeoShape geo_shape_;
+ std::string store_key_;
+ bool store_distance_ = false;
+
+ Status setShapeType(GeoShapeType shape_type) {
+ if (shape_type_ != NONE) {
Review Comment:
The thought process here is that only one of BYBOX or BYRADIUS should be present.
Initially the internal `shape_type_` is none,
- suppose I parse `BYBOX` first, I'll verify first if the shape is not already set to something else, if not then I can set it to RECTANGULAR.
- Now suppose I parse `BYRADIUS` in the command, I'll try overriding the shape to CIRCULAR which shouldn't be allowed. Hence sending the mentioned error message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] torwig commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1271309884
##########
src/commands/cmd_geo.cc:
##########
@@ -355,6 +363,249 @@ class CommandGeoRadius : public CommandGeoBase {
double latitude_ = 0;
};
+class CommandGeoSearch : public CommandGeoBase {
+ public:
+ CommandGeoSearch() : CommandGeoBase() {}
+
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ key_ = GET_OR_RET(parser.TakeStr());
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("frommember")) {
+ auto s = setOriginType(kMember);
+ if (!s.IsOK()) return s;
+
+ member_ = GET_OR_RET(parser.TakeStr());
+ } else if (parser.EatEqICase("fromlonlat")) {
+ auto s = setOriginType(kLongLat);
+ if (!s.IsOK()) return s;
+
+ longitude_ = GET_OR_RET(parser.TakeFloat());
+ latitude_ = GET_OR_RET(parser.TakeFloat());
+ s = ValdiateLongLat(&longitude_, &latitude_);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("byradius")) {
+ auto s = setShapeType(CIRCULAR);
+ if (!s.IsOK()) return s;
+ radius_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("bybox")) {
+ auto s = setShapeType(RECTANGULAR);
+ if (!s.IsOK()) return s;
+ width_ = GET_OR_RET(parser.TakeFloat());
+ height_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("asc") && sort_ == kSortNone) {
+ sort_ = kSortASC;
+ } else if (parser.EatEqICase("desc") && sort_ == kSortNone) {
+ sort_ = kSortDESC;
+ } else if (parser.EatEqICase("count")) {
+ count_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ } else if (parser.EatEqICase("withcoord")) {
+ with_coord_ = true;
+ } else if (parser.EatEqICase("withdist")) {
+ with_dist_ = true;
+ } else if (parser.EatEqICase("withhash")) {
+ with_hash_ = true;
+ } else {
+ return {Status::RedisParseErr, "Invalid argument given"};
+ }
+ }
+
+ if (member_ != "" && longitude_ != 0 && latitude_ != 0) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+
+ auto s = createGeoShape();
+ if (!s.IsOK()) {
+ return s;
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::vector<GeoPoint> geo_points;
+ redis::Geo geo_db(svr->storage, conn->GetNamespace());
+
+ auto s = geo_db.Search(args_[1], geo_shape_, origin_point_type_, member_, count_, sort_, store_key_, false,
+ GetUnitConversion(), &geo_points);
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ *output = generateOutput(geo_points);
+ // storing comes later.
+ return Status::OK();
+ }
+
+ protected:
+ double radius_ = 0;
+ double height_ = 0;
+ double width_ = 0;
+ int count_ = 0;
+ double longitude_ = 0;
+ double latitude_ = 0;
+ std::string member_;
+ std::string key_;
+ DistanceSort sort_ = kSortNone;
+ GeoShapeType shape_type_ = NONE;
+ OriginPointType origin_point_type_ = kNone;
+ GeoShape geo_shape_;
+ std::string store_key_;
+ bool store_distance_ = false;
+
+ Status setShapeType(GeoShapeType shape_type) {
+ if (shape_type_ != NONE) {
Review Comment:
Or perhaps the error message isn't correct. If you want to check whether the `shape_type_` variable was already set and this is a repetitive attempt, then the error message should be appropriate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] PragmaTwice commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1263470501
##########
src/types/geohash.cc:
##########
@@ -329,47 +329,47 @@ uint8_t GeoHashHelper::EstimateStepsByRadius(double range_meters, double lat) {
/* 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.
- *
- * This function does not behave correctly with very large radius values, for
- * instance for the coordinates 81.634948934258375 30.561509253718668 and a
- * radius of 7083 kilometers, it reports as bounding boxes:
- *
- * min_lon 7.680495, min_lat -33.119473, max_lon 155.589402, max_lat 94.242491
- *
- * However, for instance, a min_lon of 7.680495 is not correct, because the
- * point -1.27579540014266968 61.33421815228281559 is at less than 7000
- * kilometers away.
- *
- * Since this function is currently only used as an optimization, the
- * optimization is not used for very big radiuses, however the function
- * should be fixed. */
-int GeoHashHelper::BoundingBox(double longitude, double latitude, double radius_meters, double *bounds) {
+ * maximum latitude. */
+int GeoHashHelper::BoundingBox(GeoShape *geo_shape, double *bounds) {
Review Comment:
I think we'd better pass `const GeoShape&` here if the `geo_shape` cannot be null and has not been modifed in the function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] torwig commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1271310944
##########
src/commands/cmd_geo.cc:
##########
@@ -355,6 +363,249 @@ class CommandGeoRadius : public CommandGeoBase {
double latitude_ = 0;
};
+class CommandGeoSearch : public CommandGeoBase {
+ public:
+ CommandGeoSearch() : CommandGeoBase() {}
+
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ key_ = GET_OR_RET(parser.TakeStr());
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("frommember")) {
+ auto s = setOriginType(kMember);
+ if (!s.IsOK()) return s;
+
+ member_ = GET_OR_RET(parser.TakeStr());
+ } else if (parser.EatEqICase("fromlonlat")) {
+ auto s = setOriginType(kLongLat);
+ if (!s.IsOK()) return s;
+
+ longitude_ = GET_OR_RET(parser.TakeFloat());
+ latitude_ = GET_OR_RET(parser.TakeFloat());
+ s = ValdiateLongLat(&longitude_, &latitude_);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("byradius")) {
+ auto s = setShapeType(CIRCULAR);
+ if (!s.IsOK()) return s;
+ radius_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("bybox")) {
+ auto s = setShapeType(RECTANGULAR);
+ if (!s.IsOK()) return s;
+ width_ = GET_OR_RET(parser.TakeFloat());
+ height_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("asc") && sort_ == kSortNone) {
+ sort_ = kSortASC;
+ } else if (parser.EatEqICase("desc") && sort_ == kSortNone) {
+ sort_ = kSortDESC;
+ } else if (parser.EatEqICase("count")) {
+ count_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ } else if (parser.EatEqICase("withcoord")) {
+ with_coord_ = true;
+ } else if (parser.EatEqICase("withdist")) {
+ with_dist_ = true;
+ } else if (parser.EatEqICase("withhash")) {
+ with_hash_ = true;
+ } else {
+ return {Status::RedisParseErr, "Invalid argument given"};
+ }
+ }
+
+ if (member_ != "" && longitude_ != 0 && latitude_ != 0) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+
+ auto s = createGeoShape();
+ if (!s.IsOK()) {
+ return s;
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::vector<GeoPoint> geo_points;
+ redis::Geo geo_db(svr->storage, conn->GetNamespace());
+
+ auto s = geo_db.Search(args_[1], geo_shape_, origin_point_type_, member_, count_, sort_, store_key_, false,
+ GetUnitConversion(), &geo_points);
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ *output = generateOutput(geo_points);
+ // storing comes later.
+ return Status::OK();
+ }
+
+ protected:
+ double radius_ = 0;
+ double height_ = 0;
+ double width_ = 0;
+ int count_ = 0;
+ double longitude_ = 0;
+ double latitude_ = 0;
+ std::string member_;
+ std::string key_;
+ DistanceSort sort_ = kSortNone;
+ GeoShapeType shape_type_ = NONE;
+ OriginPointType origin_point_type_ = kNone;
+ GeoShape geo_shape_;
+ std::string store_key_;
+ bool store_distance_ = false;
+
+ Status setShapeType(GeoShapeType shape_type) {
+ if (shape_type_ != NONE) {
+ return {Status::RedisParseErr, "please use only one of BYBOX or BYRADIUS"};
+ }
+ shape_type_ = shape_type;
+ return Status::OK();
+ }
+
+ Status setOriginType(OriginPointType origin_point_type) {
+ if (origin_point_type_ != kNone) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+ origin_point_type_ = origin_point_type;
+ return Status::OK();
+ }
+
+ Status createGeoShape() {
+ if (shape_type_ == NONE) {
+ return {Status::RedisParseErr, "please use BYBOX or BYRADIUS"};
+ }
+ geo_shape_.type = shape_type_;
+ geo_shape_.conversion = GetUnitConversion();
+
+ if (shape_type_ == CIRCULAR) {
+ geo_shape_.radius = radius_;
+ } else {
+ geo_shape_.width = width_;
+ geo_shape_.height = height_;
+ }
+
+ if (origin_point_type_ == kLongLat) {
+ geo_shape_.xy[0] = longitude_;
+ geo_shape_.xy[1] = latitude_;
+ }
+ return Status::OK();
+ }
+
+ std::string generateOutput(const std::vector<GeoPoint> &geo_points) {
+ int result_length = static_cast<int>(geo_points.size());
+ int returned_items_count = (count_ == 0 || result_length < count_) ? result_length : count_;
+ std::vector<std::string> list;
Review Comment:
Could we call the `reserve` function on the `list` variable with the `returned_items_count` parameter here?
Also, you can rename the `list` variable to something like `output`/`response`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] torwig commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1271326455
##########
src/commands/cmd_geo.cc:
##########
@@ -355,6 +364,249 @@ class CommandGeoRadius : public CommandGeoBase {
double latitude_ = 0;
};
+class CommandGeoSearch : public CommandGeoBase {
+ public:
+ CommandGeoSearch() : CommandGeoBase() {}
+
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ key_ = GET_OR_RET(parser.TakeStr());
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("frommember")) {
+ auto s = setOriginType(kMember);
+ if (!s.IsOK()) return s;
+
+ member_ = GET_OR_RET(parser.TakeStr());
+ } else if (parser.EatEqICase("fromlonlat")) {
+ auto s = setOriginType(kLongLat);
+ if (!s.IsOK()) return s;
+
+ longitude_ = GET_OR_RET(parser.TakeFloat());
+ latitude_ = GET_OR_RET(parser.TakeFloat());
+ s = ValidateLongLat(&longitude_, &latitude_);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("byradius")) {
+ auto s = setShapeType(CIRCULAR);
+ if (!s.IsOK()) return s;
+ radius_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("bybox")) {
+ auto s = setShapeType(RECTANGULAR);
+ if (!s.IsOK()) return s;
+ width_ = GET_OR_RET(parser.TakeFloat());
+ height_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("asc") && sort_ == kSortNone) {
+ sort_ = kSortASC;
+ } else if (parser.EatEqICase("desc") && sort_ == kSortNone) {
+ sort_ = kSortDESC;
+ } else if (parser.EatEqICase("count")) {
+ count_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ } else if (parser.EatEqICase("withcoord")) {
+ with_coord_ = true;
+ } else if (parser.EatEqICase("withdist")) {
+ with_dist_ = true;
+ } else if (parser.EatEqICase("withhash")) {
+ with_hash_ = true;
+ } else {
+ return {Status::RedisParseErr, "Invalid argument given"};
+ }
+ }
+
+ if (member_ != "" && longitude_ != 0 && latitude_ != 0) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+
+ auto s = createGeoShape();
+ if (!s.IsOK()) {
+ return s;
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::vector<GeoPoint> geo_points;
+ redis::Geo geo_db(svr->storage, conn->GetNamespace());
+
+ auto s = geo_db.Search(args_[1], geo_shape_, origin_point_type_, member_, count_, sort_, false, GetUnitConversion(),
+ &geo_points);
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ *output = generateOutput(geo_points);
+
+ return Status::OK();
+ }
+
+ protected:
+ double radius_ = 0;
+ double height_ = 0;
+ double width_ = 0;
+ int count_ = 0;
+ double longitude_ = 0;
+ double latitude_ = 0;
+ std::string member_;
+ std::string key_;
+ DistanceSort sort_ = kSortNone;
+ GeoShapeType shape_type_ = NONE;
+ OriginPointType origin_point_type_ = kNone;
+ GeoShape geo_shape_;
+
+ Status setShapeType(GeoShapeType shape_type) {
+ if (shape_type_ != NONE) {
+ return {Status::RedisParseErr, "please use only one of BYBOX or BYRADIUS"};
+ }
+ shape_type_ = shape_type;
+ return Status::OK();
+ }
+
+ Status setOriginType(OriginPointType origin_point_type) {
+ if (origin_point_type_ != kNone) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+ origin_point_type_ = origin_point_type;
+ return Status::OK();
+ }
+
+ Status createGeoShape() {
+ if (shape_type_ == NONE) {
+ return {Status::RedisParseErr, "please use BYBOX or BYRADIUS"};
+ }
+ geo_shape_.type = shape_type_;
+ geo_shape_.conversion = GetUnitConversion();
+
+ if (shape_type_ == CIRCULAR) {
+ geo_shape_.radius = radius_;
+ } else {
+ geo_shape_.width = width_;
+ geo_shape_.height = height_;
+ }
+
+ if (origin_point_type_ == kLongLat) {
+ geo_shape_.xy[0] = longitude_;
+ geo_shape_.xy[1] = latitude_;
+ }
+ return Status::OK();
+ }
+
+ std::string generateOutput(const std::vector<GeoPoint> &geo_points) {
+ int result_length = static_cast<int>(geo_points.size());
+ int returned_items_count = (count_ == 0 || result_length < count_) ? result_length : count_;
+ std::vector<std::string> output;
+ output.reserve(returned_items_count);
+ for (int i = 0; i < returned_items_count; i++) {
+ auto geo_point = geo_points[i];
+ if (!with_coord_ && !with_hash_ && !with_dist_) {
+ output[i] = redis::BulkString(geo_point.member);
Review Comment:
I'm afraid this line will crash because the `reserve` function doesn't change the vector's size, it only reserves the so-called capacity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] torwig commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1271326976
##########
src/types/geohash.h:
##########
@@ -59,6 +59,8 @@ enum GeoDirection {
GEOHASH_NORT_EAST
};
+enum GeoShapeType { NONE = 0, CIRCULAR, RECTANGULAR };
Review Comment:
You can name enum values using the `kGeoShapeType` prefix: `kGeoShapeTypeNone`, `kGeoShapeTypeCircular`, `kGeoShapeTypeRectangular`. Or use an enum class (for example, `GeoShapeType::kNone` etc.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on pull request #1533: Draft: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#issuecomment-1634611524
@PragmaTwice done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on a diff in pull request #1533: Draft: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1257470896
##########
src/types/redis_geo.cc:
##########
@@ -85,11 +85,18 @@ rocksdb::Status Geo::Radius(const Slice &user_key, double longitude, double lati
rocksdb::Status s = ZSet::GetMetadata(ns_key, &metadata);
if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ GeoShape geo_shape;
+ geo_shape.type = CIRCULAR;
+ geo_shape.xy[0] = longitude;
+ geo_shape.xy[1] = latitude;
+ geo_shape.radius = radius_meters;
+ geo_shape.conversion = 1;
+
/* Get all neighbor geohash boxes for our radius search */
- GeoHashRadius georadius = GeoHashHelper::GetAreasByRadiusWGS84(longitude, latitude, radius_meters);
+ GeoHashRadius georadius = GeoHashHelper::GetAreasByShapeWGS84(&geo_shape);
Review Comment:
Ps: This new implementation is different, hence breaking some tests. (TestGeo/GEORANGE_STORE_option:_plain_usage)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#issuecomment-1645353500
Apologies for the delay, @git-hulk @torwig could you please take a look now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#issuecomment-1646565919
Rebased branch, @git-hulk / @torwig / @PragmaTwice please take a look whenever you get time, thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] torwig commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1271310403
##########
src/commands/cmd_geo.cc:
##########
@@ -355,6 +363,249 @@ class CommandGeoRadius : public CommandGeoBase {
double latitude_ = 0;
};
+class CommandGeoSearch : public CommandGeoBase {
+ public:
+ CommandGeoSearch() : CommandGeoBase() {}
+
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ key_ = GET_OR_RET(parser.TakeStr());
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("frommember")) {
+ auto s = setOriginType(kMember);
+ if (!s.IsOK()) return s;
+
+ member_ = GET_OR_RET(parser.TakeStr());
+ } else if (parser.EatEqICase("fromlonlat")) {
+ auto s = setOriginType(kLongLat);
+ if (!s.IsOK()) return s;
+
+ longitude_ = GET_OR_RET(parser.TakeFloat());
+ latitude_ = GET_OR_RET(parser.TakeFloat());
+ s = ValdiateLongLat(&longitude_, &latitude_);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("byradius")) {
+ auto s = setShapeType(CIRCULAR);
+ if (!s.IsOK()) return s;
+ radius_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("bybox")) {
+ auto s = setShapeType(RECTANGULAR);
+ if (!s.IsOK()) return s;
+ width_ = GET_OR_RET(parser.TakeFloat());
+ height_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("asc") && sort_ == kSortNone) {
+ sort_ = kSortASC;
+ } else if (parser.EatEqICase("desc") && sort_ == kSortNone) {
+ sort_ = kSortDESC;
+ } else if (parser.EatEqICase("count")) {
+ count_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ } else if (parser.EatEqICase("withcoord")) {
+ with_coord_ = true;
+ } else if (parser.EatEqICase("withdist")) {
+ with_dist_ = true;
+ } else if (parser.EatEqICase("withhash")) {
+ with_hash_ = true;
+ } else {
+ return {Status::RedisParseErr, "Invalid argument given"};
+ }
+ }
+
+ if (member_ != "" && longitude_ != 0 && latitude_ != 0) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+
+ auto s = createGeoShape();
+ if (!s.IsOK()) {
+ return s;
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::vector<GeoPoint> geo_points;
+ redis::Geo geo_db(svr->storage, conn->GetNamespace());
+
+ auto s = geo_db.Search(args_[1], geo_shape_, origin_point_type_, member_, count_, sort_, store_key_, false,
+ GetUnitConversion(), &geo_points);
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ *output = generateOutput(geo_points);
+ // storing comes later.
+ return Status::OK();
+ }
+
+ protected:
+ double radius_ = 0;
+ double height_ = 0;
+ double width_ = 0;
+ int count_ = 0;
+ double longitude_ = 0;
+ double latitude_ = 0;
+ std::string member_;
+ std::string key_;
+ DistanceSort sort_ = kSortNone;
+ GeoShapeType shape_type_ = NONE;
+ OriginPointType origin_point_type_ = kNone;
+ GeoShape geo_shape_;
+ std::string store_key_;
+ bool store_distance_ = false;
+
+ Status setShapeType(GeoShapeType shape_type) {
+ if (shape_type_ != NONE) {
+ return {Status::RedisParseErr, "please use only one of BYBOX or BYRADIUS"};
+ }
+ shape_type_ = shape_type;
+ return Status::OK();
+ }
+
+ Status setOriginType(OriginPointType origin_point_type) {
+ if (origin_point_type_ != kNone) {
Review Comment:
The same consideration as with the `setShapeType` function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] git-hulk commented on pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#issuecomment-1642209960
Sorry for pending so long, will review in a few days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on a diff in pull request #1533: Draft: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1257468542
##########
src/types/redis_geo.cc:
##########
@@ -85,11 +85,18 @@ rocksdb::Status Geo::Radius(const Slice &user_key, double longitude, double lati
rocksdb::Status s = ZSet::GetMetadata(ns_key, &metadata);
if (!s.ok()) return s.IsNotFound() ? rocksdb::Status::OK() : s;
+ GeoShape geo_shape;
+ geo_shape.type = CIRCULAR;
+ geo_shape.xy[0] = longitude;
+ geo_shape.xy[1] = latitude;
+ geo_shape.radius = radius_meters;
+ geo_shape.conversion = 1;
+
/* Get all neighbor geohash boxes for our radius search */
- GeoHashRadius georadius = GeoHashHelper::GetAreasByRadiusWGS84(longitude, latitude, radius_meters);
+ GeoHashRadius georadius = GeoHashHelper::GetAreasByShapeWGS84(&geo_shape);
Review Comment:
@infdahai , @git-hulk I wanted an opinion on this.
Should I change the existing georadius functions to use the new shape specific functions or should I let them be for now and they will be taken as part of a different refactor?
If taken as different refactor, I'll revert this piece of code to use `GetAreasByRadiusWGS84`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] PragmaTwice commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1263470681
##########
src/types/geohash.cc:
##########
@@ -329,47 +329,47 @@ uint8_t GeoHashHelper::EstimateStepsByRadius(double range_meters, double lat) {
/* 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.
- *
- * This function does not behave correctly with very large radius values, for
- * instance for the coordinates 81.634948934258375 30.561509253718668 and a
- * radius of 7083 kilometers, it reports as bounding boxes:
- *
- * min_lon 7.680495, min_lat -33.119473, max_lon 155.589402, max_lat 94.242491
- *
- * However, for instance, a min_lon of 7.680495 is not correct, because the
- * point -1.27579540014266968 61.33421815228281559 is at less than 7000
- * kilometers away.
- *
- * Since this function is currently only used as an optimization, the
- * optimization is not used for very big radiuses, however the function
- * should be fixed. */
-int GeoHashHelper::BoundingBox(double longitude, double latitude, double radius_meters, double *bounds) {
+ * maximum latitude. */
+int GeoHashHelper::BoundingBox(GeoShape *geo_shape, double *bounds) {
if (!bounds) return 0;
-
- bounds[0] = longitude - RadDeg(radius_meters / EARTH_RADIUS_IN_METERS / cos(DegRad(latitude)));
- bounds[2] = longitude + RadDeg(radius_meters / EARTH_RADIUS_IN_METERS / cos(DegRad(latitude)));
- bounds[1] = latitude - RadDeg(radius_meters / EARTH_RADIUS_IN_METERS);
- bounds[3] = latitude + RadDeg(radius_meters / EARTH_RADIUS_IN_METERS);
+ double longitude = geo_shape->xy[0];
+ double latitude = geo_shape->xy[1];
+ double height = geo_shape->conversion * (geo_shape->type == CIRCULAR ? geo_shape->radius : geo_shape->height / 2);
+ double width = geo_shape->conversion * (geo_shape->type == CIRCULAR ? geo_shape->radius : geo_shape->width / 2);
+
+ const double lat_delta = RadDeg(height / EARTH_RADIUS_IN_METERS);
+ const double long_delta_top = RadDeg(width / EARTH_RADIUS_IN_METERS / cos(DegRad(latitude + lat_delta)));
+ const double long_delta_bottom = RadDeg(width / EARTH_RADIUS_IN_METERS / cos(DegRad(latitude - lat_delta)));
+
+ bool is_in_southern_hemisphere = latitude < 0;
+ bounds[0] = is_in_southern_hemisphere ? longitude - long_delta_bottom : longitude - long_delta_top;
+ bounds[2] = is_in_southern_hemisphere ? longitude + long_delta_bottom : longitude + long_delta_top;
+ bounds[1] = latitude - lat_delta;
+ bounds[3] = latitude + lat_delta;
return 1;
}
-/* Return a set of areas (center + 8) that are able to cover a range query
- * for the specified position and radius. */
-GeoHashRadius GeoHashHelper::GetAreasByRadius(double longitude, double latitude, double radius_meters) {
+GeoHashRadius GeoHashHelper::GetAreasByShapeWGS84(GeoShape *geo_shape) {
Review Comment:
ditto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1271378568
##########
src/commands/cmd_geo.cc:
##########
@@ -355,6 +364,249 @@ class CommandGeoRadius : public CommandGeoBase {
double latitude_ = 0;
};
+class CommandGeoSearch : public CommandGeoBase {
+ public:
+ CommandGeoSearch() : CommandGeoBase() {}
+
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ key_ = GET_OR_RET(parser.TakeStr());
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("frommember")) {
+ auto s = setOriginType(kMember);
+ if (!s.IsOK()) return s;
+
+ member_ = GET_OR_RET(parser.TakeStr());
+ } else if (parser.EatEqICase("fromlonlat")) {
+ auto s = setOriginType(kLongLat);
+ if (!s.IsOK()) return s;
+
+ longitude_ = GET_OR_RET(parser.TakeFloat());
+ latitude_ = GET_OR_RET(parser.TakeFloat());
+ s = ValidateLongLat(&longitude_, &latitude_);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("byradius")) {
+ auto s = setShapeType(CIRCULAR);
+ if (!s.IsOK()) return s;
+ radius_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("bybox")) {
+ auto s = setShapeType(RECTANGULAR);
+ if (!s.IsOK()) return s;
+ width_ = GET_OR_RET(parser.TakeFloat());
+ height_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("asc") && sort_ == kSortNone) {
+ sort_ = kSortASC;
+ } else if (parser.EatEqICase("desc") && sort_ == kSortNone) {
+ sort_ = kSortDESC;
+ } else if (parser.EatEqICase("count")) {
+ count_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ } else if (parser.EatEqICase("withcoord")) {
+ with_coord_ = true;
+ } else if (parser.EatEqICase("withdist")) {
+ with_dist_ = true;
+ } else if (parser.EatEqICase("withhash")) {
+ with_hash_ = true;
+ } else {
+ return {Status::RedisParseErr, "Invalid argument given"};
+ }
+ }
+
+ if (member_ != "" && longitude_ != 0 && latitude_ != 0) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+
+ auto s = createGeoShape();
+ if (!s.IsOK()) {
+ return s;
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::vector<GeoPoint> geo_points;
+ redis::Geo geo_db(svr->storage, conn->GetNamespace());
+
+ auto s = geo_db.Search(args_[1], geo_shape_, origin_point_type_, member_, count_, sort_, false, GetUnitConversion(),
+ &geo_points);
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ *output = generateOutput(geo_points);
+
+ return Status::OK();
+ }
+
+ protected:
+ double radius_ = 0;
+ double height_ = 0;
+ double width_ = 0;
+ int count_ = 0;
+ double longitude_ = 0;
+ double latitude_ = 0;
+ std::string member_;
+ std::string key_;
+ DistanceSort sort_ = kSortNone;
+ GeoShapeType shape_type_ = NONE;
+ OriginPointType origin_point_type_ = kNone;
+ GeoShape geo_shape_;
+
+ Status setShapeType(GeoShapeType shape_type) {
+ if (shape_type_ != NONE) {
+ return {Status::RedisParseErr, "please use only one of BYBOX or BYRADIUS"};
+ }
+ shape_type_ = shape_type;
+ return Status::OK();
+ }
+
+ Status setOriginType(OriginPointType origin_point_type) {
+ if (origin_point_type_ != kNone) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+ origin_point_type_ = origin_point_type;
+ return Status::OK();
+ }
+
+ Status createGeoShape() {
+ if (shape_type_ == NONE) {
+ return {Status::RedisParseErr, "please use BYBOX or BYRADIUS"};
+ }
+ geo_shape_.type = shape_type_;
+ geo_shape_.conversion = GetUnitConversion();
+
+ if (shape_type_ == CIRCULAR) {
+ geo_shape_.radius = radius_;
+ } else {
+ geo_shape_.width = width_;
+ geo_shape_.height = height_;
+ }
+
+ if (origin_point_type_ == kLongLat) {
+ geo_shape_.xy[0] = longitude_;
+ geo_shape_.xy[1] = latitude_;
+ }
+ return Status::OK();
+ }
+
+ std::string generateOutput(const std::vector<GeoPoint> &geo_points) {
+ int result_length = static_cast<int>(geo_points.size());
+ int returned_items_count = (count_ == 0 || result_length < count_) ? result_length : count_;
+ std::vector<std::string> output;
+ output.reserve(returned_items_count);
+ for (int i = 0; i < returned_items_count; i++) {
+ auto geo_point = geo_points[i];
+ if (!with_coord_ && !with_hash_ && !with_dist_) {
+ output[i] = redis::BulkString(geo_point.member);
Review Comment:
ah my bad, fixing this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] git-hulk commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1271682127
##########
src/commands/cmd_geo.cc:
##########
@@ -52,10 +54,17 @@ class CommandGeoBase : public Commander {
*longitude = *long_stat;
*latitude = *lat_stat;
+ auto s = ValidateLongLat(longitude, latitude);
+ if (!s.OK()) return s;
+
+ return Status::OK();
Review Comment:
```suggestion
return ValidateLongLat(longitude, latitude);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] git-hulk merged pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on pull request #1533: Draft: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#issuecomment-1627464685
[WIP] Bug fixed in the radius section of RADIUS searches.
- [ ] Work on GEOSEARCH BYBOX tests
- [ ] Find the bug and check if expected implementation work similar to redis search.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] torwig commented on pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#issuecomment-1642558346
@uds5501 Could you please run the `./x.py format` command to format the code with `clang`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] uds5501 commented on pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "uds5501 (via GitHub)" <gi...@apache.org>.
uds5501 commented on PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#issuecomment-1645216978
@git-hulk / @torwig will update this today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] git-hulk commented on pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#issuecomment-1644863067
@uds5501 PR generally looks good to me, can help to format like what @torwig mentioned.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] torwig commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1271306498
##########
src/commands/cmd_geo.cc:
##########
@@ -355,6 +363,249 @@ class CommandGeoRadius : public CommandGeoBase {
double latitude_ = 0;
};
+class CommandGeoSearch : public CommandGeoBase {
+ public:
+ CommandGeoSearch() : CommandGeoBase() {}
+
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ key_ = GET_OR_RET(parser.TakeStr());
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("frommember")) {
+ auto s = setOriginType(kMember);
+ if (!s.IsOK()) return s;
+
+ member_ = GET_OR_RET(parser.TakeStr());
+ } else if (parser.EatEqICase("fromlonlat")) {
+ auto s = setOriginType(kLongLat);
+ if (!s.IsOK()) return s;
+
+ longitude_ = GET_OR_RET(parser.TakeFloat());
+ latitude_ = GET_OR_RET(parser.TakeFloat());
+ s = ValdiateLongLat(&longitude_, &latitude_);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("byradius")) {
+ auto s = setShapeType(CIRCULAR);
+ if (!s.IsOK()) return s;
+ radius_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("bybox")) {
+ auto s = setShapeType(RECTANGULAR);
+ if (!s.IsOK()) return s;
+ width_ = GET_OR_RET(parser.TakeFloat());
+ height_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("asc") && sort_ == kSortNone) {
+ sort_ = kSortASC;
+ } else if (parser.EatEqICase("desc") && sort_ == kSortNone) {
+ sort_ = kSortDESC;
+ } else if (parser.EatEqICase("count")) {
+ count_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ } else if (parser.EatEqICase("withcoord")) {
+ with_coord_ = true;
+ } else if (parser.EatEqICase("withdist")) {
+ with_dist_ = true;
+ } else if (parser.EatEqICase("withhash")) {
+ with_hash_ = true;
+ } else {
+ return {Status::RedisParseErr, "Invalid argument given"};
+ }
+ }
+
+ if (member_ != "" && longitude_ != 0 && latitude_ != 0) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+
+ auto s = createGeoShape();
+ if (!s.IsOK()) {
+ return s;
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::vector<GeoPoint> geo_points;
+ redis::Geo geo_db(svr->storage, conn->GetNamespace());
+
+ auto s = geo_db.Search(args_[1], geo_shape_, origin_point_type_, member_, count_, sort_, store_key_, false,
+ GetUnitConversion(), &geo_points);
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ *output = generateOutput(geo_points);
+ // storing comes later.
+ return Status::OK();
+ }
+
+ protected:
+ double radius_ = 0;
+ double height_ = 0;
+ double width_ = 0;
+ int count_ = 0;
+ double longitude_ = 0;
+ double latitude_ = 0;
+ std::string member_;
+ std::string key_;
+ DistanceSort sort_ = kSortNone;
+ GeoShapeType shape_type_ = NONE;
+ OriginPointType origin_point_type_ = kNone;
+ GeoShape geo_shape_;
+ std::string store_key_;
+ bool store_distance_ = false;
Review Comment:
This variable isn't used in the `GEOSEARCH` command.
##########
src/commands/cmd_geo.cc:
##########
@@ -52,10 +54,16 @@ class CommandGeoBase : public Commander {
*longitude = *long_stat;
*latitude = *lat_stat;
+ auto s = ValdiateLongLat(longitude, latitude);
+ if (!s.OK()) return s;
+
+ return Status::OK();
+ }
+
+ static Status ValdiateLongLat(double *longitude, double *latitude) {
if (*longitude < GEO_LONG_MIN || *longitude > GEO_LONG_MAX || *latitude < GEO_LAT_MIN || *latitude > GEO_LAT_MAX) {
- return {Status::RedisParseErr, "invalid longitude,latitude pair " + longitude_para + "," + latitude_para};
+ return {Status::RedisParseErr, "invalid longitude,latitude pair"};
Review Comment:
I think it's a good idea to include longitude and latitude values in the error message.
##########
src/commands/cmd_geo.cc:
##########
@@ -52,10 +54,16 @@ class CommandGeoBase : public Commander {
*longitude = *long_stat;
*latitude = *lat_stat;
+ auto s = ValdiateLongLat(longitude, latitude);
+ if (!s.OK()) return s;
+
+ return Status::OK();
+ }
+
+ static Status ValdiateLongLat(double *longitude, double *latitude) {
Review Comment:
Typo (between I and D) in the function name. Should be `ValidateLongLat`.
##########
src/commands/cmd_geo.cc:
##########
@@ -355,6 +363,249 @@ class CommandGeoRadius : public CommandGeoBase {
double latitude_ = 0;
};
+class CommandGeoSearch : public CommandGeoBase {
+ public:
+ CommandGeoSearch() : CommandGeoBase() {}
+
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ key_ = GET_OR_RET(parser.TakeStr());
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("frommember")) {
+ auto s = setOriginType(kMember);
+ if (!s.IsOK()) return s;
+
+ member_ = GET_OR_RET(parser.TakeStr());
+ } else if (parser.EatEqICase("fromlonlat")) {
+ auto s = setOriginType(kLongLat);
+ if (!s.IsOK()) return s;
+
+ longitude_ = GET_OR_RET(parser.TakeFloat());
+ latitude_ = GET_OR_RET(parser.TakeFloat());
+ s = ValdiateLongLat(&longitude_, &latitude_);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("byradius")) {
+ auto s = setShapeType(CIRCULAR);
+ if (!s.IsOK()) return s;
+ radius_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("bybox")) {
+ auto s = setShapeType(RECTANGULAR);
+ if (!s.IsOK()) return s;
+ width_ = GET_OR_RET(parser.TakeFloat());
+ height_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("asc") && sort_ == kSortNone) {
+ sort_ = kSortASC;
+ } else if (parser.EatEqICase("desc") && sort_ == kSortNone) {
+ sort_ = kSortDESC;
+ } else if (parser.EatEqICase("count")) {
+ count_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ } else if (parser.EatEqICase("withcoord")) {
+ with_coord_ = true;
+ } else if (parser.EatEqICase("withdist")) {
+ with_dist_ = true;
+ } else if (parser.EatEqICase("withhash")) {
+ with_hash_ = true;
+ } else {
+ return {Status::RedisParseErr, "Invalid argument given"};
+ }
+ }
+
+ if (member_ != "" && longitude_ != 0 && latitude_ != 0) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+
+ auto s = createGeoShape();
+ if (!s.IsOK()) {
+ return s;
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::vector<GeoPoint> geo_points;
+ redis::Geo geo_db(svr->storage, conn->GetNamespace());
+
+ auto s = geo_db.Search(args_[1], geo_shape_, origin_point_type_, member_, count_, sort_, store_key_, false,
+ GetUnitConversion(), &geo_points);
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ *output = generateOutput(geo_points);
+ // storing comes later.
+ return Status::OK();
+ }
+
+ protected:
+ double radius_ = 0;
+ double height_ = 0;
+ double width_ = 0;
+ int count_ = 0;
+ double longitude_ = 0;
+ double latitude_ = 0;
+ std::string member_;
+ std::string key_;
+ DistanceSort sort_ = kSortNone;
+ GeoShapeType shape_type_ = NONE;
+ OriginPointType origin_point_type_ = kNone;
+ GeoShape geo_shape_;
+ std::string store_key_;
+ bool store_distance_ = false;
+
+ Status setShapeType(GeoShapeType shape_type) {
+ if (shape_type_ != NONE) {
Review Comment:
I just scratch my head when I see this condition. Perhaps you wanted to check the `if (shape_type == NONE)` here? Because currently you check the already set value of the `shape_type_` variable (pay attention to the `_` character at the end of the name).
##########
src/commands/cmd_geo.cc:
##########
@@ -355,6 +363,249 @@ class CommandGeoRadius : public CommandGeoBase {
double latitude_ = 0;
};
+class CommandGeoSearch : public CommandGeoBase {
+ public:
+ CommandGeoSearch() : CommandGeoBase() {}
+
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ key_ = GET_OR_RET(parser.TakeStr());
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("frommember")) {
+ auto s = setOriginType(kMember);
+ if (!s.IsOK()) return s;
+
+ member_ = GET_OR_RET(parser.TakeStr());
+ } else if (parser.EatEqICase("fromlonlat")) {
+ auto s = setOriginType(kLongLat);
+ if (!s.IsOK()) return s;
+
+ longitude_ = GET_OR_RET(parser.TakeFloat());
+ latitude_ = GET_OR_RET(parser.TakeFloat());
+ s = ValdiateLongLat(&longitude_, &latitude_);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("byradius")) {
+ auto s = setShapeType(CIRCULAR);
+ if (!s.IsOK()) return s;
+ radius_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("bybox")) {
+ auto s = setShapeType(RECTANGULAR);
+ if (!s.IsOK()) return s;
+ width_ = GET_OR_RET(parser.TakeFloat());
+ height_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("asc") && sort_ == kSortNone) {
+ sort_ = kSortASC;
+ } else if (parser.EatEqICase("desc") && sort_ == kSortNone) {
+ sort_ = kSortDESC;
+ } else if (parser.EatEqICase("count")) {
+ count_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ } else if (parser.EatEqICase("withcoord")) {
+ with_coord_ = true;
+ } else if (parser.EatEqICase("withdist")) {
+ with_dist_ = true;
+ } else if (parser.EatEqICase("withhash")) {
+ with_hash_ = true;
+ } else {
+ return {Status::RedisParseErr, "Invalid argument given"};
+ }
+ }
+
+ if (member_ != "" && longitude_ != 0 && latitude_ != 0) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+
+ auto s = createGeoShape();
+ if (!s.IsOK()) {
+ return s;
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::vector<GeoPoint> geo_points;
+ redis::Geo geo_db(svr->storage, conn->GetNamespace());
+
+ auto s = geo_db.Search(args_[1], geo_shape_, origin_point_type_, member_, count_, sort_, store_key_, false,
+ GetUnitConversion(), &geo_points);
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ *output = generateOutput(geo_points);
+ // storing comes later.
Review Comment:
I think this comment is useless.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] torwig commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1533:
URL: https://github.com/apache/kvrocks/pull/1533#discussion_r1271311976
##########
src/commands/cmd_geo.cc:
##########
@@ -355,6 +363,249 @@ class CommandGeoRadius : public CommandGeoBase {
double latitude_ = 0;
};
+class CommandGeoSearch : public CommandGeoBase {
+ public:
+ CommandGeoSearch() : CommandGeoBase() {}
+
+ Status Parse(const std::vector<std::string> &args) override {
+ CommandParser parser(args, 1);
+ key_ = GET_OR_RET(parser.TakeStr());
+
+ while (parser.Good()) {
+ if (parser.EatEqICase("frommember")) {
+ auto s = setOriginType(kMember);
+ if (!s.IsOK()) return s;
+
+ member_ = GET_OR_RET(parser.TakeStr());
+ } else if (parser.EatEqICase("fromlonlat")) {
+ auto s = setOriginType(kLongLat);
+ if (!s.IsOK()) return s;
+
+ longitude_ = GET_OR_RET(parser.TakeFloat());
+ latitude_ = GET_OR_RET(parser.TakeFloat());
+ s = ValdiateLongLat(&longitude_, &latitude_);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("byradius")) {
+ auto s = setShapeType(CIRCULAR);
+ if (!s.IsOK()) return s;
+ radius_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("bybox")) {
+ auto s = setShapeType(RECTANGULAR);
+ if (!s.IsOK()) return s;
+ width_ = GET_OR_RET(parser.TakeFloat());
+ height_ = GET_OR_RET(parser.TakeFloat());
+ std::string distance_raw = GET_OR_RET(parser.TakeStr());
+ s = ParseDistanceUnit(distance_raw);
+ if (!s.IsOK()) return s;
+ } else if (parser.EatEqICase("asc") && sort_ == kSortNone) {
+ sort_ = kSortASC;
+ } else if (parser.EatEqICase("desc") && sort_ == kSortNone) {
+ sort_ = kSortDESC;
+ } else if (parser.EatEqICase("count")) {
+ count_ = GET_OR_RET(parser.TakeInt<int>(NumericRange<int>{1, std::numeric_limits<int>::max()}));
+ } else if (parser.EatEqICase("withcoord")) {
+ with_coord_ = true;
+ } else if (parser.EatEqICase("withdist")) {
+ with_dist_ = true;
+ } else if (parser.EatEqICase("withhash")) {
+ with_hash_ = true;
+ } else {
+ return {Status::RedisParseErr, "Invalid argument given"};
+ }
+ }
+
+ if (member_ != "" && longitude_ != 0 && latitude_ != 0) {
+ return {Status::RedisParseErr, "please use only one of FROMMEMBER or FROMLONLAT"};
+ }
+
+ auto s = createGeoShape();
+ if (!s.IsOK()) {
+ return s;
+ }
+ return Commander::Parse(args);
+ }
+
+ Status Execute(Server *svr, Connection *conn, std::string *output) override {
+ std::vector<GeoPoint> geo_points;
+ redis::Geo geo_db(svr->storage, conn->GetNamespace());
+
+ auto s = geo_db.Search(args_[1], geo_shape_, origin_point_type_, member_, count_, sort_, store_key_, false,
+ GetUnitConversion(), &geo_points);
+
+ if (!s.ok()) {
+ return {Status::RedisExecErr, s.ToString()};
+ }
+ *output = generateOutput(geo_points);
+ // storing comes later.
+ return Status::OK();
+ }
+
+ protected:
+ double radius_ = 0;
+ double height_ = 0;
+ double width_ = 0;
+ int count_ = 0;
+ double longitude_ = 0;
+ double latitude_ = 0;
+ std::string member_;
+ std::string key_;
+ DistanceSort sort_ = kSortNone;
+ GeoShapeType shape_type_ = NONE;
+ OriginPointType origin_point_type_ = kNone;
+ GeoShape geo_shape_;
+ std::string store_key_;
+ bool store_distance_ = false;
Review Comment:
Actually, you don't use the `store_key_` variable in the `GEOSEARCH` command so you can delete it and use just `""` as a function parameter later. But this leads us to the situation when the `Geo::Search` function in the case of the `GEOSEARCH` command doesn't use 2 input parameters so maybe we can have two separate functions instead of one `Geo::Search`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org