Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(echarts): Implement stream graph for Echarts Timeseries #23410

Merged
merged 13 commits into from
Mar 20, 2023

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Mar 17, 2023

SUMMARY

Echarts don't support stream graph for area chart. This PR implements stream graph for Echarts Area and other Timeseries charts to achieve feature parity with legacy NVD3 charts.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Stream graph area chart:
image

image

Stream graph bar chart:
image

Stream graph area chart with categorical x axis:
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
      Current: 0.21 s
      10+: 0.12 s
      100+: 0.11 s
      1000+: 0.12 s
  • Introduces new feature or API
  • Removes existing feature or API

@villebro
Copy link
Member

image

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

First pass comments, code looks good at first glance, but would be good to add a simple test case for the wiggle algo

@kgabryje kgabryje requested a review from a team as a code owner March 17, 2023 12:49
@michael-s-molina
Copy link
Member

applause

@codecov
Copy link

codecov bot commented Mar 17, 2023

Codecov Report

Merging #23410 (88e378f) into master (e3a7d20) will increase coverage by 0.15%.
The diff coverage is 87.59%.

❗ Current head 88e378f differs from pull request most recent head 9b4a0e8. Consider uploading reports for the commit 9b4a0e8 to get more accurate results

@@            Coverage Diff             @@
##           master   #23410      +/-   ##
==========================================
+ Coverage   67.49%   67.65%   +0.15%     
==========================================
  Files        1907     1908       +1     
  Lines       73531    73716     +185     
  Branches     7981     7989       +8     
==========================================
+ Hits        49633    49875     +242     
+ Misses      21850    21793      -57     
  Partials     2048     2048              
Flag Coverage Δ
hive 52.73% <49.21%> (?)
javascript 53.87% <74.62%> (+0.04%) ⬆️
mysql 78.45% <76.43%> (-0.01%) ⬇️
postgres 78.51% <76.43%> (-0.02%) ⬇️
presto 52.66% <49.73%> (-0.03%) ⬇️
python 82.35% <92.14%> (+0.23%) ⬆️
sqlite 77.02% <81.15%> (+0.02%) ⬆️
unit 52.52% <61.25%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
...s/legacy-plugin-chart-country-map/src/countries.ts 100.00% <ø> (ø)
...chart-echarts/src/Timeseries/Area/controlPanel.tsx 40.00% <ø> (ø)
...charts/src/Timeseries/Regular/Bar/controlPanel.tsx 35.71% <ø> (ø)
...gin-chart-echarts/src/components/ExtraControls.tsx 0.00% <0.00%> (ø)
...d/plugins/plugin-chart-echarts/src/utils/series.ts 88.48% <0.00%> (+1.43%) ⬆️
superset-frontend/src/SqlLab/actions/sqlLab.js 64.05% <0.00%> (ø)
superset-frontend/src/SqlLab/reducers/sqlLab.js 36.87% <ø> (ø)
superset/dashboards/commands/importers/v1/utils.py 85.00% <50.00%> (-0.90%) ⬇️
superset/db_engine_specs/snowflake.py 66.86% <57.14%> (+1.24%) ⬆️
...gin-chart-echarts/src/Timeseries/transformProps.ts 58.33% <66.66%> (+1.19%) ⬆️
... and 17 more

... and 2 files with indirect coverage changes

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

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

When I created a stream bar chart, then changed the chart type to line chart, the lines went completely missing:
image

I wonder if it's because the stacking control type changed from string to boolean based? Also, I think we may need to migrate all ECharts plugins that are using the boolean based stacking control to the new string based one to avoid clashes. Optionally, we may need to add one of those form data migrators that we're using when switching between some other charts to handle the control value conversions.


Base = declarative_base()

CHART_TYPE = "echarts_timeseries_bar"
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we also migrate the other ECharts "timeseries" like plugins to use the new select-based control? Here I'm using the ECharts "Generic Chart", and the stack control is still a checkbox:
image

@kgabryje
Copy link
Member Author

Thanks for comments @villebro, updated the PR. Now all timeseries echarts support stream graph :)
image

@kgabryje
Copy link
Member Author

@villebro For some reason the updated import paths (import from @superset-ui/plugin-chart-echarts instead of ../../../controls) broke the unit tests. I reverted the change to unblock the CI. We can try to resolve it in a separate PR

@villebro
Copy link
Member

@villebro For some reason the updated import paths (import from @superset-ui/plugin-chart-echarts instead of ../../../controls) broke the unit tests. I reverted the change to unblock the CI. We can try to resolve it in a separate PR

Oof, my bad, I totally forgot those tests want those relative imports.. Carry on!

@kgabryje kgabryje changed the title feat(echarts): Implement stream graph for Area chart feat(echarts): Implement stream graph for Echarts Timeseries Mar 17, 2023
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Left three major comments that turned out to be incorrect upon closer inspection 😄 But I think that one naming nit still has merit, I think (unless it also turns out to be incorrect). Tested and works great, so LGTM with an optional nit.

@@ -116,6 +117,7 @@ const config: ControlPanelConfig = {
},
],
[onlyTotalControl],
[percentageThresholdControl],
Copy link
Member

Choose a reason for hiding this comment

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

Oh nice bycatch

@villebro
Copy link
Member

Getting ready to kill NVD3..
image

@kgabryje
Copy link
Member Author

Getting ready to kill NVD3.. image

I'll bring the garlic!

@kgabryje kgabryje merged commit b0d83e8 into apache:master Mar 20, 2023
@stephenLYZ
Copy link
Member

OMG. It's amazing!

@rusackas
Copy link
Member

LOVE this :) Nice work!

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants