Skip to content

fix(plugin-chart-echarts): normalize temporal string groupbys#24134

Merged
villebro merged 1 commit into
apache:masterfrom
villebro:villebro/fix-timestamp-format
May 19, 2023
Merged

fix(plugin-chart-echarts): normalize temporal string groupbys#24134
villebro merged 1 commit into
apache:masterfrom
villebro:villebro/fix-timestamp-format

Conversation

@villebro
Copy link
Copy Markdown
Member

SUMMARY

#23339 introduced the possibility of normalizing non-UTC timestamps so that they produced correctly formatted timestamps. However, the normalizer wasn't being used in the ECharts plugin, causing string based non-timezone timestamps to display incorrectly when used as a groupby.

This adds a call to the normalizeTimestamp util in formatSeriesName to ensure that string based timestamps are normalized prior to being fed to the timestamp formatter.

AFTER

Now the timestamp is formatted correctly:
image

BEFORE

Previously the timestamp was assumed to be in local timezone, causing the formatted timestamp to be offset by the UTC offset (this was run in UTC+0200):
image

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bycatch: the getLegendProps block was incorrectly nested inside the formatSeriesName block - check with "Hide whitespace" to see the relevant changes.

@villebro villebro force-pushed the villebro/fix-timestamp-format branch from 5603c21 to 48b36ee Compare May 19, 2023 11:45
@villebro villebro added the 2.1.1 label May 19, 2023
@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2023

Codecov Report

Merging #24134 (381e255) into master (6b54591) will decrease coverage by 0.02%.
The diff coverage is 85.86%.

❗ Current head 381e255 differs from pull request most recent head 48b36ee. Consider uploading reports for the commit 48b36ee to get more accurate results

@@            Coverage Diff             @@
##           master   #24134      +/-   ##
==========================================
- Coverage   68.27%   68.26%   -0.02%     
==========================================
  Files        1952     1952              
  Lines       75468    75397      -71     
  Branches     8202     8204       +2     
==========================================
- Hits        51529    51472      -57     
+ Misses      21832    21819      -13     
+ Partials     2107     2106       -1     
Flag Coverage Δ
javascript 54.68% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/datasets/commands/importers/v1/utils.py 79.41% <0.00%> (+0.77%) ⬆️
superset/db_engine_specs/databricks.py 70.92% <0.00%> (+0.49%) ⬆️
superset/db_engine_specs/snowflake.py 67.64% <0.00%> (+0.39%) ⬆️
superset/migrations/env.py 0.00% <0.00%> (ø)
superset/models/helpers.py 69.90% <0.00%> (+0.07%) ⬆️
superset/views/api.py 68.75% <0.00%> (+1.05%) ⬆️
superset/charts/api.py 86.09% <50.00%> (+0.23%) ⬆️
superset/viz.py 61.36% <50.00%> (+0.04%) ⬆️
superset/views/core.py 75.28% <86.66%> (-0.13%) ⬇️
...d/plugins/plugin-chart-echarts/src/utils/series.ts 86.93% <100.00%> (+0.64%) ⬆️
... and 42 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@villebro villebro merged commit f817c10 into apache:master May 19, 2023
@eschutho eschutho added the v2.1 label Jul 11, 2023
@eschutho
Copy link
Copy Markdown
Member

Removing this PR from 2.1 since normalize non-iso timestamps is is not in 2.1.0

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 First shipped in 3.0.0 labels Mar 8, 2024
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 3.0.0 First shipped in 3.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants