Fix Copilot review issues for date/geo support

- Fix geo_distance parsing: use func_name_lower for case-insensitive match
- Reject date function predicates combined with OR operator
- Add date function source fields to LOAD clause
- Validate DATE_FORMAT requires exactly 2 args with literal format string
- Defer date literal conversion to translator (preserve strings in parser)
- Quote string values in FILTER expressions for date functions
- Update tests to reflect deferred date conversion behavior

Author: nkanu17

sql_redis/parser.py

@@ -19,6 +19,56 @@
 )
 
 
+def parse_date_to_timestamp(value: str) -> int | None:
+    """Parse an ISO 8601 date/datetime string to Unix timestamp.
+
+    Supports:
+    - Date: '2024-01-01' (interpreted as midnight UTC)
+    - Datetime: '2024-01-01T12:00:00' or '2024-01-01 12:00:00'
+    - Datetime with timezone: '2024-01-01T12:00:00Z', '2024-01-01T12:00:00+00:00'
+
+    Args:
+        value: The string value to parse.
+
+    Returns:
+        Unix timestamp as integer, or None if not a valid date string.
+    """
+    # Check if it matches date pattern
+    if DATE_PATTERN.match(value):
+        try:
+            dt = datetime.strptime(value, "%Y-%m-%d")
+            # Treat as UTC midnight
+            dt = dt.replace(tzinfo=timezone.utc)
+            return int(dt.timestamp())
+        except ValueError:
+            return None
+
+    # Check if it matches datetime pattern
+    if DATETIME_PATTERN.match(value):
+        # Normalize: replace space with T for parsing
+        normalized = value.replace(" ", "T")
+
+        # Normalize 'Z' (UTC designator) to '+00:00' for fromisoformat
+        if normalized.endswith("Z"):
+            normalized = normalized[:-1] + "+00:00"
+
+        # Normalize timezone offsets without colon (+0000 -> +00:00)
+        # This ensures compatibility with datetime.fromisoformat
+        normalized = re.sub(r"([+-]\d{2})(\d{2})$", r"\1:\2", normalized)
+
+        try:
+            # Use fromisoformat for robust parsing (handles fractional seconds)
+            dt = datetime.fromisoformat(normalized)
+            # If no timezone info, treat as UTC
+            if dt.tzinfo is None:
+                dt = dt.replace(tzinfo=timezone.utc)
+            return int(dt.timestamp())
+        except ValueError:
+            return None
+
+    return None
+
+
 @dataclass
 class AggregationSpec:
     """Specification for an aggregation function."""
@@ -364,7 +414,7 @@ def _process_select_expression_inner(
                             field=field_name,
                             alias=alias or func_name_lower,
                         )
-            elif func_name == "geo_distance":
+            elif func_name_lower == "geo_distance":
                 # geo_distance(field, POINT(lon, lat), unit) in SELECT
                 self._process_geo_distance_select(expression, result, alias)
             elif func_name_lower in redis_reducers:
@@ -493,16 +543,25 @@ def _process_date_function(
         field_name = None
         format_string = None
 
-        if expression.expressions:
-            first_arg = expression.expressions[0]
+        args = expression.expressions or []
+
+        if func_name == "DATE_FORMAT":
+            # DATE_FORMAT requires exactly 2 arguments: field, format_string
+            if len(args) != 2:
+                raise ValueError(
+                    "DATE_FORMAT requires exactly 2 arguments: field, format_string"
+                )
+            first_arg, second_arg = args
+            if isinstance(first_arg, exp.Column):
+                field_name = first_arg.name
+            # Format argument must be a literal string
+            if not isinstance(second_arg, exp.Literal) or not second_arg.is_string:
+                raise ValueError("DATE_FORMAT format argument must be a literal string")
+            format_string = second_arg.this
+        elif args:
+            first_arg = args[0]
             if isinstance(first_arg, exp.Column):
                 field_name = first_arg.name
-
-            # For DATE_FORMAT, extract the format string as second argument
-            if func_name == "DATE_FORMAT" and len(expression.expressions) >= 2:
-                second_arg = expression.expressions[1]
-                if isinstance(second_arg, exp.Literal):
-                    format_string = second_arg.this
 
         if field_name:
             # Generate default alias if not provided
@@ -822,12 +881,15 @@ def _add_function_condition(
                     )
                 )
 
-    def _extract_literal_value(self, expression, convert_dates: bool = True):
+    def _extract_literal_value(self, expression, convert_dates: bool = False):
         """Extract a Python value from a sqlglot Literal or Neg expression.
 
         Args:
             expression: The sqlglot expression to extract from.
             convert_dates: If True, convert ISO 8601 date strings to Unix timestamps.
+                          Default is False to avoid changing semantics for TEXT/TAG
+                          fields. Date conversion should be handled by the translator
+                          when the field type is known to be NUMERIC.
 
         Returns:
             The extracted value, or None if not a literal.
@@ -872,48 +934,6 @@ def _validate_geo_unit(self, unit_val: object) -> str:
     def _parse_date_to_timestamp(self, value: str) -> int | None:
         """Parse an ISO 8601 date/datetime string to Unix timestamp.
 
-        Supports:
-        - Date: '2024-01-01' (interpreted as midnight UTC)
-        - Datetime: '2024-01-01T12:00:00' or '2024-01-01 12:00:00'
-        - Datetime with timezone: '2024-01-01T12:00:00Z', '2024-01-01T12:00:00+00:00'
-
-        Args:
-            value: The string value to parse.
-
-        Returns:
-            Unix timestamp as integer, or None if not a valid date string.
+        Delegates to module-level parse_date_to_timestamp function.
         """
-        # Check if it matches date pattern
-        if DATE_PATTERN.match(value):
-            try:
-                dt = datetime.strptime(value, "%Y-%m-%d")
-                # Treat as UTC midnight
-                dt = dt.replace(tzinfo=timezone.utc)
-                return int(dt.timestamp())
-            except ValueError:
-                return None
-
-        # Check if it matches datetime pattern
-        if DATETIME_PATTERN.match(value):
-            # Normalize: replace space with T for parsing
-            normalized = value.replace(" ", "T")
-
-            # Normalize 'Z' (UTC designator) to '+00:00' for fromisoformat
-            if normalized.endswith("Z"):
-                normalized = normalized[:-1] + "+00:00"
-
-            # Normalize timezone offsets without colon (+0000 -> +00:00)
-            # This ensures compatibility with datetime.fromisoformat
-            normalized = re.sub(r"([+-]\d{2})(\d{2})$", r"\1:\2", normalized)
-
-            try:
-                # Use fromisoformat for robust parsing (handles fractional seconds)
-                dt = datetime.fromisoformat(normalized)
-                # If no timezone info, treat as UTC
-                if dt.tzinfo is None:
-                    dt = dt.replace(tzinfo=timezone.utc)
-                return int(dt.timestamp())
-            except ValueError:
-                return None
-
-        return None
+        return parse_date_to_timestamp(value)

sql_redis/translator.py

@@ -10,6 +10,7 @@
     Condition,
     GeoDistanceCondition,
     SQLParser,
+    parse_date_to_timestamp,
 )
 from sql_redis.query_builder import QueryBuilder
 from sql_redis.schema import AsyncSchemaRegistry, SchemaRegistry
@@ -99,6 +100,16 @@ def _build_command(self, analyzed: AnalyzedQuery) -> TranslatedQuery:
             self._is_date_function_condition(c) for c in parsed.conditions
         )
 
+        # Validate: date function predicates cannot be combined with OR
+        # Date filters are applied via FILTER clauses (ANDed with query).
+        # Combining with OR would change semantics.
+        if has_date_func_conditions and parsed.boolean_operator == "OR":
+            raise ValueError(
+                "Date function predicates cannot be combined with OR; "
+                "they are applied as top-level filters and would change query "
+                "semantics. Rewrite the query to avoid OR with date functions."
+            )
+
         # Determine if we need FT.AGGREGATE
         use_aggregate = (
             len(analyzed.aggregations) > 0
@@ -188,11 +199,16 @@ def _build_condition(self, condition: Condition, field_type: str | None) -> str:
             # Cast value to expected type for numeric conditions
             numeric_value: int | float | tuple[int | float, int | float]
             if isinstance(condition.value, tuple):
-                numeric_value = condition.value  # type: ignore[assignment]
+                # Handle tuple values (e.g., BETWEEN) - try date conversion for each
+                low, high = condition.value
+                low_val = self._convert_to_numeric(low)
+                high_val = self._convert_to_numeric(high)
+                numeric_value = (low_val, high_val)
             elif isinstance(condition.value, (int, float)):
                 numeric_value = condition.value
             else:
-                numeric_value = float(condition.value)  # type: ignore[arg-type]
+                # Try date string conversion for NUMERIC fields
+                numeric_value = self._convert_to_numeric(condition.value)
             return self._query_builder.build_numeric_condition(
                 condition.field,
                 operator,
@@ -207,6 +223,29 @@ def _build_condition(self, condition: Condition, field_type: str | None) -> str:
                 condition.negated,
             )
 
+    def _convert_to_numeric(self, value: object) -> int | float:
+        """Convert a value to numeric, trying date string conversion if needed.
+
+        Args:
+            value: The value to convert. Can be int, float, or string (possibly a date).
+
+        Returns:
+            Numeric value (int or float).
+
+        Raises:
+            ValueError: If conversion fails.
+        """
+        if isinstance(value, (int, float)):
+            return value
+        if isinstance(value, str):
+            # Try date string to timestamp conversion first
+            timestamp = parse_date_to_timestamp(value)
+            if timestamp is not None:
+                return timestamp
+            # Fall back to float conversion
+            return float(value)
+        return float(value)  # type: ignore[arg-type]
+
     def _build_search(
         self, analyzed: AnalyzedQuery, query_string: str
     ) -> TranslatedQuery:
@@ -292,6 +331,13 @@ def _build_aggregate(
         # Load geo fields used in geo_distance() WHERE with >, >=, BETWEEN
         for geo_cond in geo_filter_conditions:
             load_fields.add(geo_cond.field)
+        # Load source fields for date functions in SELECT
+        for date_func in analyzed.date_functions:
+            load_fields.add(date_func.field)
+        # Load source fields for date function conditions in WHERE
+        for condition in parsed.conditions:
+            if self._is_date_function_condition(condition):
+                load_fields.add(condition.field)
         # Load explicit SELECT fields for FT.AGGREGATE
         for field_name in parsed.fields:
             if field_name != "*":
@@ -576,4 +622,23 @@ def _build_date_function_filter(self, condition) -> str:
         op_map = {"=": "==", "!=": "!=", ">": ">", ">=": ">=", "<": "<", "<=": "<="}
         redis_op = op_map.get(op, "==")
 
-        return f"@{alias} {redis_op} {condition.value}"
+        # Normalize value for FILTER expression (quote strings, pass numbers as-is)
+        normalized_value = self._normalize_filter_value(condition.value)
+
+        return f"@{alias} {redis_op} {normalized_value}"
+
+    def _normalize_filter_value(self, value: object) -> str:
+        """Normalize a value for use in FILTER expressions.
+
+        Redis FILTER expressions require string values to be quoted.
+
+        Args:
+            value: The value to normalize.
+
+        Returns:
+            String representation suitable for FILTER expression.
+        """
+        if isinstance(value, (int, float)):
+            return str(value)
+        # Quote string values for FILTER
+        return f'"{value}"'