fix: negate the day range in Timestamp != date filters#655
Open
TheSaiEaranti wants to merge 1 commit into
Open
fix: negate the day range in Timestamp != date filters#655TheSaiEaranti wants to merge 1 commit into
TheSaiEaranti wants to merge 1 commit into
Conversation
Timestamp.__ne__'s date branch called self.between(start, end) — the same
positive range construction as __eq__ — so `Timestamp("f") != date(...)`
matched the entire day its docstring says it excludes, silently returning
exactly the rows the caller asked to filter out. The non-date path already
negates via FilterOperator.NE, as do the NE forms of Tag, Text, Num and Geo.
Compute the day bounds exactly as __eq__ does (including the astimezone
conversion the date branch of __ne__ was also missing) and emit them through
the existing NE operator template, making `!= date` the exact string negation
of `== date` on any machine. Date-only ISO strings take the same path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
Timestamp.__ne__'s docstring promises: "For date objects (without time), this excludes the entire day." The implementation calledself.between(start, end)— the same positive range construction as__eq__— so the filter matched the entire day instead:Silent inverted filtering:
!=with a date returns exactly the rows the caller asked to exclude, with no error or warning. The non-date path of the same method already negates correctly viaFilterOperator.NE((-@created_at:[v v])), as do theNEforms ofTag,Text,Num, andGeo— the date branch was the one exception.There was a second, smaller asymmetry hiding in the same branch:
__eq__computes its day bounds with.astimezone(timezone.utc)while__ne__used baredatetime.combine, so the two methods disagreed about where the day starts on machines whose local timezone isn't UTC.The fix
Compute the day bounds exactly as
__eq__does (including theastimezoneconversion), convert with_convert_to_timestampexactly asbetween()does, and emit through the existingOPERATOR_MAP[FilterOperator.NE]template. The invariant this establishes:str(ts != d)is byte-for-bytef"(-{ts == d})"on any machine, for bothdateobjects and date-only ISO strings ("2023-03-17"), which take the same branch. The datetime/unix path is untouched.Tests
Added
test_timestamp_not_equal_date, which asserts the negated-range form, the exact-negation-of-__eq__invariant, and the ISO-string path. The existing suite covered!=only with adatetime(the non-date path), which is why this never failed. The new test fails on currentmainand passes with the fix.tests/unit/test_filter.py: 63/63 pass. Fulltests/unit: 1168 passed, 1 skipped — with 6 errors identical on unmodifiedmainin my environment (Docker-dependent CLI fixtures; no daemon locally).black,isort, andmypyclean.Notes
auto:patchlabel per CONTRIBUTING — I can't set labels as an outside contributor.filter.pywhile working on docs: add exception reference and error handling guide #654 (exception docs, still open) — happy to rebase either PR if they land in either order; they don't overlap.Note
Medium Risk
Changes query semantics for a previously broken filter path; callers relying on the old (incorrect) behavior would see different results, but the fix aligns with documented behavior and other filter types.
Overview
Fixes
Timestamp != date(and date-only ISO strings) so they exclude the full day instead of matching it. The date branch no longer callsbetween(); it builds the same UTC day bounds as==, converts them with_convert_to_timestamp, and emits a negated Redis range viaFilterOperator.NE—sostr(ts != d)matches(-{ts == d}).Adds
test_timestamp_not_equal_dateto lock in the negated query shape, the==/!=pairing, and the ISO-string path.Reviewed by Cursor Bugbot for commit 1157c08. Bugbot is set up for automated code reviews on this repo. Configure here.