Skip to content

fix(search): render longitude before latitude in querystring geo() - #370

Closed
simpleqt wants to merge 1 commit into
valkey-io:mainfrom
simpleqt:fix/search-geo-latlon-swap
Closed

simpleqt wants to merge 1 commit into
valkey-io:mainfrom
simpleqt:fix/search-geo-latlon-swap

Conversation

@simpleqt

Copy link
Copy Markdown
Contributor

fix(search): render longitude before latitude in querystring geo()

geo() takes its coordinates in lat, lon order and then passes them
positionally into GeoValue, whose parameters are lon, lat:

def geo(lat, lon, radius, unit="km"):
    return GeoValue(lat, lon, radius, unit)

class GeoValue(Value):
    def __init__(self, lon, lat, radius, unit="km"):
        ...
    def to_string(self):
        return f"[{self.lon} {self.lat} {self.radius} {self.unit}]"

So the two coordinates are swapped on the way in, and a filter built for
London renders the wrong point:

>>> geo(lat=51.45, lon=-0.44, radius=10).to_string()
'[51.45 -0.44 10 km]'      # before: the point is (lat, lon), i.e. in the Indian Ocean
'[-0.44 51.45 10 km]'      # after

[-0.44 51.45 ...] is what RediSearch expects — a geo filter is [lon lat radius unit] — and it is the order this repo already uses everywhere else:
GeoValue.__init__(lon, lat, ...), its to_string(), and
query.GeoFilter(field, lon, lat, radius, unit). Only this wrapper disagreed,
which is why the bug is invisible from the GeoValue side and only shows up for
callers of geo().

Change

def geo(lat, lon, radius, unit="km"):
    """...``GeoValue`` stores and renders longitude first, so the arguments are
    swapped here to keep the ``lat, lon`` order of this helper's public
    signature."""
    return GeoValue(lon, lat, radius, unit)

The public signature stays lat, lon, so no caller has to change; the value is
just handed to GeoValue in the order that class actually takes. The docstring
now records why the swap exists, so it does not get "simplified" back.

Testing

tests/test_search_querystring.py (new — the query-string builders had no test
file) covers the rendering, that geo() agrees with a GeoValue built directly,
that the requested unit survives, and that it behaves inside a compound query:

  • on the untouched base: 4 failed — e.g.
    assert '[51.45 -0.44 10 km]' == '[-0.44 51.45 10 km]'
  • with the fix: 4 passed

ruff at the version .pre-commit-config.yaml/pyproject.toml pins (0.17.0)
passes check and format --check on both files.

No open PR or issue covers this (checked the open PR list; my own open PR #369 is
the sentinel parsers and does not touch these files).

AI assistance: this change was produced with AI assistance (an autonomous coding
agent); the analysis, the reproduction, the before/after test runs and the lint
runs were executed locally, and a human reviewed the result before submission.

geo(lat, lon, radius) forwarded its arguments positionally to
GeoValue(lon, lat, radius), so the geo filter it builds carried the
coordinates swapped: geo(lat=51.45, lon=-0.44, radius=10) rendered
[51.45 -0.44 10 km], where RediSearch expects [lon lat radius unit].

The helper's public lat, lon signature is unchanged; only the
forwarding is corrected. Adds tests/test_search_querystring.py, which
covers the pure-Python query-string builders without a server.

Signed-off-by: simpleqt <2015900818@qq.com>
Copilot AI balanced review requested due to automatic review settings October 21, 2026 15:56

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@simpleqt

simpleqt commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Closing this to get my open pull requests in this repository down to a sane number: I had 8 open here at once, which is more than anyone should be asked to review. The branch stays in my fork if it is ever useful - happy to reopen on request.

@simpleqt simpleqt closed this Nov 9, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in valkey-py planning Nov 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants