Python: adds GEOSEARCHSTORE command #1581

shohamazon · 2024-06-16T12:09:18Z

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Yury-Fridlyand · 2024-06-17T17:18:07Z

python/python/glide/async_commands/core.py

+        See https://valkey.io/commands/geosearchstore/ for more details.
+
+        Note:
+            When in cluster mode, both `source` and `destination` must map to the same hash slot.


Yury-Fridlyand · 2024-06-17T17:18:24Z

python/python/glide/async_commands/core.py

+            When in cluster mode, both `source` and `destination` must map to the same hash slot.
+
+        Args:
+            destination (str): TThe key to store the search results.


Suggested change

destination (str): TThe key to store the search results.

destination (str): The key to store the search results.

Yury-Fridlyand · 2024-06-17T17:19:58Z

python/python/glide/async_commands/core.py

+                If not specified, stores all results.
+            store_dist (bool): Whether to store the distance from the center as the sorted set score. Defaults to False.
+                - The distance is from the center of the circle or box, as a floating-point number, in the same unit specified for that shape.
+                - If set to False, the geohash of the location will be stored as the sorted set score.


Is geohash stored if store_dist set to True?

If store_dist is false - the geohash is stored as score
If it's true- the distance is stored as score

Can you please update the doc? It was unclear for me.

Yury-Fridlyand · 2024-06-17T17:22:00Z

python/python/glide/async_commands/sorted_set.py

 ) -> List[str]:
-    args = [key]
+    args = [*keys]


Optional:

Suggested change

args = [*keys]

args = keys

Yury-Fridlyand · 2024-06-17T17:22:13Z

python/python/glide/async_commands/transaction.py

+        See https://valkey.io/commands/geosearchstore/ for more details.
+
+        Args:
+            destination (str): TThe key to store the search results.


Suggested change

destination (str): TThe key to store the search results.

destination (str): The key to store the search results.

Yury-Fridlyand · 2024-06-17T17:23:15Z

python/python/tests/test_async_client.py

@@ -4491,6 +4793,12 @@ async def test_multi_key_command_returns_cross_slot_error(
                "abc", "zxy", ListDirection.LEFT, ListDirection.LEFT, 1
            ),
            redis_client.msetnx({"abc": "abc", "zxy": "zyx"}),
+            redis_client.geosearchstore(


nit: add a version check (see line 4804 for example)

acarbonetto · 2024-06-18T05:10:46Z

python/python/glide/async_commands/core.py

+        store_dist: bool = False,
+    ) -> int:
+        """
+        Stores the results of a geospatial search into a destination sorted set.


we could be a bit more descriptive with the operation.
Example: zunionsstore:
https://github.com/aws/glide-for-redis/pull/1550/files#diff-37602a66f00251738190a1e0ebddd5d027022433028ab20ba728852a05abab7eR1978-R1980

Maybe:

Searches for members in a sorted set stored at `key` representing geospatial data within a circular or rectangular area and stores the result in `destination`. If `destination` already exists, it is overwritten. Otherwise, a new sorted set will be created. To get the result directly, see `geosearch`.

I agree, thanks!

acarbonetto · 2024-06-18T05:11:36Z

python/python/glide/async_commands/core.py

+                - If set to False, the geohash of the location will be stored as the sorted set score.
+
+        Returns:
+            int: The number of elements in the resulting sorted set.


Suggested change

int: The number of elements in the resulting sorted set.

int: The number of elements in the resulting sorted set stored at `destination`.

acarbonetto · 2024-06-18T05:12:33Z

python/python/glide/async_commands/transaction.py

+        store_dist: bool = False,
+    ) -> TTransaction:
+        """
+        Stores the results of a geospatial search into a destination sorted set.


Same as core.py

acarbonetto · 2024-06-18T05:12:41Z

python/python/glide/async_commands/transaction.py

+                - If set to False, the geohash of the location will be stored as the sorted set score.
+
+        Commands response:
+            int: The number of elements in the resulting sorted set.


same as core.py

acarbonetto · 2024-06-18T05:33:20Z

python/python/tests/test_async_client.py

@@ -2291,6 +2291,308 @@ async def test_geosearch_no_result(self, redis_client: TRedisClient):
                GeoSearchByBox(10, 10, GeoUnit.MILES),
            )

+    @pytest.mark.parametrize("cluster_mode", [True, False])
+    @pytest.mark.parametrize("protocol", [ProtocolVersion.RESP2, ProtocolVersion.RESP3])
+    async def test_geosearchstore_by_box(self, redis_client: TRedisClient):


is it worthwhile adding tests to compare beyond the dateline? Or up near the poles.

Right now, our tests are focused around Europe, which isn't anywhere close to the Pacific coast datelines, and it close to the equator. I'm not sure how much we want to stress the system by doing geosearch around something like Tonga Island (-21.178986, -175.198242). If you do a large enough search, you should get islands on the other side of the meridian, like Fiji (-17.713371, 178.065033).

Of course, this kind of test is more testing Redis, rather than our API. So this is optional.

maybe we can add it as a post GA small task

shohamazon added the python Python wrapper label Jun 16, 2024

shohamazon requested a review from a team as a code owner June 16, 2024 12:09

shohamazon force-pushed the python/geosearchstore branch from fdb8bfc to 5f0c379 Compare June 16, 2024 12:13

Yury-Fridlyand requested a review from aaron-congo June 17, 2024 17:17

Yury-Fridlyand reviewed Jun 17, 2024

View reviewed changes

acarbonetto approved these changes Jun 18, 2024

View reviewed changes

shohamazon force-pushed the python/geosearchstore branch from 5f0c379 to ae616a1 Compare June 18, 2024 08:05

Python: adds GEOSEARCHSTORE command

efc5f39

shohamazon force-pushed the python/geosearchstore branch from ae616a1 to efc5f39 Compare June 18, 2024 08:09

shohamazon merged commit 5a48f24 into valkey-io:main Jun 18, 2024
46 checks passed

shohamazon deleted the python/geosearchstore branch June 18, 2024 08:18

acarbonetto pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 18, 2024

Python: adds GEOSEARCHSTORE command (valkey-io#1581)

9d1eb55

cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 19, 2024

Python: adds GEOSEARCHSTORE command (valkey-io#1581)

3fda0e6

cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 24, 2024

Python: adds GEOSEARCHSTORE command (valkey-io#1581)

01706ca

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python: adds GEOSEARCHSTORE command #1581

Python: adds GEOSEARCHSTORE command #1581

shohamazon commented Jun 16, 2024

Yury-Fridlyand Jun 17, 2024

Yury-Fridlyand Jun 17, 2024

Yury-Fridlyand Jun 17, 2024

shohamazon Jun 17, 2024

Yury-Fridlyand Jun 17, 2024

Yury-Fridlyand Jun 17, 2024

shohamazon Jun 17, 2024

Yury-Fridlyand Jun 17, 2024

Yury-Fridlyand Jun 17, 2024

acarbonetto Jun 18, 2024

shohamazon Jun 18, 2024

acarbonetto Jun 18, 2024

acarbonetto Jun 18, 2024

acarbonetto Jun 18, 2024

acarbonetto Jun 18, 2024

shohamazon Jun 18, 2024

	destination (str): TThe key to store the search results.
	destination (str): The key to store the search results.

	int: The number of elements in the resulting sorted set.
	int: The number of elements in the resulting sorted set stored at `destination`.

Python: adds GEOSEARCHSTORE command #1581

Python: adds GEOSEARCHSTORE command #1581

Conversation

shohamazon commented Jun 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment