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: Migrates TreeMap chart #23741

Merged
merged 9 commits into from
Jun 8, 2023

Conversation

michael-s-molina
Copy link
Member

@michael-s-molina michael-s-molina commented Apr 19, 2023

SUMMARY

This PR migrates the Treemap chart to the ECharts version.

  • Removes the legacy Treemap code and unregisters the plugin.
  • Removes old plugin references throughout the codebase
  • Updates unit tests
  • Updates E2E tests
  • Updates the example dashboards
  • Updates translation files

Note a previous migration existed however the legacy chart wasn't removed and thus new legacy Treemap charts could have been created.

AFTER SCREENSHOT

Screenshot 2023-04-19 at 11 48 43

TESTING INSTRUCTIONS

1 - Make sure all Treemap (legacy) charts were converted to Treemap
2 - Make sure Treemap (legacy) is not available anymore in the viz picker
3 - Make sure that it's possible to revert the migration by executing superset db downgrade and reverting this PR

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

@michael-s-molina
Copy link
Member Author

@villebro @kgabryje Can you execute the migration in your staging environments to make sure it runs successfully and the charts are correctly migrated? I executed it successfully at Airbnb.

@michael-s-molina michael-s-molina marked this pull request as ready for review April 19, 2023 14:57
@michael-s-molina michael-s-molina requested a review from a team as a code owner April 19, 2023 14:57
@michael-s-molina michael-s-molina added risk:breaking-change Issues or PRs that will introduce breaking changes v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch labels Apr 19, 2023
@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

Merging #23741 (efecd50) into master (522eb97) will increase coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head efecd50 differs from pull request most recent head 6bd8548. Consider uploading reports for the commit 6bd8548 to get more accurate results

@@            Coverage Diff             @@
##           master   #23741      +/-   ##
==========================================
+ Coverage   68.42%   68.49%   +0.06%     
==========================================
  Files        1951     1946       -5     
  Lines       75472    75373      -99     
  Branches     8219     8218       -1     
==========================================
- Hits        51640    51624      -16     
+ Misses      21720    21638      -82     
+ Partials     2112     2111       -1     
Flag Coverage Δ
hive 53.49% <ø> (+0.01%) ⬆️
javascript 54.91% <100.00%> (+0.08%) ⬆️
mysql 79.04% <ø> (+0.03%) ⬆️
postgres 79.11% <ø> (+0.03%) ⬆️
presto 53.41% <ø> (+0.01%) ⬆️
python 82.97% <ø> (+0.03%) ⬆️
sqlite 77.64% <ø> (+0.03%) ⬆️
unit 53.82% <ø> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
...ponents/controls/VizTypeControl/VizTypeGallery.tsx 88.23% <ø> (ø)
...-frontend/src/visualizations/presets/MainPreset.js 100.00% <ø> (ø)
superset/examples/supported_charts_dashboard.py 0.00% <ø> (ø)
superset/examples/world_bank.py 30.66% <ø> (ø)
superset/viz.py 62.71% <ø> (+0.40%) ⬆️
...src/dashboard/components/PropertiesModal/index.tsx 64.32% <100.00%> (+0.28%) ⬆️

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

@villebro
Copy link
Member

@michael-s-molina please also 🔥 class TreemapViz(BaseViz) in viz.py!

@EugeneTorap
Copy link
Contributor

EugeneTorap commented Apr 19, 2023

Hello @michael-s-molina @villebro

  1. We can create a discussion where we describe all the legacy chart viz that we want to migrate, as we did for ui pages structure in the src/pages structure proposal.
  2. Do we plan to migrate SQL Lab to SPA in 3.0 version?

@michael-s-molina
Copy link
Member Author

Hello @michael-s-molina @villebro

  1. We can create a discussion where we describe all the legacy chart viz that we want to migrate, as we did for ui pages structure in the src/pages structure proposal.
  2. Do we plan to migrate SQL Lab to SPA in 3.0 version?

Hi @EugeneTorap. The Superset 3.0 project answers both questions. There you'll find all legacy viz migrations that were voted and approved for 3.0. SQL Lab to SPA was not proposed for 3.0.

@EugeneTorap
Copy link
Contributor

@rusackas Can we add move SQL Lab to SPA to the v3.0 proposal? This is super important for us 🙏

@michael-s-molina
Copy link
Member Author

@rusackas Can we add move SQL Lab to SPA to the v3.0 proposal? This is super important for us 🙏

@EugeneTorap There's a 3.0 discussion going on for this type of request.

* specific language governing permissions and limitations
* under the License.
*/
describe('Visualization > Treemap', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the test because it's not applicable anymore. By default, charts are rendered using canvas. Even if we created a test configuration to render the charts using SVG, the groupby colors are not static anymore, they suffer small variations depending on the value. This type of test is more suitable with visual regression testing using Applitools or Chromatic, which is not in the scope of this PR.

# Ensure `slice.params` and `slice.query_context`` in MySQL is MEDIUMTEXT
# before migration, as the migration will save a duplicate form_data backup
# which may significantly increase the size of these fields.
if isinstance(op.get_bind().dialect, MySQLDialect):
Copy link
Member

Choose a reason for hiding this comment

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

It would be great (possibly before) to make sure these columns are defined as a MediumText in the model and had a corresponding migration so we wouldn't need this logic.

Copy link
Member

Choose a reason for hiding this comment

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

@michael-s-molina has there been any update to the underlying model which would then make this redundant?

Copy link
Member Author

Choose a reason for hiding this comment

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

@john-bodley This migration is referenced by a previous script that was created a while ago. Because of that, we need to keep this piece of code even if the models were updated later.

@@ -535,12 +535,6 @@ describe('Dashboard edit', () => {
applyChanges();
saveChanges();

cy.get('.treemap #rect-sum__SP_POP_TOTL').should(
Copy link
Member Author

Choose a reason for hiding this comment

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

These kind of tests should be handled by Applitools or Chromatic. These selectors don't exist anymore now that we're using canvas to draw the charts.

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.

LGTM

operator: '!='
sqlExpression: null
subject: communite_time
- clause: WHERE
Copy link
Member

Choose a reason for hiding this comment

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

Is this not over-indented?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, it does look right - how was this not causing problems before? 😕

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

Hooray! Happy to see this thing finally get killed off!!

predator-arnold-schwarzenegger
— Treemap v1

@michael-s-molina michael-s-molina force-pushed the migrate-treemap branch 2 times, most recently from 3f24cea to b45898f Compare May 23, 2023 19:38
@michael-s-molina michael-s-molina force-pushed the migrate-treemap branch 3 times, most recently from 505121d to 1893774 Compare June 6, 2023 19:16
@michael-s-molina michael-s-molina merged commit af24092 into apache:master Jun 8, 2023
@michael-s-molina michael-s-molina removed the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Jun 26, 2023
@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 hold! On hold risk:breaking-change Issues or PRs that will introduce breaking changes size/XXL 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants