Repository navigation
fix: Read naive datetimes as UTC in UnixTimestamp values - #6937
jyejare merged 3 commits into
Conversation
Sorry, something went wrong.
Converting a naive datetime.datetime to a UnixTimestamp value called datetime.timestamp(), which reads a naive value in the machine's local timezone. A naive pd.Timestamp or np.datetime64 is read as UTC, as is a naive ZonedTimestamp, and Feast treats naive datetimes as UTC elsewhere (make_tzaware). So the same wall-clock value was stored at a different instant depending on its Python type and on the host timezone, e.g. a naive request timestamp passed to get_online_features came back shifted by the server's UTC offset. Attach UTC to naive datetimes before taking the timestamp. This covers UnixTimestamp scalars, lists and sets. Signed-off-by: Mohammad Hijjawi <mohammad.hijjawi1997@gmail.com>
jyejare
left a comment
There was a problem hiding this comment.
The change fixes the common case where naive datetimes were interpreted in the host’s local timezone instead of UTC, and the regression test covers scalar, list, and set values. One edge case remains: Python also considers datetimes with a non-None tzinfo but a None utcoffset() to be naive, and those still use the local timezone. The test setup could also be more explicit on platforms where it cannot set the process timezone.
Sorry, something went wrong.
| # A naive datetime is UTC, as everywhere else in Feast. Without this, | ||
| # datetime.timestamp() would read it in the machine's local timezone. | ||
| if value.tzinfo is None: |
There was a problem hiding this comment.
A datetime is naive not only when tzinfo is None, but also when tzinfo is set and utcoffset() returns None. For that valid case, this condition leaves the value unchanged, so timestamp() still interprets it in the machine’s local timezone. Consider checking value.utcoffset() is None instead.
Suggested:
| # A naive datetime is UTC, as everywhere else in Feast. Without this, | |
| # datetime.timestamp() would read it in the machine's local timezone. | |
| if value.tzinfo is None: | |
| if value.utcoffset() is None: | |
| value = value.replace(tzinfo=timezone.utc) | |
| int_timestamps.append(int(value.timestamp())) |
Sorry, something went wrong.
| @pytest.fixture | ||
| def non_utc_local_timezone(monkeypatch): | ||
| """Run the test with a local timezone that is not UTC, where tzset exists.""" | ||
| if hasattr(time, "tzset"): | ||
| monkeypatch.setenv("TZ", "America/Los_Angeles") | ||
| time.tzset() | ||
| yield | ||
| if hasattr(time, "tzset"): |
There was a problem hiding this comment.
On platforms without time.tzset, this fixture does not change the process timezone, but the test still runs. If that platform happens to use UTC, the test passes without exercising the regression. Consider skipping the test when tzset is unavailable, or otherwise ensuring the test verifies that the local timezone actually changed.
Suggested:
| @pytest.fixture | |
| def non_utc_local_timezone(monkeypatch): | |
| """Run the test with a local timezone that is not UTC, where tzset exists.""" | |
| if hasattr(time, "tzset"): | |
| monkeypatch.setenv("TZ", "America/Los_Angeles") | |
| time.tzset() | |
| yield | |
| if hasattr(time, "tzset"): | |
| if not hasattr(time, "tzset"): | |
| pytest.skip("Changing the process timezone requires time.tzset") | |
| monkeypatch.setenv("TZ", "America/Los_Angeles") | |
| time.tzset() | |
| yield | |
| monkeypatch.undo() | |
| time.tzset() |
Sorry, something went wrong.
| """Run the test with a local timezone that is not UTC, where tzset exists.""" | ||
| if hasattr(time, "tzset"): |
There was a problem hiding this comment.
America/Los_Angeles depends on timezone database availability. In minimal environments without that data, setting TZ may not produce a non-UTC timezone, weakening the regression test. A POSIX-style fixed offset such as UTC+8, or an explicit assertion that the resulting local offset is nonzero, would make the test’s setup more reliable.
Suggested:
| """Run the test with a local timezone that is not UTC, where tzset exists.""" | |
| if hasattr(time, "tzset"): | |
| monkeypatch.setenv("TZ", "UTC+8") | |
| time.tzset() | |
| assert datetime.now().astimezone().utcoffset() != timezone.utc.utcoffset(None) |
Sorry, something went wrong.
| ], | ||
| ) | ||
| def test_naive_datetime_unix_timestamp_is_utc(non_utc_local_timezone, value_type): | ||
| """A naive datetime is read as UTC, not in the local timezone.""" | ||
| naive = datetime(2024, 7, 1, 12, 0, 0) | ||
| value = naive if value_type == ValueType.UNIX_TIMESTAMP else [naive] | ||
|
|
||
| proto = python_values_to_proto_values([value], value_type)[0] | ||
| converted = feast_value_type_to_python_type(proto) | ||
|
|
||
| expected = naive.replace(tzinfo=timezone.utc) | ||
| if value_type == ValueType.UNIX_TIMESTAMP: | ||
| assert converted == expected | ||
| else: |
There was a problem hiding this comment.
The production condition misses datetimes whose tzinfo is non-None but whose utcoffset() is None. Adding a small custom tzinfo test case would lock in Python’s full definition of a naive datetime and prevent this edge case from regressing.
Sorry, something went wrong.
A datetime is naive when utcoffset() returns None, which includes a tzinfo whose utcoffset() is None, not only tzinfo=None. Check utcoffset() so those values are also read as UTC instead of raising. Make the regression test stricter: skip it where time.tzset is not available, use the fixed POSIX offset UTC+8 so no timezone database is needed, assert the local offset really changed, and cover a custom tzinfo whose utcoffset() is None. Signed-off-by: Mohammad Hijjawi <mohammad.hijjawi1997@gmail.com>
|
⚠️ Please install the Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## master #6937 +/- ##
==========================================
+ Coverage 49.06% 49.32% +0.25%
==========================================
Files 435 443 +8
Lines 54528 55080 +552
Branches 7954 8016 +62
==========================================
+ Hits 26752 27166 +414
- Misses 25898 26030 +132
- Partials 1878 1884 +6
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
|
Thanks @jyejare, all four applied in 8983022:
ruff and mypy are clean on the touched files. |
Sorry, something went wrong.
What this PR does / why we need it:
_python_datetime_to_int_timestamp in feast/type_map.py converts a naive datetime.datetime with datetime.timestamp(), which reads a naive value in the machine's local timezone. Everything else treats a naive value as UTC: a naive pd.Timestamp or np.datetime64 in the same function, a naive ZonedTimestamp, and utils.make_tzaware ("We assume tz-naive datetimes are UTC").
So on any host that is not on UTC, UnixTimestamp values (scalar, Array and Set) built from Python datetimes are shifted by the host's UTC offset. The same wall-clock time also lands at a different instant depending on whether it arrives as a datetime or a pd.Timestamp.
Repro: a local sqlite store, a RequestSource with a UnixTimestamp field, and a python-mode ODFV that returns it unchanged. Called with entity_rows=[{"driver_id": 1, "request_ts": datetime(2024, 7, 1, 12)}] on a host in Europe/London:
The fix attaches UTC to a naive datetime before taking its timestamp, the same way the ZONED_TIMESTAMP path already does. Aware datetimes are unchanged.
Which issue(s) this PR fixes:
No existing issue; found while testing timestamp round-trips.
Checks
Testing Strategy
Misc
test_naive_datetime_unix_timestamp_is_utc covers the scalar, list and set types. A fixture switches the local timezone to the fixed offset UTC+8 via TZ + time.tzset() (and skips where tzset is unavailable), so the test fails without the fix even on UTC CI runners. It also covers a tzinfo whose utcoffset() returns None. ruff check/format and mypy on type_map.py are clean.