fix(filters): raise FilterError when comparing naive and aware datetimes in ordering operators - #12254
Closed
Aftabbs wants to merge 3 commits into
Closed
Conversation
…mes in ordering operators Ordering operators (>, >=, <, <=) were inconsistent with the equality operators (== and !=): they silently copied the timezone from one datetime to the other, which allowed a >= b and a <= b to both return True while a == b returned False. This violated the identity a == b <=> a >= b and a <= b. The root cause is _ensure_both_dates_naive_or_aware, which was introduced separately from the == / != refusal logic. Fix: raise FilterError for mixed-awareness pairs in ordering operators, matching the policy already in place for == and !=- Closes deepset-ai#12246
|
@Aftabbs is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
…rror behavior
After the naive/aware datetime ordering fix, mixing a naive filter value
('2025-02-01') with timezone-aware document values now raises FilterError.
Split the old test into two:
- test_run_datetime_with_timezone_raises_filter_error: verifies the new
error is raised when filter value is naive and document values are aware.
- test_run_datetime_with_matching_timezone: verifies the happy path when
both filter and document values carry explicit timezone info.
Contributor
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
Contributor
|
Thanks for your contribution! However, we decided to go with a different approach for solving the issue in this PR #12257 |
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
2 participants
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.
Related Issues
Proposed Changes
Before this fix, the metadata filter ordering operators (
>,>=,<,<=) were inconsistent with the equality operators (==,!=) when one datetime was timezone-naive and the other was timezone-aware.Root cause:
_ensure_both_dates_naive_or_aware(used only by the ordering path) silently copied the timezone from one side to the other. The equality path (refactored in #11963) already raisesFilterErrorfor the same mixed-awareness pair.Consequence: For a document with
meta.d = "2023-01-01T00:00:00"(naive) and a filter value of"2023-01-01T00:00:00+00:00"(UTC-aware):==→FilterError✓ (correct)>=→True✗ (timezone was copied, comparing equal instants)<=→True✗ (same)>→False✗ (no error, wrong branch)This violates the identity
a == b ⟺ a >= b ∧ a <= b.Fix:
_ensure_both_dates_naive_or_awarenow raisesFilterErrorwhen the two datetimes differ in timezone awareness, consistent with the policy already in place for==and!=. The error message mirrors the existing one and tells the user what to do.How did you test it?
Truefor mixed-awareness ordering (they tested the old, now-incorrect behaviour).>=with a valid same-awareness pair.test_ordering_and_equality_consistent_for_mixed_timezone_awarenessthat assertsFilterErroris raised for all four ordering operators (>,>=,<,<=) when the document value is naive and the filter value is aware.pytest test/utils/test_filters.py— 115 passed.pytest test/document_stores/test_in_memory.py -k "date or filter"— 68 passed.Notes for the reviewer
The change is confined to
_ensure_both_dates_naive_or_awareinhaystack/utils/filters.py(≈10 line diff). The equality path (_dates_are_equal) is unchanged. The release note is inreleasenotes/notes/filter-naive-aware-datetime-ordering-75719e82d84f43d6.yaml.Checklist
fix:).