fix: Normalize epoch timestamp generation to UTC#40034
Conversation
Code Review Agent Run #3d9aa8Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
dosubot
Bot
added
change:backend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| import uuid | ||
| from collections.abc import Hashable | ||
| from datetime import datetime, timedelta | ||
| from datetime import UTC, datetime, timedelta |
There was a problem hiding this comment.
Suggestion: datetime.UTC is only available in Python 3.11+, but this project supports Python 3.10. Importing it at module load will raise an ImportError on 3.10 and break startup. Use timezone.utc (or a compatibility fallback) instead. [import error]
Severity Level: Critical 🚨
- ❌ Superset backend crashes on Python 3.10 startup.
- ❌ Unit tests importing SqlaTable fail with ImportError.Steps of Reproduction ✅
1. Confirm supported Python versions: open `superset-core/pyproject.toml` and see
`requires-python = ">=3.10"` and classifiers for `3.10`, `3.11`, `3.12` at lines 10–20.
2. Start a Python 3.10 environment and run any code that imports `SqlaTable` from
`superset.connectors.sqla.models` (e.g., `pytest
tests/unit_tests/models/core_test.py::test_dttm_sql_literal` which uses `SqlaTable` at
`tests/unit_tests/models/core_test.py:51-57`).
3. Importing `tests/unit_tests/models/core_test.py` causes Python to import
`superset.connectors.sqla.models`, which in turn imports `superset.models.helpers` via
`from superset.models.helpers import (AuditMixinNullable, CertificationMixin,
ExploreMixin, ImportExportMixin, QueryResult, SQLA_QUERY_KEYS)` at
`superset/connectors/sqla/models.py:18-25`.
4. When `superset.models.helpers` is imported on Python 3.10, the statement `from datetime
import UTC, datetime, timedelta` at `superset/models/helpers.py:29` executes, raising
`ImportError: cannot import name 'UTC' from 'datetime'` because `datetime.UTC` does not
exist before Python 3.11; this prevents the application and tests from starting.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 29:29
**Comment:**
*Import Error: `datetime.UTC` is only available in Python 3.11+, but this project supports Python 3.10. Importing it at module load will raise an ImportError on 3.10 and break startup. Use `timezone.utc` (or a compatibility fallback) instead.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if tf: | ||
| if tf in {"epoch_ms", "epoch_s"}: | ||
| seconds_since_epoch = int(dttm.timestamp()) | ||
| seconds_since_epoch = int(dttm.replace(tzinfo=UTC).timestamp()) |
There was a problem hiding this comment.
Suggestion: This conversion overwrites timezone info for every datetime, including timezone-aware values, which changes the represented instant for non-UTC inputs. For aware datetimes, convert with astimezone(UTC); only treat naive datetimes as UTC before timestamp conversion. [logic error]
Severity Level: Critical 🚨
- ❌ Time-based filters miscompute bounds for aware datetimes.
- ⚠️ Charts and reports show shifted or missing time ranges.Steps of Reproduction ✅
1. Note that `SqlaTable` in `superset/connectors/sqla/models.py:1219-1227` subclasses
`ExploreMixin` from `superset.models.helpers`, so `SqlaTable.dttm_sql_literal()` is
implemented by `ExploreMixin.dttm_sql_literal` at `superset/models/helpers.py:2370-2398`.
2. In a Python session (or new unit test alongside
`tests/unit_tests/models/core_test.py:test_dttm_sql_literal`), construct a timezone-aware
datetime: `dttm = pytz.timezone("US/Eastern").localize(datetime(2023, 1, 1, 1, 23, 45,
600000))`.
3. Construct a column that uses epoch formatting: `col =
TableColumn(python_date_format="epoch_s")`, and a database: `db =
Database(sqlalchemy_uri="sqlite://")`, then call
`SqlaTable(database=db).dttm_sql_literal(dttm, col)`.
4. Inside `dttm_sql_literal`, since `tf == "epoch_s"`, the code executes
`seconds_since_epoch = int(dttm.replace(tzinfo=UTC).timestamp())` at
`superset/models/helpers.py:2392`, which discards the original `US/Eastern` offset and
treats `01:23:45` as UTC; the returned epoch seconds are off by the timezone offset
compared to the correct value `int(dttm.astimezone(UTC).timestamp())`, leading to
incorrect SQL time filters when timezone-aware datetimes are used.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 2392:2392
**Comment:**
*Logic Error: This conversion overwrites timezone info for every datetime, including timezone-aware values, which changes the represented instant for non-UTC inputs. For aware datetimes, convert with `astimezone(UTC)`; only treat naive datetimes as UTC before timestamp conversion.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
This relates closely to #39782 - that one looks at the tests, but "works around" the implementation without testing the actual code path this addresses. However, as noted by bots, this PR changes the import to Hopefully that'll work, but if not, it may be time to consider making the case for a 3.11 requirement, which could be a "beaking change" requiring waiting for a major version. Let me know what you think. |
rusackas
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40034 +/- ##
==========================================
- Coverage 63.83% 63.21% -0.62%
==========================================
Files 2587 2587
Lines 137317 137317
Branches 31770 31770
==========================================
- Hits 87650 86809 -841
- Misses 48149 48986 +837
- Partials 1518 1522 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Normalizes naive datetime epoch conversion in dttm_sql_literal() to use UTC explicitly, avoiding dependence on the local machine timezone.
Changes:
- Import
UTCfromdatetime. - Replace
dttm.timestamp()withdttm.replace(tzinfo=UTC).timestamp()for epoch literals. - Incidental trailing whitespace added near end of file.
| import uuid | ||
| from collections.abc import Hashable | ||
| from datetime import datetime, timedelta | ||
| from datetime import UTC, datetime, timedelta |
| if tf: | ||
| if tf in {"epoch_ms", "epoch_s"}: | ||
| seconds_since_epoch = int(dttm.timestamp()) | ||
| seconds_since_epoch = int(dttm.replace(tzinfo=UTC).timestamp()) |
| sqla_query=qry, | ||
| prequeries=prequeries, | ||
| ) | ||
| ) |
| if tf: | ||
| if tf in {"epoch_ms", "epoch_s"}: | ||
| seconds_since_epoch = int(dttm.timestamp()) | ||
| seconds_since_epoch = int(dttm.replace(tzinfo=UTC).timestamp()) |
Fixes #39781.
Normalizes epoch timestamp generation to UTC when handling timezone-naive datetimes in
dttm_sql_literal().Previously,
datetime.timestamp()depended on the local machine timezone, causingtest_dttm_sql_literal()to fail on non-UTC environments.This change ensures deterministic epoch conversion behavior across environments by explicitly treating naive datetimes as UTC before generating epoch literals.
Note: I was unable to fully run the local test suite because the Superset development dependencies are not fully installed in my environment (
alembicmissing).