[GitHub] [kvrocks] uds5501 commented on a diff in pull request #1533: Add support for GEOSEARCH and GEOSEARCHSTORE

["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