Copilot commented on code in PR #40034:
URL: https://github.com/apache/superset/pull/40034#discussion_r3236494186
##########
superset/models/helpers.py:
##########
@@ -2389,7 +2389,7 @@ def dttm_sql_literal(self, dttm: datetime, col:
"TableColumn") -> str:
if tf:
if tf in {"epoch_ms", "epoch_s"}:
- seconds_since_epoch = int(dttm.timestamp())
+ seconds_since_epoch = int(dttm.replace(tzinfo=UTC).timestamp())
Review Comment:
`dttm.replace(tzinfo=UTC)` unconditionally overwrites any existing tzinfo.
If a timezone-aware `dttm` (e.g. in a non-UTC zone) is passed in, this silently
reinterprets the wall-clock time as UTC and produces an incorrect epoch value.
Consider only attaching UTC when `dttm.tzinfo is None` (or converting via
`astimezone(UTC)` otherwise).
##########
superset/models/helpers.py:
##########
@@ -3532,4 +3532,4 @@ def _create_top_groups_condition(col_name: str, expr:
Any) -> Any:
labels_expected=labels_expected,
sqla_query=qry,
prequeries=prequeries,
- )
+ )
Review Comment:
Unrelated trailing whitespace introduced on this line. Please revert to keep
the diff focused on the fix.
##########
superset/models/helpers.py:
##########
@@ -2389,7 +2389,7 @@ def dttm_sql_literal(self, dttm: datetime, col:
"TableColumn") -> str:
if tf:
if tf in {"epoch_ms", "epoch_s"}:
- seconds_since_epoch = int(dttm.timestamp())
+ seconds_since_epoch = int(dttm.replace(tzinfo=UTC).timestamp())
Review Comment:
Since the PR description references `test_dttm_sql_literal()` failing in
non-UTC environments, consider adding/updating a test that exercises this
branch with a tz-aware datetime to lock in the new behavior and prevent
regressions.
##########
superset/models/helpers.py:
##########
@@ -26,7 +26,7 @@
import re
import uuid
from collections.abc import Hashable
-from datetime import datetime, timedelta
+from datetime import UTC, datetime, timedelta
Review Comment:
`datetime.UTC` was only added in Python 3.11. If Superset still supports
Python 3.10 or earlier, this import will raise `ImportError` at module load
time and break the application. Prefer `from datetime import timezone` and use
`timezone.utc` for broader compatibility.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
