codeant-ai-for-open-source[bot] commented on code in PR #41579:
URL: https://github.com/apache/superset/pull/41579#discussion_r3498695656
##########
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:
**Suggestion:** This now forces all `DateTime` literals to UTC even when the
input `dttm` is timezone-naive. `convert_dttm` is used outside time-range
bounds too (for example with parsed temporal strings from other query paths),
and those naive values can represent server/session-local wall time; coercing
them to UTC shifts comparisons by the timezone offset and can silently filter
out matching rows. Only emit `'UTC'` when the input is explicitly
UTC-normalized (or make callers pass aware UTC datetimes), instead of treating
every naive datetime as UTC. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Top-N group filters mis-match ClickHouse temporal dimensions.
- ⚠️ Prequery temporal comparisons off on non-UTC ClickHouse servers.
- ⚠️ Temporal dimension filters may exclude matching ClickHouse rows.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a ClickHouse database using the legacy `clickhouse_sqlalchemy`
connector so
it uses `ClickHouseEngineSpec` (engine spec defined in
`superset/db_engine_specs/clickhouse.py`, class `ClickHouseEngineSpec`
starting around
line 143 in the PR hunk).
2. Create a chart that uses the "Top N" group-by feature on a temporal
dimension backed by
a ClickHouse `DateTime` column, so the query pathway uses `_get_top_groups`
in
`superset/connectors/sqla/models.py:1847-1873` (this function iterates
`df.iterrows()` and
calls `self._normalize_prequery_result_type(...)` for each `dimension`).
3. Observe that `_get_top_groups` calls `_normalize_prequery_result_type` in
`superset/models/helpers.py:1377-1458`, where for temporal dimensions with
string values
(`column_.type`/`column_.is_temporal` and `isinstance(value, str)`), the
code parses the
string with `dateutil.parser.parse(value)` (producing a timezone-naive
`datetime`) and
then calls `self.db_engine_spec.convert_dttm(...)` (lines 44-53).
4. For a ClickHouse server configured in a non-UTC timezone (e.g.,
`Europe/Moscow`), the
new `convert_dttm` implementation at
`superset/db_engine_specs/clickhouse.py:127-139`
turns that naive datetime into a literal `toDateTime('{formatted_dttm}',
'UTC')` (lines
137-138), causing ClickHouse to interpret the wall time as UTC instead of
server-local
time; this shifts the effective comparison by the timezone offset so the
generated
`groupby_exprs[dimension] == value` filters in `_get_top_groups` no longer
match the
underlying `DateTime` column values, and expected rows can be silently
excluded from the
result.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=88eb490322b246c5872133b5106a0e7c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=88eb490322b246c5872133b5106a0e7c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/clickhouse.py
**Line:** 137:138
**Comment:**
*Logic Error: This now forces all `DateTime` literals to UTC even when
the input `dttm` is timezone-naive. `convert_dttm` is used outside time-range
bounds too (for example with parsed temporal strings from other query paths),
and those naive values can represent server/session-local wall time; coercing
them to UTC shifts comparisons by the timezone offset and can silently filter
out matching rows. Only emit `'UTC'` when the input is explicitly
UTC-normalized (or make callers pass aware UTC datetimes), instead of treating
every naive datetime as UTC.
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41579&comment_hash=50fa3e4d33cb73449331e0120abbd7161bbbf9ec148f61e09a4164b8b26e4858&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41579&comment_hash=50fa3e4d33cb73449331e0120abbd7161bbbf9ec148f61e09a4164b8b26e4858&reaction=dislike'>👎</a>
--
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]
