Skip to content

Python: adds GEOSEARCHSTORE command - #1581

Merged
shohamazon merged 1 commit into
valkey-io:mainfrom
shohamazon:python/geosearchstore
Jun 18, 2024
Merged

shohamazon merged 1 commit into
valkey-io:mainfrom
shohamazon:python/geosearchstore

Conversation

@shohamazon

Copy link
Copy Markdown
Collaborator

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.

@shohamazon shohamazon added the python 🐍 Python wrapper label Jun 16, 2024
@shohamazon
shohamazon requested a review from a team as a code owner June 16, 2024 12:09
@shohamazon
shohamazon force-pushed the python/geosearchstore branch from fdb8bfc to 5f0c379 Compare June 16, 2024 12:13
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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<3

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is geohash stored if store_dist set to True?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

store_dist: bool = False,
) -> List[str]:
args = [key]
args = [*keys]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional:

Suggested change
args = [*keys]
args = keys

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😶

See https://valkey.io/commands/geosearchstore/ for more details.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"abc", "zxy", ListDirection.LEFT, ListDirection.LEFT, 1
),
redis_client.msetnx({"abc": "abc", "zxy": "zyx"}),
redis_client.geosearchstore(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, thanks!

- 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as core.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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as core.py


@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):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can add it as a post GA small task

@shohamazon
shohamazon force-pushed the python/geosearchstore branch from 5f0c379 to ae616a1 Compare June 18, 2024 08:05
@shohamazon
shohamazon force-pushed the python/geosearchstore branch from ae616a1 to efc5f39 Compare June 18, 2024 08:09
@shohamazon
shohamazon merged commit 5a48f24 into valkey-io:main Jun 18, 2024
@shohamazon
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
cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 19, 2024
cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 24, 2024
Yury-Fridlyand pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 6, 2025
affonsov pushed a commit that referenced this pull request Aug 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python 🐍 Python wrapper

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants