innovark37 commented on code in PR #41579:
URL: https://github.com/apache/superset/pull/41579#discussion_r3498803605
##########
superset/db_engine_specs/clickhouse.py:
##########
@@ -132,7 +132,10 @@ def convert_dttm(
if isinstance(sqla_type, types.Date):
return f"toDate('{dttm.date().isoformat()}')"
if isinstance(sqla_type, types.DateTime):
- return f"""toDateTime('{dttm.isoformat(sep=" ",
timespec="seconds")}')"""
+ if dttm.tzinfo is not None and dttm.utcoffset() is not None:
+ dttm = dttm.astimezone(timezone.utc).replace(tzinfo=None)
+ formatted_dttm = dttm.isoformat(sep=" ", timespec="seconds")
+ return f"toDateTime('{formatted_dttm}', 'UTC')"
Review Comment:
I don’t think we should gate the UTC argument on `tzinfo`.
The bug being fixed happens specifically because Superset passes
UTC-normalized time filter bounds as timezone-naive Python `datetime` values.
If we only emit `toDateTime(..., 'UTC')` for aware datetimes, the affected time
filters would continue to generate `toDateTime('...')`, and ClickHouse would
still interpret those literals in the server/session timezone.
This change makes ClickHouse interpret Superset-generated datetime literals
according to Superset’s UTC time-filter semantics instead of making the result
depend on the ClickHouse server/session timezone. Aware datetimes are
normalized before rendering so an offset-bearing value is not mislabeled as UTC.
##########
tests/unit_tests/db_engine_specs/test_clickhouse.py:
##########
@@ -62,6 +62,26 @@ def test_convert_dttm(
assert_convert_dttm(spec, target_type, expected_result, dttm)
+def test_convert_dttm_normalizes_aware_datetime_to_utc() -> None:
+ from superset.db_engine_specs.clickhouse import (
+ ClickHouseEngineSpec as spec, # noqa: N813
+ )
+
+ aware_dttm = datetime(
+ 2026,
+ 6,
+ 30,
+ 12,
+ 30,
+ tzinfo=timezone(timedelta(hours=3)),
+ )
Review Comment:
Updated. Added an explicit `datetime` annotation for `aware_dttm`.
##########
superset/db_engine_specs/clickhouse.py:
##########
@@ -132,7 +132,10 @@ def convert_dttm(
if isinstance(sqla_type, types.Date):
return f"toDate('{dttm.date().isoformat()}')"
if isinstance(sqla_type, types.DateTime):
- return f"""toDateTime('{dttm.isoformat(sep=" ",
timespec="seconds")}')"""
+ if dttm.tzinfo is not None and dttm.utcoffset() is not None:
+ dttm = dttm.astimezone(timezone.utc).replace(tzinfo=None)
+ formatted_dttm = dttm.isoformat(sep=" ", timespec="seconds")
Review Comment:
Updated. Added an explicit `str` annotation for `formatted_dttm`.
--
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]
