-
Notifications
You must be signed in to change notification settings - Fork 290
fix(order_by): read a naive datetime as UTC, not local time #1345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Aryan-Pardeshi
wants to merge
1
commit into
qdrant:dev
from
Aryan-Pardeshi:fix/1342-naive-datetime-utc-v2
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| import os | ||
| import time | ||
| from datetime import datetime, timedelta, timezone | ||
|
|
||
| import pytest | ||
|
|
||
| from qdrant_client.local.order_by import to_order_value | ||
|
|
||
| # 2024-06-15 12:30:45 UTC | ||
| WALL_CLOCK = (2024, 6, 15, 12, 30, 45) | ||
| EXPECTED_MICROS = 1718454645000000 | ||
|
|
||
|
|
||
| def test_naive_datetime_object_is_interpreted_as_utc() -> None: | ||
| """A datetime with no tzinfo means UTC, matching how qdrant core reads a | ||
| datetime string with no offset.""" | ||
| assert to_order_value(datetime(*WALL_CLOCK)) == EXPECTED_MICROS | ||
|
|
||
|
|
||
| def test_naive_object_string_and_aware_utc_agree() -> None: | ||
| """The same wall-clock time must order identically however it is spelled.""" | ||
| naive_object = to_order_value(datetime(*WALL_CLOCK)) | ||
| naive_string = to_order_value("2024-06-15 12:30:45") | ||
| aware_utc = to_order_value(datetime(*WALL_CLOCK, tzinfo=timezone.utc)) | ||
|
|
||
| assert naive_object == naive_string == aware_utc | ||
|
|
||
|
|
||
| def test_aware_non_utc_datetime_is_converted_not_stripped() -> None: | ||
| """An explicit offset is still honoured -- naive-means-UTC must not turn | ||
| into treating every datetime as UTC.""" | ||
| aware = datetime(*WALL_CLOCK, tzinfo=timezone(timedelta(hours=5, minutes=30))) | ||
|
|
||
| assert to_order_value(aware) == EXPECTED_MICROS - int(5.5 * 3600 * 1_000_000) | ||
|
|
||
|
|
||
| @pytest.mark.skipif( | ||
| not hasattr(time, "tzset"), | ||
| reason="process timezone is not settable on this platform", | ||
| ) | ||
| def test_order_value_does_not_depend_on_the_client_timezone() -> None: | ||
| """Without this, the naive-object path went through datetime.timestamp(), | ||
| which reads the machine's local time, so the same call produced different | ||
| order values on differently-configured clients.""" | ||
| original = os.environ.get("TZ") | ||
| try: | ||
| values = [] | ||
| for zone in ("UTC", "Asia/Kolkata", "America/Los_Angeles"): | ||
| os.environ["TZ"] = zone | ||
| time.tzset() | ||
| values.append(to_order_value(datetime(*WALL_CLOCK))) | ||
| assert values == [EXPECTED_MICROS] * 3 | ||
| finally: | ||
| if original is None: | ||
| os.environ.pop("TZ", None) | ||
| else: | ||
| os.environ["TZ"] = original | ||
| time.tzset() |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
According to the Python datetime documentation, is a datetime whose tzinfo is non-None but utcoffset() returns None naive, and how does datetime.timestamp() interpret it?💡 Result:
According to the official Python datetime documentation, a datetime object d is considered naive if d.tzinfo is None, or if d.tzinfo is not None but d.tzinfo.utcoffset(d) returns None [1][2]. Conversely, a datetime object is considered aware only if both d.tzinfo is not None and d.tzinfo.utcoffset(d) does not return None [1][2]. When calling datetime.timestamp on such an object (where tzinfo is non-None but utcoffset returns None), the method treats the object as naive [3]. Because the datetime instance is classified as naive, datetime.timestamp assumes it represents local time and relies on platform-specific C functions to perform the conversion to a POSIX timestamp [3].
Citations:
🏁 Script executed:
Repository: qdrant/qdrant-client
Length of output: 50376
🏁 Script executed:
Repository: qdrant/qdrant-client
Length of output: 2925
🌐 Web query:
site:docs.python.org/3/library/datetime.html datetime timestamp tzinfo utcoffset None naive implementation TypeError💡 Result:
In Python's datetime module, the TypeError associated with utcoffset and naive datetime objects usually arises because the datetime object lacks the necessary timezone information to perform calculations or comparisons that require an offset [1]. According to the official Python documentation, a datetime object is considered aware only if its tzinfo attribute is not None and its tzinfo.utcoffset(dt) method returns a timedelta object [1]. If a datetime object is naive (its tzinfo is None), calling certain methods or performing operations that depend on timezone offsets—such as subtracting an aware datetime from a naive one, or comparing naive and aware datetime objects—will raise a TypeError [1]. Specifically, the datetime.utcoffset method returns None if the object's tzinfo is None [1]. If you are implementing a custom tzinfo class, the utcoffset method must return a timedelta object representing the offset, or None if the offset is unknown [1]. If your custom implementation returns something other than None or a valid timedelta object (e.g., if it improperly handles naive input), it will trigger an exception [1]. To avoid these errors: 1. Ensure you are not mixing naive and aware datetime objects in arithmetic or comparison operations [1]. 2. If working with aware objects, ensure the tzinfo attribute is correctly set and that the utcoffset implementation adheres to the required return types [1]. 3. If you encounter this error while implementing a custom class, verify that your utcoffset method logic does not inadvertently return invalid types or improperly handle None inputs [1].
Citations:
🏁 Script executed:
Repository: qdrant/qdrant-client
Length of output: 290
Handle
datetimevalues with aNoneoffset.A
datetimewith non-Nonetzinfoandutcoffset() is Noneis naive. The current condition passes it todatetime.timestamp(), which raisesTypeError. Usedt.tzinfo is None or dt.utcoffset() is Noneand add a regression test with a customtzinfothat returnsNone.🤖 Prompt for AI Agents