You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@superset.apache.org by "porkedchop (via GitHub)" <gi...@apache.org> on 2026/10/15 13:25:35 UTC

[PR] fix(dataset): convert timezone-aware values with the dataset timezone [superset]

porkedchop opened a new pull request, #45344:
URL: https://github.com/apache/superset/pull/45344

### SUMMARY
#37014 added a dataset timezone (`"timezone"` in the dataset's extra). `normalize_dttm_col` applies it by localizing the column to UTC and then converting it to that timezone.

That assumes the values are naive. Columns of a timezone-aware type, like Postgres `timestamptz`, BigQuery `TIMESTAMP` or ClickHouse `DateTime64` with a zone, come out of `SupersetResultSet` as `datetime64[ns, tz]`, and `tz_localize("UTC")` raises `TypeError: Already tz-aware, use tz_convert to convert.` on them. So once a timezone is set on such a dataset, charts that return the column fail.

This only localizes naive values to UTC and converts tz-aware values as they are. Naive columns behave the same as before.

This touches the same block as #45203 (DATE columns), but the two changes are independent.

### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A

### TESTING INSTRUCTIONS
1. On a Postgres dataset with a `timestamptz` column, set the dataset extra to `{"timezone": "America/New_York"}`.
2. Build a table chart that includes the column.
3. Before: the chart fails with "Already tz-aware". After: the values are shown in New York time.

`pytest tests/unit_tests/utils/test_core.py -k tz_aware` fails on master and passes with this change.

### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API

--
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(dataset): convert timezone-aware values with the dataset timezone [superset]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.

Copilot commented on code in PR #45344:
URL: https://github.com/apache/superset/pull/45344#discussion_r4286244912


##########
superset/utils/core.py:
##########
@@ -2188,7 +2188,11 @@ def normalize_dttm_col(
                 if not series.empty and series.notna().any():
                     # Convert UTC to target timezone
                     df[_col.col_label] = (
-                        series.dt.tz_localize("UTC")
+                        (
+                            series.dt.tz_localize("UTC")
+                            if series.dt.tz is None
+                            else series
+                        )
                         .dt.tz_convert(tz)
                         .dt.tz_localize(None)  # Remove timezone info for display
                     )

Review Comment:
   The inline comments state that “Data is stored in UTC” and that the code “First make the datetime UTC-aware”, but the new logic intentionally skips UTC-localization for tz-aware series (converting from their existing timezone instead). Please update these comments to match the actual behavior (e.g., “assume naive values are UTC; if already tz-aware, convert directly”) to avoid future misunderstandings/bugs.



##########
tests/unit_tests/utils/test_core.py:
##########
@@ -473,6 +473,25 @@ def test_normalize_dttm_col_timezone_handles_dst() -> None:
     assert df["date_col"][0].strftime("%Y-%m-%d %H:%M:%S") == "2020-07-01 02:00:00"
 
 
+def test_normalize_dttm_col_timezone_with_tz_aware_values() -> None:
+    """Values that already carry a timezone are converted, not re-localized."""
+    df = pd.DataFrame(
+        {
+            "date_col": pd.to_datetime(
+                ["2020-01-01 00:00:00+00:00", "2020-07-01 05:00:00+05:00"],
+                utc=True,
+            )
+        }
+    )
+    dttm_cols = (DateColumn(col_label="date_col", timezone="Europe/Berlin"),)
+
+    normalize_dttm_col(df, dttm_cols)
+
+    assert is_datetime64_dtype(df["date_col"])

Review Comment:
   This assertion verifies a datetime dtype, but it doesn’t explicitly assert that the timezone info was removed after normalization (a key part of the behavior via `.dt.tz_localize(None)`). Consider also asserting that the result is *not* tz-aware (e.g., using `is_datetime64tz_dtype(...)` or checking `df["date_col"].dt.tz is None`) so the test fails if a future refactor keeps tz info attached.



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(dataset): convert timezone-aware values with the dataset timezone [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.

codeant-ai-for-open-source[bot] commented on PR #45344:
URL: https://github.com/apache/superset/pull/45344#issuecomment-6193304672

   ## CodeAnt PR Risk: Medium Risk
   
   - The PR needs attention before merging because mixed-offset datetime values may fail during timezone conversion.
   - Check whether _process_datetime_column guarantees a datetime dtype; otherwise series.dt.tz can raise for object-dtype values.
   
   **Assessed commit:** `d1ed05841a6e`


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(dataset): convert timezone-aware values with the dataset timezone [superset]

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.

codecov[bot] commented on PR #45344:
URL: https://github.com/apache/superset/pull/45344#issuecomment-6194358470

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45344?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :white_check_mark: All modified and coverable lines are covered by tests.
   :white_check_mark: Project coverage is 64.91%. Comparing base ([`4c546ed`](https://app.codecov.io/gh/apache/superset/commit/4c546edbd8fafe9a0ce22b79d54ccafb5ccd379f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d1ed058`](https://app.codecov.io/gh/apache/superset/commit/d1ed05841a6e118927b7eebe2252bce642b9cc2a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 3 commits behind head on master.
   > :exclamation:  There is a different number of reports uploaded between BASE (4c546ed) and HEAD (d1ed058). Click for more details.
   > 
   > <details><summary>HEAD has 4 uploads less than BASE</summary>
   >
   >| Flag | BASE (4c546ed) | HEAD (d1ed058) |
   >|------|------|------|
   >|python|7|3|
   ></details>
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #45344       +/-   ##
   ===========================================
   - Coverage   83.45%   64.91%   -18.54%     
   ===========================================
     Files        3045     3045               
     Lines      199148   199180       +32     
     Branches    46438    46445        +7     
   ===========================================
   - Hits       166199   129304    -36895     
   - Misses      29723    66962    +37239     
   + Partials     3226     2914      -312     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/superset/pull/45344/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [hive](https://app.codecov.io/gh/apache/superset/pull/45344/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `35.00% <ø> (-0.01%)` | :arrow_down: |
   | [mysql](https://app.codecov.io/gh/apache/superset/pull/45344/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [postgres](https://app.codecov.io/gh/apache/superset/pull/45344/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [presto](https://app.codecov.io/gh/apache/superset/pull/45344/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.76% <ø> (-0.01%)` | :arrow_down: |
   | [python](https://app.codecov.io/gh/apache/superset/pull/45344/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.84% <ø> (-34.13%)` | :arrow_down: |
   | [sqlite](https://app.codecov.io/gh/apache/superset/pull/45344/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.73% <ø> (-0.01%)` | :arrow_down: |
   | [unit](https://app.codecov.io/gh/apache/superset/pull/45344/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   </details>
   
   [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45344?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).   
   :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(dataset): convert timezone-aware values with the dataset timezone [superset]

Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.

netlify[bot] commented on PR #45344:
URL: https://github.com/apache/superset/pull/45344#issuecomment-6200922079

   ### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
   
   
   |  Name | Link |
   |:-:|------------------------|
   |<span aria-hidden="true">🔨</span> Latest commit | 538d5ac206e75316c86b5216925f24e8ecce894f |
   |<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ad12df43d18bde6bf0f4dd0 |
   |<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-45344--superset-docs-preview.netlify.app](https://deploy-preview-45344--superset-docs-preview.netlify.app) |
   |<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br />![QR Code](https://app.netlify.com/qr-code/eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1cmwiOiJodHRwczovL2RlcGxveS1wcmV2aWV3LTQ1MzQ0LS1zdXBlcnNldC1kb2NzLXByZXZpZXcubmV0bGlmeS5hcHAifQ.J0WC_vnrDa4_jDT9mTFetxZvXDKNVT9kuY5QsAVbOc8)<br /><br />_Use your smartphone camera to open QR code link._</details> |
   |<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpLCoAwDAXAu2Rt8VdEvIHHSH0pCmpLDfjDu9vdLOalmMIWlQaiglzifZqz_XKVYOVD1OiyyRN2MfoYPjlJjpC4hntErh2jbuBti7p3kM75ylugou8H) |
   ---
   <!-- [superset-docs-preview Preview](https://deploy-preview-45344--superset-docs-preview.netlify.app) -->
   _To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(dataset): convert timezone-aware values with the dataset timezone [superset]

Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.

codeant-ai-for-open-source[bot] commented on code in PR #45344:
URL: https://github.com/apache/superset/pull/45344#discussion_r4283593950


##########
superset/utils/core.py:
##########
@@ -2188,7 +2188,11 @@ def normalize_dttm_col(
                 if not series.empty and series.notna().any():
                     # Convert UTC to target timezone
                     df[_col.col_label] = (
-                        series.dt.tz_localize("UTC")
+                        (
+                            series.dt.tz_localize("UTC")
+                            if series.dt.tz is None
+                            else series
+                        )

Review Comment:
   **Suggestion:** When mixed UTC offsets produce an object-dtype series, `series.dt.tz` raises `AttributeError`, so charts with those values fail instead of converting.
   
   **Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely` · 🏷️ `Type error`
   
   [![Use CodeAnt Skill](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/use-codeant-skill-flat-v2.svg)](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [![Fix in Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2589365597d647c7bc8da8f9595af5e3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [![Fix in VSCode Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2589365597d647c7bc8da8f9595af5e3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset/utils/core.py
   **Line:** 2191:2195
   **Comment:**
   	*Type Error: When mixed UTC offsets produce an object-dtype series, `series.dt.tz` raises `AttributeError`, so charts with those values fail instead of converting.
   
   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%2F45344&comment_hash=e1ac30e15f96d63dccbd587a1d23b1bbab5ec5196ca6841eee5a40bb46d19e29&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45344&comment_hash=e1ac30e15f96d63dccbd587a1d23b1bbab5ec5196ca6841eee5a40bb46d19e29&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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

Re: [PR] fix(dataset): convert timezone-aware values with the dataset timezone [superset]

Posted by "rusackas (via GitHub)" <gi...@apache.org>.

rusackas merged PR #45344:
URL: https://github.com/apache/superset/pull/45344


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org