codeant-ai-for-open-source[bot] commented on code in PR #40034:
URL: https://github.com/apache/superset/pull/40034#discussion_r3221364363
##########
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:
**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]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Time-based filters miscompute bounds for aware datetimes.
- ⚠️ Charts and reports show shifted or missing time ranges.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmodels%2Fhelpers.py%0A%2A%2ALine%3A%2A%2A%202392%3A2392%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20This%20conversion%20overwrites%20timezone%20info%20for%20every%20datetime%2C%20including%20timezone-aware%20values%2C%20which%20changes%20the%20represented%20instant%20for%20non-UTC%20inputs.%20For%20aware%20datetimes%2C%20convert%20with%20%60astimezone%28UTC%29%60%3B%20only%20treat%20naive%20datetimes%20as%20UTC%20before%20timestamp%20conversion.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20res
t%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmodels%2Fhelpers.py%0A%2A%2ALine%3A%2A%2A%202392%3A2392%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20This%20conversion%20overwrites%20timezone%20info%20for%20every%20datetime%2C%20including%20timezone-aware%20values%2C%20which%20changes%20the%20represented%20instant%20for%20non-UTC%20inputs.%20For%20aware%20datetimes%2C%20convert%20with%20%60astimezone%28UTC%29%60%3B%20only%20treat%20naive%20datetimes%20as%20UTC%20before%20timestamp%20conversion.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20
make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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/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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40034&comment_hash=6d9d6fc7efc524f2d76b2394bb677f63291dde2077fef762ff9877a0666f4b0e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40034&comment_hash=6d9d6fc7efc524f2d76b2394bb677f63291dde2077fef762ff9877a0666f4b0e&reaction=dislike'>👎</a>
##########
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:
**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]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Superset backend crashes on Python 3.10 startup.
- ❌ Unit tests importing SqlaTable fail with ImportError.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmodels%2Fhelpers.py%0A%2A%2ALine%3A%2A%2A%2029%3A29%0A%2A%2AComment%3A%2A%2A%0A%09%2AImport%20Error%3A%20%60datetime.UTC%60%20is%20only%20available%20in%20Python%203.11%2B%2C%20but%20this%20project%20supports%20Python%203.10.%20Importing%20it%20at%20module%20load%20will%20raise%20an%20ImportError%20on%203.10%20and%20break%20startup.%20Use%20%60timezone.utc%60%20%28or%20a%20compatibility%20fallback%29%20instead.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%2
0said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmodels%2Fhelpers.py%0A%2A%2ALine%3A%2A%2A%2029%3A29%0A%2A%2AComment%3A%2A%2A%0A%09%2AImport%20Error%3A%20%60datetime.UTC%60%20is%20only%20available%20in%20Python%203.11%2B%2C%20but%20this%20project%20supports%20Python%203.10.%20Importing%20it%20at%20module%20load%20will%20raise%20an%20ImportError%20on%203.10%20and%20break%20startup.%20Use%20%60timezone.utc%60%20%28or%20a%20compatibility%20fallback%29%20instead.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comme
nts%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(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/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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40034&comment_hash=1dd7eee0bfb68a170a6fbecbf743d70a6fad1f37d4e9c0bf08f6a6c7b99494e4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40034&comment_hash=1dd7eee0bfb68a170a6fbecbf743d70a6fad1f37d4e9c0bf08f6a6c7b99494e4&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]
