feat: TreeMap migration #20346

zhaoyongjie · 2022-06-10T07:46:25Z

SUMMARY

This PR intends to introduce an approach that migrates from v1 viz to v2 viz. The first PR implement TreeMap migration.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

# upgrade
superset db upgrade

# downgrade
superset db downgrade

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

superset/utils/migrate_viz.py

zhaoyongjie · 2022-06-10T09:24:13Z

...igrations/versions/2021-05-27_16-10_6f139c533bea_add_advanced_data_types_to_column_models.py

-"""adding_advanced_data_type.py
+"""adding advanced data type to column models
+
 Revision ID: 6f139c533bea
 Revises: cbe71abde154
 Create Date: 2021-05-27 16:10:59.567684
+


bycatch: pretty output

codecov · 2022-06-10T09:33:18Z

Codecov Report

Merging #20346 (0aa273e) into master (90d486a) will decrease coverage by 0.03%.
The diff coverage is 69.47%.

❗ Current head 0aa273e differs from pull request most recent head b1d10bd. Consider uploading reports for the commit b1d10bd to get more accurate results

@@            Coverage Diff             @@
##           master   #20346      +/-   ##
==========================================
- Coverage   66.71%   66.68%   -0.04%     
==========================================
  Files        1752     1753       +1     
  Lines       65478    65710     +232     
  Branches     6916     6940      +24     
==========================================
+ Hits        43681    43816     +135     
- Misses      20049    20132      +83     
- Partials     1748     1762      +14

Flag	Coverage Δ
mysql	`82.41% <96.27%> (+0.04%)`	⬆️
postgres	`82.48% <96.27%> (+0.04%)`	⬆️
presto	`?`
python	`82.56% <96.27%> (-0.09%)`	⬇️
sqlite	`82.27% <96.27%> (+0.04%)`	⬆️
unit	`?`

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

Impacted Files	Coverage Δ
...t-ui-chart-controls/src/operators/pivotOperator.ts	`100.00% <ø> (ø)`
...controls/src/operators/timeComparePivotOperator.ts	`100.00% <ø> (ø)`
...t-controls/src/sections/echartsTimeSeriesQuery.tsx	`50.00% <ø> (ø)`
...d/packages/superset-ui-chart-controls/src/types.ts	`100.00% <ø> (ø)`
...superset-ui-core/src/query/types/PostProcessing.ts	`100.00% <ø> (ø)`
...egacy-preset-chart-deckgl/src/layers/Grid/Grid.jsx	`0.00% <0.00%> (ø)`
...reset-chart-deckgl/src/layers/Grid/controlPanel.ts	`50.00% <0.00%> (-50.00%)`	⬇️
.../legacy-preset-chart-deckgl/src/layers/Hex/Hex.jsx	`0.00% <0.00%> (ø)`
...preset-chart-deckgl/src/layers/Hex/controlPanel.ts	`50.00% <0.00%> (-50.00%)`	⬇️
...et-chart-deckgl/src/layers/Polygon/controlPanel.ts	`33.33% <0.00%> (-16.67%)`	⬇️
... and 89 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 90d486a...b1d10bd. Read the comment docs.

zhaoyongjie · 2022-06-10T09:41:07Z

tests/unit_tests/conftest.py

-    app.config["SQLALCHEMY_DATABASE_URI"] = "sqlite://"
+    app.config["SQLALCHEMY_DATABASE_URI"] = (
+        os.environ.get("SUPERSET__SQLALCHEMY_DATABASE_URI") or "sqlite://"
+    )


Use integration test database instance for local debugging.

michael-s-molina · 2022-06-16T13:24:19Z

tests/unit_tests/utils/viz_migration/treemap_migration_test.py

+treemap_processor = get_migrate_class[MigrateVizEnum.treemap]
+
+
+def test_treemap_migrate(app_context: SupersetApp) -> None:


Maybe have separated tests for upgrade and downgrade?

the method of downgrade needs an updated slice so combine upgrade and downgrade in the same unit test case.

michael-s-molina · 2022-06-16T13:25:12Z

superset/utils/migrate_viz.py

+        return slc
+
+
+class MigrateTreeMap(MigrateViz):


Maybe move MigrateTreeMap to its own file? I guess we'll have a bunch of these.

This is a one-time script. There is no need to split moudles.

Won't we have other viz types to be migrated? If yes, are you going to add all the viz types to this file?

yep, all migrations are placed in this script.

Generally, scripts are waterfalls, while libary should be modular.

michael-s-molina · 2022-06-16T13:31:00Z

superset/utils/migrate_viz.py

+            if key in self.mapping_keys and self.mapping_keys[key] in rv_data:
+                raise ValueError("Duplicate key in target viz")
+
+            if key in self.mapping_keys:
+                rv_data[self.mapping_keys[key]] = value


I personally find this more readable, but it's not a blocker.

Suggested change

if key in self.mapping_keys and self.mapping_keys[key] in rv_data:

raise ValueError("Duplicate key in target viz")

if key in self.mapping_keys:

rv_data[self.mapping_keys[key]] = value

if key in self.mapping_keys:

mapped_key = self.mapping_keys[key]

if mapped_key in rv_data:

raise ValueError("Duplicate key in target viz")

rv_data[mapped_key] = value

Exception handling is easier to maintain if it can be put together.

michael-s-molina · 2022-07-06T12:47:01Z

@zhaoyongjie @rusackas Shouldn't we disable TreeMap v1 after the migration? By that, I mean no more TreeMap v1 on Add Slice and Chart Type filter in the charts list. Right now, the user can still create the old version which kind of defeats the whole purpose of the migration.

zhaoyongjie · 2022-07-06T13:56:42Z

@zhaoyongjie @rusackas Shouldn't we disable TreeMap v1 after the migration? By that, I mean no more TreeMap v1 on Add Slice and Chart Type filter in the charts list. Right now, the user can still create the old version which kind of defeats the whole purpose of the migration.

I intend to do a separated PR that removes the Legacy TreeMap after merged migration PR. the follow-up PR will 1) remove Legacy TreeMap from plugins, 2) remove legacy TreeMap from MainPresent.js.

michael-s-molina · 2022-07-06T14:00:46Z

@zhaoyongjie @rusackas Shouldn't we disable TreeMap v1 after the migration? By that, I mean no more TreeMap v1 on Add Slice and Chart Type filter in the charts list. Right now, the user can still create the old version which kind of defeats the whole purpose of the migration.

I intend to do a separated PR that removes the Legacy TreeMap after merged migration PR. the follow-up PR will 1) remove Legacy TreeMap from plugins, 2) remove legacy TreeMap from MainPresent.js.

May I suggest implementing this in the same PR to avoid the risk of someone executing the script and creating an old TreeMap before the follow-up PR is merged?

superset/migrations/versions/2022-06-30_22-04_c747c78868b6_migrating_legacy_treemap.py

superset/utils/migrate_viz.py

tests/unit_tests/utils/viz_migration/treemap_migration_test.py

zhaoyongjie · 2022-07-07T02:48:51Z

@john-bodley thanks for the reviewing, I have addressed comments.

zhaoyongjie requested a review from a team as a code owner June 10, 2022 07:46

pull-request-size bot added the size/L label Jun 10, 2022

kgabryje reviewed Jun 10, 2022

View reviewed changes

superset/utils/migrate_viz.py Outdated Show resolved Hide resolved

zhaoyongjie commented Jun 10, 2022

View reviewed changes

zhaoyongjie requested review from a team and rusackas June 10, 2022 12:15

zhaoyongjie force-pushed the feat/migrate_treemap branch 3 times, most recently from 7b4e0dc to 9e44fa0 Compare June 13, 2022 07:00

zhaoyongjie mentioned this pull request Jun 13, 2022

feat: Area viz migration #20359

Merged

9 tasks

zhaoyongjie closed this Jun 13, 2022

zhaoyongjie reopened this Jun 13, 2022

rusackas requested review from stephenLYZ and michael-s-molina June 15, 2022 20:22

michael-s-molina reviewed Jun 16, 2022

View reviewed changes

zhaoyongjie requested review from betodealmeida and a team June 21, 2022 07:19

zhaoyongjie added 2 commits June 30, 2022 10:32

feat: treemap migration

ef3bc6d

refine migration

0d10fb3

zhaoyongjie force-pushed the feat/migrate_treemap branch from b2daecf to 0d10fb3 Compare June 30, 2022 02:38

rusackas requested a review from john-bodley July 6, 2022 16:09

john-bodley reviewed Jul 6, 2022

View reviewed changes

addressing comments

b1d10bd

michael-s-molina approved these changes Jul 7, 2022

View reviewed changes

zhaoyongjie merged commit 7626c31 into apache:master Jul 7, 2022

ktmud mentioned this pull request Jul 19, 2022

fix(db): use paginated_update for viz migration #20761

Merged

9 tasks

akshatsri pushed a commit to charan1314/superset that referenced this pull request Jul 19, 2022

feat: TreeMap migration (apache#20346)

74da86c

sadpandajoe mentioned this pull request Jul 19, 2022

chore: update viz type to echarts_area and treemap_v2 preset-io/public-examples#11

Merged

ktmud mentioned this pull request Jul 19, 2022

fix(db): set slices.query_context to MEDIUMTEXT for mysql #20779

Merged

9 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: TreeMap migration #20346

feat: TreeMap migration #20346

zhaoyongjie commented Jun 10, 2022

zhaoyongjie Jun 10, 2022 •

edited

Loading

codecov bot commented Jun 10, 2022 •

edited

Loading

zhaoyongjie Jun 10, 2022

michael-s-molina Jun 16, 2022

zhaoyongjie Jun 16, 2022

michael-s-molina Jun 16, 2022

zhaoyongjie Jun 16, 2022

michael-s-molina Jun 16, 2022

zhaoyongjie Jun 16, 2022

zhaoyongjie Jun 16, 2022

michael-s-molina Jun 16, 2022

zhaoyongjie Jun 30, 2022

michael-s-molina commented Jul 6, 2022

zhaoyongjie commented Jul 6, 2022 •

edited

Loading

michael-s-molina commented Jul 6, 2022 •

edited

Loading

zhaoyongjie commented Jul 7, 2022

		treemap_processor = get_migrate_class[MigrateVizEnum.treemap]


		def test_treemap_migrate(app_context: SupersetApp) -> None:

feat: TreeMap migration #20346

feat: TreeMap migration #20346

Conversation

zhaoyongjie commented Jun 10, 2022

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

zhaoyongjie Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jun 10, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-s-molina commented Jul 6, 2022

zhaoyongjie commented Jul 6, 2022 • edited Loading

michael-s-molina commented Jul 6, 2022 • edited Loading

zhaoyongjie commented Jul 7, 2022

zhaoyongjie Jun 10, 2022 •

edited

Loading

codecov bot commented Jun 10, 2022 •

edited

Loading

zhaoyongjie commented Jul 6, 2022 •

edited

Loading

michael-s-molina commented Jul 6, 2022 •

edited

Loading