Fail fast for malformed geo_distance expressions

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

Author: nkanu17

sql_redis/parser.py

@@ -337,44 +337,59 @@ def _process_vector_distance(
     def _process_geo_distance_select(
         self, expression, result: ParsedQuery, alias: str | None
     ) -> None:
-        """Process geo_distance() in SELECT clause for FT.AGGREGATE APPLY."""
+        """Process geo_distance() in SELECT clause for FT.AGGREGATE APPLY.
+
+        Expected signature: geo_distance(field, POINT(lon, lat)[, unit])
+        Raises ValueError for malformed usage rather than silently ignoring.
+        """
         func_args = expression.expressions
         if not func_args:
-            return
-
-        field_name = None
-        geo_lon = None
-        geo_lat = None
-        geo_unit = "m"  # Default to meters for geodistance()
+            raise ValueError(
+                "geo_distance() requires at least 2 arguments: "
+                "geo_distance(field, POINT(lon, lat)[, unit])"
+            )
 
-        # First arg: field name
-        if isinstance(func_args[0], exp.Column):
-            field_name = func_args[0].name
+        # First arg: field name must be a column
+        if not isinstance(func_args[0], exp.Column):
+            raise ValueError("geo_distance() first argument must be a column reference")
+        field_name = func_args[0].name
 
-        # Second arg: POINT(lon, lat) - matches Redis's native format
-        if len(func_args) >= 2 and isinstance(func_args[1], exp.Anonymous):
-            point_func = func_args[1]
-            if point_func.name.upper() == "POINT" and len(point_func.expressions) >= 2:
-                # POINT(lon, lat) - no swap needed, matches Redis
-                geo_lon = self._extract_literal_value(point_func.expressions[0])
-                geo_lat = self._extract_literal_value(point_func.expressions[1])
+        # Second arg: POINT(lon, lat) required
+        if len(func_args) < 2:
+            raise ValueError(
+                "geo_distance() requires a POINT(lon, lat) second argument"
+            )
+        if not isinstance(func_args[1], exp.Anonymous):
+            raise ValueError("geo_distance() second argument must be POINT(lon, lat)")
+        point_func = func_args[1]
+        if point_func.name.upper() != "POINT" or len(point_func.expressions) < 2:
+            raise ValueError("geo_distance() second argument must be POINT(lon, lat)")
+
+        # Extract literal lon/lat values
+        geo_lon = self._extract_literal_value(point_func.expressions[0])
+        geo_lat = self._extract_literal_value(point_func.expressions[1])
+        if geo_lon is None or geo_lat is None:
+            raise ValueError(
+                "geo_distance() POINT(lon, lat) arguments must be literal values"
+            )
 
         # Third arg (optional): unit
+        geo_unit = "m"  # Default to meters
         if len(func_args) >= 3:
             unit_val = self._extract_literal_value(func_args[2])
-            if unit_val:
-                geo_unit = self._validate_geo_unit(unit_val)
+            if unit_val is None:
+                raise ValueError("geo_distance() unit argument must be a literal value")
+            geo_unit = self._validate_geo_unit(unit_val)
 
-        if field_name and geo_lon is not None and geo_lat is not None:
-            result.geo_distance_selects.append(
-                GeoDistanceSelect(
-                    field=field_name,
-                    lon=float(geo_lon),
-                    lat=float(geo_lat),
-                    alias=alias or "geo_distance",
-                    unit=geo_unit,
-                )
+        result.geo_distance_selects.append(
+            GeoDistanceSelect(
+                field=field_name,
+                lon=float(geo_lon),
+                lat=float(geo_lat),
+                alias=alias or "geo_distance",
+                unit=geo_unit,
             )
+        )
 
     def _process_where_clause(
         self, expression, result: ParsedQuery, negated: bool = False
@@ -463,7 +478,12 @@ def _add_condition(
                 value = int(value) if "." not in str(value) else float(value)
 
         if field_name is not None:
-            if is_geo_distance and geo_lon is not None and geo_lat is not None:
+            if is_geo_distance:
+                # Fail fast if POINT(lon, lat) coordinates couldn't be parsed
+                if geo_lon is None or geo_lat is None:
+                    raise ValueError(
+                        "geo_distance() requires POINT(lon, lat) with literal values"
+                    )
                 # Negated geo_distance is not supported; fail clearly
                 if negated:
                     raise ValueError(
@@ -546,7 +566,12 @@ def _add_between_condition(
         high_val = self._extract_literal_value(high)
 
         if field_name is not None:
-            if is_geo_distance and geo_lon is not None and geo_lat is not None:
+            if is_geo_distance:
+                # Fail fast if POINT(lon, lat) coordinates couldn't be parsed
+                if geo_lon is None or geo_lat is None:
+                    raise ValueError(
+                        "geo_distance() BETWEEN requires POINT(lon, lat) with literal values"
+                    )
                 # Negation is not supported for geo_distance BETWEEN; fail clearly
                 if negated:
                     raise ValueError(

sql_redis/translator.py

@@ -407,14 +407,18 @@ def _build_geo_filter_expression(
                 high_m = self._convert_to_meters(geo_cond.radius[1], geo_cond.unit)
                 return f"@{alias} >= {low_m} && @{alias} <= {high_m}"
             else:
-                # Fallback - shouldn't happen
-                return f"@{alias} >= 0"
+                # Internal inconsistency: BETWEEN requires (low, high) tuple
+                raise ValueError(
+                    f"Invalid geo radius for BETWEEN operator: {geo_cond.radius!r}"
+                )
 
         # Convert radius to meters if needed (geodistance() returns meters)
-        # At this point, radius is guaranteed to be a float (BETWEEN case handled above)
+        # At this point, radius should be a float (BETWEEN case handled above)
         if isinstance(geo_cond.radius, tuple):
-            # Shouldn't reach here, but handle gracefully
-            return f"@{alias} >= 0"
+            # Internal inconsistency: tuple radius outside BETWEEN context
+            raise ValueError(
+                f"Unexpected tuple geo radius outside BETWEEN: {geo_cond.radius!r}"
+            )
         radius_m = self._convert_to_meters(geo_cond.radius, geo_cond.unit)
 
         if geo_cond.operator == ">":