Feature/geo support#11
Conversation
- Add GeoDistanceCondition and GeoDistanceSelect dataclasses - Support POINT(lat, lon) syntax (latitude-first, Google Maps style) - Default unit: meters (SQL standard) - Support operators: <, <=, >, >=, BETWEEN - Route <, <= to FT.SEARCH GEOFILTER (optimized) - Route >, >=, BETWEEN to FT.AGGREGATE with FILTER - Add geo_distance() in SELECT for distance calculations
- Add test_geo_fields.py with 12 tests covering: - Basic GEOFILTER generation - All operators (<, <=, >, >=, BETWEEN) - Combined filters (GEO + TAG/TEXT) - geo_distance() in SELECT clause - Integration tests with Redis - Update test_sql_parser.py for POINT(lat, lon) syntax
- Document POINT(lat, lon) coordinate order - Document default unit (meters) and all supported units - Add examples for all operators - Add examples for distance calculation in SELECT - Add examples for combined filters
- Remove coordinate swap - POINT(lon, lat) passes directly to Redis - Consistent with redisvl GeoRadius(lon, lat) API - Update all tests and documentation
There was a problem hiding this comment.
Pull request overview
Adds first-class GEO field support to sql-redis, enabling geo_distance(field, POINT(lon, lat), unit) in both WHERE and SELECT, and translating it into appropriate RediSearch GEOFILTER (for </<=) or FT.AGGREGATE with APPLY/FILTER (for >/>=/BETWEEN).
Changes:
- Extend SQL parsing to produce structured GEO distance conditions/selects (
geo_conditions,geo_distance_selects). - Update translation logic to generate
GEOFILTERfor radius queries andFT.AGGREGATE+FILTERfor other operators and SELECT distance calculations. - Add GEO-specific tests and document GEO usage in the README.
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
uv.lock |
Bumps project version metadata. |
sql_redis/parser.py |
Introduces GEO AST structures and parsing for geo_distance() in WHERE/SELECT. |
sql_redis/analyzer.py |
Tracks referenced fields from GEO WHERE conditions. |
sql_redis/translator.py |
Emits GEOFILTER / FT.AGGREGATE + geo APPLY/FILTER logic. |
tests/test_sql_parser.py |
Updates parser tests to assert GEO-specific parse results. |
tests/test_geo_fields.py |
Adds translator-focused GEO tests plus a raw Redis GEOFILTER sanity check. |
README.md |
Adds detailed GEO feature documentation and examples. |
.gitignore |
Ignores Jupyter notebook artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| Returns: | ||
| Distance in meters. | ||
| """ | ||
| conversions = { | ||
| "m": 1.0, |
There was a problem hiding this comment.
The miles conversion factor used here (1609.344) differs from the factor used for geo_distance() unit conversion in QueryBuilder.build_geo_distance_apply() (1609.34). This can make WHERE filtering (converted to meters) disagree slightly with SELECT output (converted from meters) around boundary values. Consider centralizing these constants (or using the same factor in both places) to keep behavior consistent.
rbs333
left a comment
There was a problem hiding this comment.
High level: looks good!
See comment about lat, lng vs lng, lat. SQL to redis is designed to hide redis weirdness so I think we should keep more sane conventions.
| # First arg: field name | ||
| if isinstance(func_args[0], exp.Column): | ||
| field_name = func_args[0].name | ||
| # Second arg: POINT(lon, lat) - matches Redis's native format |
There was a problem hiding this comment.
I think since the aim of the SQLquery functionality is to abstract Redis weirdness from the user, we should adopt whatever is more common. Therefore, I think we should accept lat,lng and do the switch within sql-redis since a user would theoretically probably do that by default.
There was a problem hiding this comment.
I also left emojis on the bot suggestion worth looking into
There was a problem hiding this comment.
The issue is redisvl does lon, lat too and that causes confusion
Either we change it in both places or none
There was a problem hiding this comment.
Why can't the parser handle swapping them as input to Redis?
geo_distance(lat, lng) => parse => redis geo(lon, lat)
There was a problem hiding this comment.
We can but it made user experience a little off. I suppose someone using this wont care how it's implemented in redisvl.
There was a problem hiding this comment.
Taking this to slack
| is_geo_distance = False | ||
| geo_lon = None | ||
| geo_lat = None | ||
| geo_unit = "m" # Default to meters |
There was a problem hiding this comment.
I think km would be the default for geo right? Not sure what Redis does but yeah
- Fix 1: Validate units in _convert_to_meters, raise ValueError for unsupported - Fix 2: Add geo_distance_selects fields to analyzer validation - Fix 3: Validate radius is provided, raise ValueError if missing - Fix 4: Normalize units to lowercase and validate in parser - Fix 5: Use consistent miles conversion factor (1609.344) across codebase - Fix 6: Apply < and <= geo conditions in FT.AGGREGATE path (bug fix) - Fix 7: Refactor build_geo_distance_apply to return (expr, alias) tuple Added 3 new validation tests: - test_invalid_unit_raises_error - test_uppercase_unit_is_normalized - test_geo_in_select_with_filter_applies_both
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Add @ prefix to LOAD fields in FT.AGGREGATE (bug fix) - Add validation to reject negated geo_distance comparisons - Add validation to reject negated geo_distance BETWEEN - Add validation to reject OR queries with geo_distance - Extract _validate_geo_unit helper to DRY up unit validation - Add 3 new tests for validation errors - Fix test expecting @category in LOAD args - Remove duplicate test_parse_select_without_from - Clean up unused variables in _build_command
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 25 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Replace permissive fallbacks in _build_geo_filter_expression with ValueError (was: '@alias >= 0' which silently broadened results) - Refactor _process_geo_distance_select to validate arguments strictly: - Require at least 2 args - First arg must be a column - Second arg must be POINT(lon, lat) with literal values - Unit arg (if present) must be literal - Fix _add_condition to fail fast when geo_distance() is detected but POINT coordinates can't be parsed (was: fallback to regular Condition) - Fix _add_between_condition with same fail-fast behavior This ensures malformed geo_distance predicates raise clear errors instead of silently producing incorrect queries.
rbs333
left a comment
There was a problem hiding this comment.
Looks good! just probably want to drop those review files
There was a problem hiding this comment.
probably don't want to merge these specs
Add GEO Field Support
Summary
Adds full GEO field support to sql-redis, enabling location-based queries using familiar SQL syntax.
This is a new feature - GEO support does not exist on the main branch.
New Features
1.
geo_distance()FunctionQuery by geographic distance using
POINT(lon, lat)syntax:2. Coordinate Order:
POINT(lon, lat)Uses longitude-first order, matching Redis's native format:
3. Default Unit: Meters
Aligns with SQL spatial standards (PostGIS, MySQL, BigQuery all use meters):
4. Full Operator Support
<,<=FT.SEARCHwithGEOFILTER(optimized)>,>=,BETWEENFT.AGGREGATEwithFILTER5. Distance Calculation in SELECT
Files Changed
sql_redis/parser.pysql_redis/translator.pysql_redis/analyzer.pytests/test_geo_fields.pytests/test_sql_parser.pyREADME.mdUsage Examples
Basic Radius Query (meters)
With Explicit Unit
All Supported Units
All Operators
Distance Calculation
Combined Filters