From 446430f8bdc67295cca33e8da407c47301a2be30 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 6 Aug 2026 14:32:57 +0200 Subject: [PATCH 1/2] fix: bound Timestamp date comparisons to the whole UTC day MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Timestamp.__gt__`, `__lt__`, `__ge__` and `__le__` all called `_convert_to_timestamp(other)` without `end_date=True`, so a bare `date` always resolved to that day's 00:00:00. Two of the four need the end of the day instead: > date(2023,3,17) -> @x:[(1679011200.0 +inf] # excluded only 00:00:00 <= date(2023,3,17) -> @x:[-inf 1679011200.0] # included only 00:00:00 So `> date(d)` behaved like `>=` (matching nearly all of day d) and `<= date(d)` behaved like `<` (matching almost none of it). `>=` and `<` were correct only coincidentally, since both legitimately anchor to the start of the day. Pass `end_date=True` in `__gt__`/`__le__` so a bare date resolves to 23:59:59.999999 — the same bound `between()` already uses for its upper end. This treats a bare date as the whole UTC calendar day, consistent with `==`/`!=`. Date-only ISO strings ("2023-03-17") needed one extra step: they reach `_convert_to_timestamp` as strings and `fromisoformat` turns them into a midnight *datetime*, which skips the `date` branch that reads `end_date` altogether. The new `_as_date` helper coerces them to a `date` first, so strings and `date` objects take the same path. The datetime, full-ISO-string and Unix-timestamp paths are untouched. Tests: `test_timestamp_operators` exercised these four operators with a full `datetime` only, never a bare `date`, which is why this went unnoticed. Added `test_timestamp_comparison_operators_with_date` (bounds hard-coded against 2023-03-17Z so it cannot drift with the implementation) and `test_timestamp_comparison_date_bounds_are_utc_days`, which sweeps five dates across five zones — including the non-hour offsets +05:45 and +12:45/+13:45 — using `calendar.timegm` as a timezone-independent oracle. All six fail on current `main`. --- redisvl/query/filter.py | 33 ++++++++++++++- tests/unit/test_filter.py | 86 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 2 deletions(-) diff --git a/redisvl/query/filter.py b/redisvl/query/filter.py index c452a299..9a4ab909 100644 --- a/redisvl/query/filter.py +++ b/redisvl/query/filter.py @@ -716,6 +716,19 @@ def _is_date_only(iso_string: str) -> bool: date_pattern = r"^\d{4}-\d{2}-\d{2}$" return bool(re.match(date_pattern, iso_string)) + @staticmethod + def _as_date(value: Any) -> Any: + """Normalize a date-only ISO string to a date, leaving anything else alone. + + _convert_to_timestamp parses every string with fromisoformat, which turns + "2023-03-17" into a midnight *datetime* and so skips the date branch that + honors end_date. Coercing to a date first keeps date-only strings and bare + date objects on the same path. + """ + if isinstance(value, str) and Timestamp._is_date_only(value): + return datetime.datetime.strptime(value, "%Y-%m-%d").date() + return value + def _convert_to_timestamp(self, value, end_date=False): """ Convert various inputs to a Unix timestamp (seconds since epoch in UTC). @@ -825,13 +838,18 @@ def __gt__(self, other): """ Filter for timestamps greater than the specified value. + For a bare date (or date-only ISO string), this means after the *end* of + that UTC day, so the day itself is excluded. + Args: other: A datetime, date, ISO string, or Unix timestamp Returns: self: The filter object for method chaining """ - timestamp = self._convert_to_timestamp(other) + # end_date anchors a bare date to 23:59:59.999999 so the exclusive lower + # bound skips the whole day rather than just its first instant. + timestamp = self._convert_to_timestamp(self._as_date(other), end_date=True) self._set_value(timestamp, self.SUPPORTED_TYPES, FilterOperator.GT) return FilterExpression(str(self)) @@ -839,6 +857,9 @@ def __lt__(self, other): """ Filter for timestamps less than the specified value. + For a bare date (or date-only ISO string), this means before the *start* + of that UTC day, so the day itself is excluded. + Args: other: A datetime, date, ISO string, or Unix timestamp @@ -853,6 +874,9 @@ def __ge__(self, other): """ Filter for timestamps greater than or equal to the specified value. + For a bare date (or date-only ISO string), this means from the *start* of + that UTC day, so the day itself is included. + Args: other: A datetime, date, ISO string, or Unix timestamp @@ -867,13 +891,18 @@ def __le__(self, other): """ Filter for timestamps less than or equal to the specified value. + For a bare date (or date-only ISO string), this means through the *end* of + that UTC day, so the day itself is included. + Args: other: A datetime, date, ISO string, or Unix timestamp Returns: self: The filter object for method chaining """ - timestamp = self._convert_to_timestamp(other) + # end_date anchors a bare date to 23:59:59.999999 so the inclusive upper + # bound covers the whole day rather than just its first instant. + timestamp = self._convert_to_timestamp(self._as_date(other), end_date=True) self._set_value(timestamp, self.SUPPORTED_TYPES, FilterOperator.LE) return FilterExpression(str(self)) diff --git a/tests/unit/test_filter.py b/tests/unit/test_filter.py index c8c336da..d263fcb1 100644 --- a/tests/unit/test_filter.py +++ b/tests/unit/test_filter.py @@ -1,4 +1,5 @@ import calendar +import operator import time as time_module from datetime import date, datetime, time, timedelta, timezone @@ -554,6 +555,91 @@ def test_timestamp_operators(): assert str(ts) == f"@created_at:[({ts_value} {ts_value2}]" +# The four comparison operators, keyed by the symbol used in failure messages. +TIMESTAMP_COMPARISONS = { + ">": operator.gt, + "<": operator.lt, + ">=": operator.ge, + "<=": operator.le, +} + + +def test_timestamp_comparison_operators_with_date(): + """A bare date bounds the whole UTC day, so > and <= sit at its end. + + Hard-coded against 2023-03-17T00:00:00Z / T23:59:59.999999Z so the test + cannot drift along with the implementation. + """ + expected = { + ">": "@created_at:[(1679097599.999999 +inf]", + "<": "@created_at:[-inf (1679011200.0]", + ">=": "@created_at:[1679011200.0 +inf]", + "<=": "@created_at:[-inf 1679097599.999999]", + } + + for symbol, op in TIMESTAMP_COMPARISONS.items(): + # Bare dates and date-only ISO strings take the same branch + for value in (date(2023, 3, 17), "2023-03-17"): + assert ( + str(op(Timestamp("created_at"), value)) == expected[symbol] + ), f"{symbol} {value!r}" + + +@pytest.mark.skipif(not hasattr(time_module, "tzset"), reason="tzset() is POSIX-only") +@pytest.mark.parametrize( + "d", + [ + date(2023, 3, 17), # ordinary date + date(2023, 3, 12), # US DST spring-forward + date(2023, 11, 5), # US DST fall-back + date(1970, 1, 1), # Unix epoch + date(2038, 1, 20), # past the signed 32-bit rollover + ], +) +def test_timestamp_comparison_date_bounds_are_utc_days(d, monkeypatch): + """Comparison operators bound the UTC day, whatever the host's local zone. + + CI runs in UTC, where the correct and the local-time conversions agree, so + this is the only test that would catch a regression to local-day bounds. + """ + # calendar.timegm is a timezone-independent oracle that shares no code with + # the conversion path under test. + start = float(calendar.timegm(datetime.combine(d, time.min).timetuple())) + end = float(calendar.timegm(datetime.combine(d, time.max).timetuple())) + 0.999999 + + # > and <= exclude/include the whole day, so they anchor to its end; >= and < + # anchor to its start. + expected = { + ">": f"@created_at:[({end} +inf]", + "<": f"@created_at:[-inf ({start}]", + ">=": f"@created_at:[{start} +inf]", + "<=": f"@created_at:[-inf {end}]", + } + + try: + # Includes zones with non-hour offsets (+05:45, +12:45/+13:45), which an + # hour-granularity mistake would pass. + for tz in ( + "UTC", + "America/New_York", + "Asia/Tokyo", + "Asia/Kathmandu", + "Pacific/Chatham", + ): + monkeypatch.setenv("TZ", tz) + time_module.tzset() + + for symbol, op in TIMESTAMP_COMPARISONS.items(): + for value in (d, d.isoformat()): + assert ( + str(op(Timestamp("created_at"), value)) == expected[symbol] + ), f"{symbol} {value!r} in TZ={tz}" + finally: + # Restore TZ and re-read it, so later tests see the original zone + monkeypatch.undo() + time_module.tzset() + + def test_timestamp_between(): """Test the between method for date ranges.""" start = datetime(2023, 3, 1, 0, 0, 0, tzinfo=timezone.utc) From f7861b2fe02bf46506307a64dae65763cf076c62 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Thu, 6 Aug 2026 15:37:04 +0200 Subject: [PATCH 2/2] fix: apply the date coercion inside _convert_to_timestamp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. The previous commit called `_as_date` at the two operator call sites, which left `between()` — the third caller passing `end_date=True` — still short-circuiting a date-only string into a midnight datetime: <= "2023-03-19" -> [-inf 1679270399.999999] end of day between("2023-03-17", "2023-03-19") -> [... 1679184000.0] MIDNIGHT Before the previous commit these agreed (both wrong); afterwards they disagreed, and `between()` also disagreed with itself depending on whether you passed `date(2023, 3, 19)` or `"2023-03-19"`. Move the coercion into `_convert_to_timestamp` so every `end_date=True` caller gets it, and drop it from `__gt__`/`__le__`. Verified across 52 input/operator combinations: output is byte-identical to the previous commit except the two `between()`-with-date-string cases, which now correctly reach the end of the day. Also normalize the error for a date-shaped non-date. `_is_date_only` matches on the digit pattern alone, so "2023-02-30" reached `strptime` and raised its raw message from `>`/`<=` while `<`/`>=` raised the curated one. `_as_date` now falls through on ValueError and lets `_convert_to_timestamp` reject it, so all four operators report "String timestamp must be in ISO format: ..." alike. Docs: add `Timestamp` to docs/api/filter.rst, which autoclassed every sibling filter (Tag, Text, Num, Geo, GeoRadius) but not this one — so none of these docstrings reached docs.redisvl.com. Verified with a sphinx build: the class plus all six operators and `between()` now render, and the private helpers stay out. Document `between()`'s whole-day semantics and its previously undocumented `inclusive` argument, and `_convert_to_timestamp`'s `end_date` argument. Tests: add `test_timestamp_between_date_only_string_matches_date_object` and `test_timestamp_date_shaped_but_invalid_string` (12 cases, four operators x three malformed inputs). Also note in the TZ test why its `timegm(...) + 0.999999` oracle is not float-exact for pre-epoch dates, so adding 1969-12-31 to that list doesn't look like a real failure. 82 pass in test_filter.py, under both TZ unset and TZ=Asia/Kathmandu. --- docs/api/filter.rst | 11 +++++++++++ redisvl/query/filter.py | 29 ++++++++++++++++++++++------- tests/unit/test_filter.py | 31 ++++++++++++++++++++++++++++++- 3 files changed, 63 insertions(+), 8 deletions(-) diff --git a/docs/api/filter.rst b/docs/api/filter.rst index bcd11ab3..068c70b1 100644 --- a/docs/api/filter.rst +++ b/docs/api/filter.rst @@ -68,3 +68,14 @@ GeoRadius :members: :special-members: :exclude-members: __hash__ + + +Timestamp +========= + +.. currentmodule:: redisvl.query.filter + +.. autoclass:: Timestamp + :members: + :special-members: + :exclude-members: __hash__ diff --git a/redisvl/query/filter.py b/redisvl/query/filter.py index 9a4ab909..bd411a5f 100644 --- a/redisvl/query/filter.py +++ b/redisvl/query/filter.py @@ -720,13 +720,18 @@ def _is_date_only(iso_string: str) -> bool: def _as_date(value: Any) -> Any: """Normalize a date-only ISO string to a date, leaving anything else alone. - _convert_to_timestamp parses every string with fromisoformat, which turns - "2023-03-17" into a midnight *datetime* and so skips the date branch that - honors end_date. Coercing to a date first keeps date-only strings and bare - date objects on the same path. + Returns a datetime.date for a "YYYY-MM-DD" string, and the value + unchanged for every other input, including a date-shaped string that is + not a real calendar date. """ if isinstance(value, str) and Timestamp._is_date_only(value): - return datetime.datetime.strptime(value, "%Y-%m-%d").date() + try: + return datetime.datetime.strptime(value, "%Y-%m-%d").date() + except ValueError: + # Date-shaped but not a real date, e.g. "2023-02-30": _is_date_only + # only checks the digit pattern. Hand it back so the caller below + # rejects it with one consistent message. + return value return value def _convert_to_timestamp(self, value, end_date=False): @@ -750,6 +755,10 @@ def _convert_to_timestamp(self, value, end_date=False): # Already a Unix timestamp return float(value) + # Coerce before the fromisoformat call below, which would otherwise turn a + # date-only string into a midnight datetime and skip the end_date branch. + value = self._as_date(value) + if isinstance(value, str): # Parse ISO format try: @@ -849,7 +858,7 @@ def __gt__(self, other): """ # end_date anchors a bare date to 23:59:59.999999 so the exclusive lower # bound skips the whole day rather than just its first instant. - timestamp = self._convert_to_timestamp(self._as_date(other), end_date=True) + timestamp = self._convert_to_timestamp(other, end_date=True) self._set_value(timestamp, self.SUPPORTED_TYPES, FilterOperator.GT) return FilterExpression(str(self)) @@ -902,7 +911,7 @@ def __le__(self, other): """ # end_date anchors a bare date to 23:59:59.999999 so the inclusive upper # bound covers the whole day rather than just its first instant. - timestamp = self._convert_to_timestamp(self._as_date(other), end_date=True) + timestamp = self._convert_to_timestamp(other, end_date=True) self._set_value(timestamp, self.SUPPORTED_TYPES, FilterOperator.LE) return FilterExpression(str(self)) @@ -910,9 +919,15 @@ def between(self, start, end, inclusive: str = "both"): """ Filter for timestamps between start and end (inclusive). + Bare dates (and date-only ISO strings) span whole UTC calendar days: + start anchors to 00:00:00 of its day and end to 23:59:59.999999 of its + day, so both endpoint days are covered in full. + Args: start: A datetime, date, ISO string, or Unix timestamp end: A datetime, date, ISO string, or Unix timestamp + inclusive: Which endpoints to include -- "both" (default), "left", + "right", or "neither". Returns: self: The filter object for method chaining diff --git a/tests/unit/test_filter.py b/tests/unit/test_filter.py index d263fcb1..db1dd08d 100644 --- a/tests/unit/test_filter.py +++ b/tests/unit/test_filter.py @@ -585,6 +585,31 @@ def test_timestamp_comparison_operators_with_date(): ), f"{symbol} {value!r}" +def test_timestamp_between_date_only_string_matches_date_object(): + """A date-only string endpoint spans the same whole day a date object does. + + `end` is the interesting one: it goes through `end_date=True`, which only + reaches the end of the day if the string was coerced to a date first. + """ + by_date = str(Timestamp("created_at").between(date(2023, 3, 1), date(2023, 3, 17))) + by_string = str(Timestamp("created_at").between("2023-03-01", "2023-03-17")) + + assert by_string == by_date + assert by_date == "@created_at:[1677628800.0 1679097599.999999]" + + +@pytest.mark.parametrize("symbol", list(TIMESTAMP_COMPARISONS)) +@pytest.mark.parametrize("value", ["2023-02-30", "2023-13-45", "0000-00-00"]) +def test_timestamp_date_shaped_but_invalid_string(symbol, value): + """Every operator rejects a date-shaped non-date the same way. + + `_is_date_only` matches on the digit pattern alone, so these reach the + coercion and have to fall through to one shared error message. + """ + with pytest.raises(ValueError, match=f"must be in ISO format: {value}"): + TIMESTAMP_COMPARISONS[symbol](Timestamp("created_at"), value) + + @pytest.mark.skipif(not hasattr(time_module, "tzset"), reason="tzset() is POSIX-only") @pytest.mark.parametrize( "d", @@ -603,7 +628,11 @@ def test_timestamp_comparison_date_bounds_are_utc_days(d, monkeypatch): this is the only test that would catch a regression to local-day bounds. """ # calendar.timegm is a timezone-independent oracle that shares no code with - # the conversion path under test. + # the conversion path under test. Caveat for anyone extending the list above: + # adding the microseconds back on is float-exact only for non-negative + # timestamps, so a pre-epoch date such as 1969-12-31 fails here spuriously + # (oracle -1.0000000000287557e-06 vs. a correct -1e-06). Assert those against + # datetime.combine(d, time.max, tzinfo=timezone.utc).timestamp() instead. start = float(calendar.timegm(datetime.combine(d, time.min).timetuple())) end = float(calendar.timegm(datetime.combine(d, time.max).timetuple())) + 0.999999