feat: migrate bubble chart to echarts

[mayurnewase]

SUMMARY

Migrates D3 bubble chart to Echart's scatter. Bubble chart has different controls than scatter, so replacing it first then plan to combine scatter and bubble chart types later.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

1. Chart and data control panel:

2. Customize control panel

3. Tooltip:

TESTING INSTRUCTIONS

1. create d3 bubble chart
2. merge commits from this PR and open existing chart should work fine as before.
3. test all controls in data and customize sections.

ADDITIONAL INFORMATION

• [ ] Has associated issue:
• [ ] Required feature flags:
• [x] Changes UI
• [x] Includes DB Migration (follow approval process in SIP-59)
  • [x] Migration is atomic, supports rollback & is backwards-compatible
  • [x] Confirm DB migration upgrade and downgrade tested
  • [ ] Runtime estimates and downtime expectations provided
• [ ] Introduces new feature or API
• [ ] Removes existing feature or API

[codecov[bot]]

Codecov Report

Merging #22107 (df22d86) into master (389e44e) will increase coverage by 0.26%. The diff coverage is 74.46%.

@@            Coverage Diff             @@
##           master   #22107      +/-   ##
==========================================
+ Coverage   66.65%   66.91%   +0.26%     
==========================================
  Files        1841     1852      +11     
  Lines       70220    70316      +96     
  Branches     7670     7684      +14     
==========================================
+ Hits        46804    47052     +248     
+ Misses      21434    21268     -166     
- Partials     1982     1996      +14     

Flag	Coverage Δ
hive	52.55% <ø> (?)
javascript	53.84% <74.46%> (+0.18%)	:arrow_up:
mysql	78.04% <ø> (-0.03%)	:arrow_down:
postgres	78.11% <ø> (-0.03%)	:arrow_down:
presto	52.45% <ø> (?)
python	81.30% <ø> (+0.36%)	:arrow_up:
sqlite	76.57% <ø> (?)
unit	50.86% <ø> (-0.01%)	:arrow_down:

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

Impacted Files	Coverage Δ
.../plugin-chart-echarts/src/Bubble/EchartsBubble.tsx	0.00% <0.00%> (ø)
...ntend/plugins/plugin-chart-echarts/src/defaults.ts	100.00% <ø> (+80.00%)	:arrow_up:
...-frontend/src/visualizations/presets/MainPreset.js	100.00% <ø> (ø)
...s/plugin-chart-echarts/src/Bubble/controlPanel.tsx	50.00% <50.00%> (ø)
...d/plugins/plugin-chart-echarts/src/Bubble/index.ts	50.00% <50.00%> (ø)
...gins/plugin-chart-echarts/src/Bubble/buildQuery.ts	66.66% <66.66%> (ø)
.../plugin-chart-echarts/src/Bubble/transformProps.ts	82.35% <82.35%> (ø)
...ugins/plugin-chart-echarts/src/Bubble/constants.ts	100.00% <100.00%> (ø)
...ugins/legacy-preset-chart-nvd3/src/Bubble/index.js	33.33% <0.00%> (-33.34%)	:arrow_down:
...c/components/Chart/DrillDetail/DrillDetailPane.tsx	76.04% <0.00%> (-6.68%)	:arrow_down:
... and 31 more

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

[mayurnewase]

First pass comments, looks great! One thing that I noticed is that the NVD3 plugin assumes truncation on the y-axis if the bounds aren't set. While I don't necessarily like the idea of defaulting to truncation (actually, I strongly oppose it), I was just wondering if we should do a migration that enables truncation for existing charts if the bounds are undefined?

agree, added migration.

[mayurnewase]

I just saw the original bubble chart(NVD3 version), and the limit control have an inconsistent setting. IMO, we should fix/improve that in the new version of Bubble Chart.

1. The Series Limit refers to a limit by series rather than a row limit.
2. No chance to set a order by clause in the SQL

here series limit made sense as its inline with echart's options.(every bubble is separate series) and description of dimension in control panel had series in it to group them by color.

[zhaoyongjie]

@mayurnewase the series limit will send a subquery instead a simple query, the time-series chart should be referenced.

[mayurnewase]

@mayurnewase the series limit will send a subquery instead a simple query, the time-series chart should be referenced.

I see, let me check.

[villebro]

FYI @mayurnewase @zhaoyongjie , slightly related, I'm opening a PR today that will clean up deprecated limit control logic from superset-ui/core so we can remove those annoying deprecation warnings on chart data requests

[mayurnewase]

updated series limit to row limit, and added order control.

@zhaoyongjie the normalizeOrderBy in core doesn't support this control panel yet and I didn't want to change anything in core in this PR so added order support in the buildQuery itself. Either this or need to something more hacky like below

orderby: normalizeOrderBy( { ...baseQueryObject, orderby: [ [...baseQueryObject.orderby, !baseQueryObject.order_desc] ] } ).orderby

what do you think?

[rusackas]

Just seeing this and catching up on the thread - this is great, thanks!

I haven't looked at the code yet, but for myself or any other reviewer, I'm hopeful that this (or a followup) includes the latest best practices: • Generic X Axis • Standardized controls (normalizing/denormalizing for fast-viz switching) • Drill to Detail support

Basically, trying to keep all the ECharts plugins current with all the first-class bells and whistles. Happy to provide support/references for any of the above if needed.

Again, thanks, this is exciting!

[mayurnewase]

Just seeing this and catching up on the thread - this is great, thanks!

I haven't looked at the code yet, but for myself or any other reviewer, I'm hopeful that this (or a followup) includes the latest best practices: • Generic X Axis • Standardized controls (normalizing/denormalizing for fast-viz switching) • Drill to Detail support

Basically, trying to keep all the ECharts plugins current with all the first-class bells and whistles. Happy to provide support/references for any of the above if needed.

Again, thanks, this is exciting!

@rusackas

1. x and y axes are both metrics
2. controls are standard but for fast switching don't think there is any other chart with metrics on x and y and 1 more for bubble size, so not sure how I can test the switch.
3. I will add cross filter and drill to detail as soon as this PR gets merged, because it may need other modifications and new tests.

possible to get 1 more review?

[rusackas]

/testenv up

[github-actions[bot]]

@rusackas Ephemeral environment spinning up at http://54.186.55.19:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[rusackas]

Is there any intent to support temporal or categorical X-axis on this? I suppose not, if the chart it's replacing, and that's more of a "phase 2" thing

Also, the ephemeral environment is up and running if any of the earlier reviewers wouldn't mind giving this another pass.

[rusackas]

There are a few things from the old chart that aren't supported in this new one • Display of X bounds and Y bounds (the extent numbers on the axes) • Auto-rotation of axis labels • Axis limit inputs (love 'em or hate 'em) • Series limit (this one seems like it would be important) • Max Bubble Size

While I think most people would be fine with most of the above, it might be a bit contentious to call it a drop-in replacement without complete feature parity. We've learned this lesson before and been stuck with difficult reverts. I see a few options:

1. Go for complete feature parity (which we might not want to!)
2. Post a DISCUSS thread asking for lazy consensus on the dev list, to see if others are OK with this change, and proceed accordingly
3. Add this as a new chart, mark the current one as legacy, and live with both in the product until we can make a breaking change (3.0)

I'm thinking option 2 might be prudent. I LOVE this PR and would love to see any NVD3 chart die, but I want to make sure we're not pushing any (admittedly small) breaking changes on people without at least some form of official consent.

[mayurnewase]

1. Display of X bounds and Wy bounds (the extent numbers on the axes) -> here bounds are visible by default.

2. Axis limit inputs -> just like old chart this one has Y axis limit -> need to click on Truncate Y Axis

3. Auto rotation -> I didnt add this for consistency, as its not present in other echart charts.

4. Series Limit -> has been changed to Row Limit as per review from @zhaoyongjie above to be inline with newer updates.

5. Max bubble size -> its there in customize section.

I tested above on ephemeral deployment.

[rusackas]

@mayurnewase this appears to need a rebase. It might also need some commits added that change the old bubble chart name to include "legacy." @villebro has done some of this type of renaming and can probably warn us of any intricacies here.

[EugeneTorap]

Hi @mayurnewase! Do you have some time to finish the PR?

[mayurnewase]

@EugeneTorap unfortunately no, feel free to take over or close this PR.

[john-bodley]

cc @michael-s-molina for context who has been working on other EChart migrations.

[michael-s-molina]

Hi @mayurnewase. Thank you for this contribution. I would like to continue the work and see if we can ship this feature. To do that I can push to your PR if you give me the GitHub permission or I can close this PR, open my own and reference you as the original author. Which method do you prefer?

[mayurnewase]

Hey thanks for taking this up, I am ok with both methods. Added you as a collaborator in my fork of this repo, and I see Maintainers are allowed to edit this pull request. in the right panel.

[michael-s-molina]

Added you as a collaborator in my fork of this repo, and I see Maintainers are allowed to edit this pull request. in the right panel.

Thanks @mayurnewase. I'll rebase the PR and modify its scope a little bit to avoid introducing a breaking change. I'll move the migration script to its own PR, similar to https://github.com/apache/superset/pull/23973 and also keep the legacy plugin. The new scope will be to just add the ECharts Bubble chart which will allow us to merge this PR without the need to wait for a major version release.

[michael-s-molina]

I deprecated the legacy chart and added thumbnails for the new version.

[michael-s-molina]

/testenv up

[github-actions[bot]]

@michael-s-molina Ephemeral environment spinning up at http://34.217.102.24:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[michael-s-molina]

Thanks for the review @villebro!

But as discussed a few months ago, I think we have some tooltip harmonization and re-styling work to do anyway, so maybe this can be deferred to that effort.

Yes, let's tackle this in a follow-up as part of the harmonization and re-styling work.

[github-actions[bot]]

Ephemeral environment shutdown and build artifacts deleted.