Add GEOSEARCH#1118
Conversation
| sealed trait GeoSearch[V] | ||
| object GeoSearch { | ||
| final case class FromMember[V](member: V) extends GeoSearch[V] | ||
| final case class FromLonLat[V](longitude: Longitude, latitude: Latitude) extends GeoSearch[V] |
There was a problem hiding this comment.
V is not used in any of the values held, just extend GeoSearch[Nothing]
There was a problem hiding this comment.
Pull request overview
Adds redis4cats wrappers for Redis GEOSEARCH / GEOSEARCHSTORE commands (backed by existing Lettuce APIs), exposing typed Scala ADTs for search origin/area and returning either members, rich results, or storing results into a sorted set.
Changes:
- Introduces
GeoSearch,GeoSearchArea, andGeoSearchResulteffect types to model GEOSEARCH inputs/outputs. - Adds new
geoSearchoverloads to the Geo algebra and implements them inBaseRedisvia Lettucegeosearch/geosearchstore. - Extends the test scenarios to exercise GEOSEARCH returning results and storing to a destination key.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| modules/tests/src/test/scala/dev/profunktor/redis4cats/TestScenarios.scala | Adds scenario coverage for GEOSEARCH result retrieval and GEOSEARCHSTORE into a ZSET. |
| modules/effects/src/main/scala/dev/profunktor/redis4cats/redis.scala | Implements geoSearch wrappers and adds Java conversion helpers for GEOSEARCH inputs/outputs. |
| modules/effects/src/main/scala/dev/profunktor/redis4cats/effects.scala | Adds new ADTs and result model types for GEOSEARCH. |
| modules/effects/src/main/scala/dev/profunktor/redis4cats/algebra/geo.scala | Extends the Geo algebra with GEOSEARCH and GEOSEARCHSTORE operations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| key, | ||
| from.asJava, | ||
| area.asJava, | ||
| GeoArgs.Builder.count(storage.count), |
There was a problem hiding this comment.
GeoRadiusKeyStorage includes a sort parameter, but this geoSearch(..., storage: GeoRadiusKeyStorage[K]) implementation ignores it and always uses only count. That means callers cannot control ASC/DESC ordering when COUNT is used (and the stored set contents may differ). Build the GeoArgs passed to geosearchstore using both storage.count and storage.sort (or otherwise thread sort through) so the wrapper matches the storage API contract implied by the parameter.
| GeoArgs.Builder.count(storage.count), | |
| GeoArgs.Builder.count(storage.count).sort(storage.sort), |
| async.flatMap( | ||
| _.geosearchstore( | ||
| storage.key, | ||
| key, | ||
| from.asJava, | ||
| area.asJava, | ||
| GeoArgs.Builder.count(storage.count), | ||
| false | ||
| ).futureLift.void | ||
| ) | ||
|
|
||
| override def geoSearch( | ||
| key: K, | ||
| from: effects.GeoSearch[V], | ||
| area: GeoSearchArea, | ||
| storage: GeoRadiusDistStorage[K] | ||
| ): F[Unit] = | ||
| async.flatMap( | ||
| _.geosearchstore( | ||
| storage.key, | ||
| key, | ||
| from.asJava, | ||
| area.asJava, | ||
| GeoArgs.Builder.count(storage.count), | ||
| true | ||
| ).futureLift.void | ||
| ) | ||
|
|
There was a problem hiding this comment.
Same as the GeoRadiusKeyStorage overload: GeoRadiusDistStorage carries a sort, but it is ignored here (only count is applied). If sort is meant to be part of the storage configuration, include it in the GeoArgs you pass to geosearchstore so ASC/DESC behavior is actually honored.
| async.flatMap( | |
| _.geosearchstore( | |
| storage.key, | |
| key, | |
| from.asJava, | |
| area.asJava, | |
| GeoArgs.Builder.count(storage.count), | |
| false | |
| ).futureLift.void | |
| ) | |
| override def geoSearch( | |
| key: K, | |
| from: effects.GeoSearch[V], | |
| area: GeoSearchArea, | |
| storage: GeoRadiusDistStorage[K] | |
| ): F[Unit] = | |
| async.flatMap( | |
| _.geosearchstore( | |
| storage.key, | |
| key, | |
| from.asJava, | |
| area.asJava, | |
| GeoArgs.Builder.count(storage.count), | |
| true | |
| ).futureLift.void | |
| ) | |
| async.flatMap { commands => | |
| val geoArgs = GeoArgs.Builder.count(storage.count) | |
| storage.sort.foreach(geoArgs.sort) | |
| commands | |
| .geosearchstore( | |
| storage.key, | |
| key, | |
| from.asJava, | |
| area.asJava, | |
| geoArgs, | |
| false | |
| ) | |
| .futureLift | |
| .void | |
| } | |
| override def geoSearch( | |
| key: K, | |
| from: effects.GeoSearch[V], | |
| area: GeoSearchArea, | |
| storage: GeoRadiusDistStorage[K] | |
| ): F[Unit] = | |
| async.flatMap { commands => | |
| val geoArgs = GeoArgs.Builder.count(storage.count) | |
| storage.sort.foreach(geoArgs.sort) | |
| commands | |
| .geosearchstore( | |
| storage.key, | |
| key, | |
| from.asJava, | |
| area.asJava, | |
| geoArgs, | |
| true | |
| ) | |
| .futureLift | |
| .void | |
| } |
| def asJava[K]: io.lettuce.core.GeoSearch.GeoRef[K] = v match { | ||
| case effects.GeoSearch.FromMember(member) => io.lettuce.core.GeoSearch.fromMember(member.asInstanceOf[K]) |
There was a problem hiding this comment.
GeoSearchOps.asJava introduces an unsafe asInstanceOf cast and a spurious type parameter (def asJava[K]), even though the conversion target type is determined by the member type V. This can be made type-safe by returning GeoRef[V] directly and passing member without casting, reducing the risk of runtime ClassCastException and making the conversion easier to reason about.
| def asJava[K]: io.lettuce.core.GeoSearch.GeoRef[K] = v match { | |
| case effects.GeoSearch.FromMember(member) => io.lettuce.core.GeoSearch.fromMember(member.asInstanceOf[K]) | |
| def asJava: io.lettuce.core.GeoSearch.GeoRef[V] = v match { | |
| case effects.GeoSearch.FromMember(member) => io.lettuce.core.GeoSearch.fromMember(member) |
| override def geoSearch(key: K, from: effects.GeoSearch[V], area: GeoSearchArea): F[Set[V]] = | ||
| async.flatMap(_.geosearch(key, from.asJava, area.asJava).futureLift.map(_.asScala.toSet)) | ||
|
|
There was a problem hiding this comment.
The PR adds multiple geoSearch overloads, but the test scenario only exercises the GeoArgs overload and the GeoRadiusKeyStorage overload. Add coverage for (1) the no-args geoSearch(key, from, area): F[Set[V]] overload and (2) the GeoRadiusDistStorage overload (ideally asserting that scores are distances when stored) to guard against regressions in these wrappers.
Added wrappers for already existing in lettuce methods.
Tried with real data in Dragonfly, works fine.