feat: Add Area chart migration and tweaks the Timeseries chart migration #25952

michael-s-molina · 2023-11-10T17:33:53Z

SUMMARY

This PR adds the Area chart migration logic (NVD3 ➡️ ECharts) and tweaks the base class used by timeseries migrations. Users can execute this migration using the #25304 CLI command and disable the legacy version with the VIZ_TYPE_DENYLIST configuration.

@sadpandajoe @jinghua-qa

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

1 - Upgrade a Area chart using the CLI command
2 - Check the new chart
3 - Downgrade a Area chart using the CLI command
4 - Check the legacy chart

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 · 2023-11-13T16:58:03Z

pytest.ini

@@ -17,4 +17,4 @@
 [pytest]
 testpaths =
    tests
-python_files = *_test.py test_*.py *_tests.py
+python_files = *_test.py test_*.py *_tests.py *viz/utils.py


Makes pytest display full assertion errors defined in that file.

john-bodley

Given there's also a refactor in combination with the are chart migration it might be helpful to annotate (via comments) which parts represent new logic.

I'm a tad complexed, given that the PR title states we're adding an area chart migration yet there was an existing "MigrateAreaChart" class.

john-bodley · 2023-11-15T21:39:00Z

tests/unit_tests/migrations/viz/dual_line_to_mixed_chart_test.py

@@ -68,30 +69,4 @@ def test_migration() -> None:


 def upgrade_downgrade(source, target) -> None:


This is legacy, but this method name is somewhat confusing, i.e., I'm not entirely sure what it does.

Removed 👍🏼

john-bodley · 2023-11-15T21:39:53Z

tests/unit_tests/migrations/viz/dual_line_to_mixed_chart_test.py

-    slc = MigrateDualLine.downgrade_slice(slc)
-    assert slc.viz_type == MigrateDualLine.source_viz_type
-    assert json.loads(slc.params) == source
+    migrate_and_assert(MigrateDualLine, source, target)


Thanks for the refactor.

michael-s-molina · 2023-11-16T13:19:51Z

superset/migrations/shared/migrate_viz/base.py

-            lambda current, total: print(
-                f"  Updating {current}/{total} charts", end="\r"
-            ),
+            lambda current, total: print(f"Upgraded {current}/{total} charts"),


Display a log entry for each block creating a log history.

michael-s-molina · 2023-11-16T13:22:23Z

I'm a tad complexed, given that the PR title states we're adding an area chart migration yet there was an existing "MigrateAreaChart" class.

Even though we had a previous migration, it was incomplete and wasn't being used.

michael-s-molina · 2023-11-16T13:24:00Z

superset/migrations/shared/migrate_viz/processors.py

@@ -36,40 +36,6 @@ def _pre_action(self) -> None:
            self.data["metric"] = self.data["metrics"][0]


-class MigrateAreaChart(MigrateViz):


Removed the incomplete and unused migration.

michael-s-molina · 2023-11-16T13:28:45Z

superset/migrations/shared/migrate_viz/processors.py

@@ -137,10 +103,22 @@ class MigrateSunburst(MigrateViz):

 class TimeseriesChart(MigrateViz):
    has_x_axis_control = True
+    rename_keys = {


This is not new. Just moved from MigrateLineChart.

michael-s-molina · 2023-11-16T13:29:04Z

superset/migrations/shared/migrate_viz/processors.py


 class MigrateLineChart(TimeseriesChart):
    source_viz_type = "line"
    target_viz_type = "echarts_timeseries_line"
-    rename_keys = {


Moved to TimeseriesChart.

michael-s-molina · 2023-11-16T13:31:13Z

tests/unit_tests/migrations/viz/pivot_table_v1_v2_test.py

    "any_other_key": "untouched",
    "columns": ["state"],
    "combine_metric": True,
-    "granularity_sqla": "ds",


Moved the time parts to a separate test file called time_related_fields_test.py

michael-s-molina · 2023-11-16T13:31:53Z

tests/unit_tests/migrations/viz/time_related_fields_test.py

+
+
+@with_feature_flags(GENERIC_CHART_AXES=False)
+def test_migration_without_generic_chart_axes() -> None:


All this came from the pivot_table_v1_v2_test.py

michael-s-molina · 2023-11-16T14:05:36Z

Given there's also a refactor in combination with the are chart migration it might be helpful to annotate (via comments) which parts represent new logic.

@john-bodley Added comments

michael-s-molina · 2023-11-16T14:07:10Z

superset/charts/commands/importers/v1/utils.py

@@ -75,7 +75,6 @@ def migrate_chart(config: dict[str, Any]) -> dict[str, Any]:
        if isclass(class_)
        and issubclass(class_, MigrateViz)
        and hasattr(class_, "source_viz_type")
-        and class_ != processors.MigrateAreaChart  # incomplete


Added the migration back to import function now that it's complete.

john-bodley · 2023-11-16T16:59:59Z

superset/migrations/shared/migrate_viz/processors.py


    def _pre_action(self) -> None:
        self.data["contributionMode"] = "row" if self.data.get("contribution") else None
-        self.data["zoomable"] = self.data.get("show_brush") != "no"
+        self.data["zoomable"] = self.data.get("show_brush") == "yes"


@michael-s-molina I see this is an update to the existing TimeseriesChart migration where the zoomable used to be True if show_brush was None whereas now it's False.

I'm not privy to knowing whether this logic (and that on lines 144–146) is correct, but maybe it make more sense to have said logic in a separate PR. Additionally is there anyway to proactively fix previously migrated charts which were wrong? I gather the x_ticks_layout field could be remedied (as it's persisted), but the show_brush was removed.

@john-bodley These PRs only add the migration logic but there's no migration invoking them directly. We'll have that in 4.0 when we forcefully remove the legacy versions. Right now, these migrations can only be invoked by the viz_migrations command which was not released yet. The command will be available in 3.1.

@michael-s-molina could we change to PR description to be something of the form,

Add Area chart migration and tweaks the Timeseries chart migration

which will aid developers if they're trying to bisect when/where a possible change occurred.

john-bodley · 2023-11-16T17:04:20Z

superset/migrations/shared/migrate_viz/processors.py

+
+        self.remove_keys.add("stacked_style")
+
+        self.data["stack"] = self.stacked_map.get(


Should the stack key be None or undefined if the resulting key isn't in self.stacked_map?

Yes. It can be None or undefined as it's optional.

Do we have a preference or whether we explicitly define None? I guess there's likely so many inconsistencies on both the front- and back-end with regards to when/where to persist values that even if we make it undefined (and thus minimizing the payload) there's no guarantee that the field wont materialize with a value of None on save.

Exactly 😆

feat: Adds Area chart migration logic

5f59515

pull-request-size bot added the size/M label Nov 10, 2023

michael-s-molina mentioned this pull request Nov 10, 2023

feat: Migrates Area chart #23870

Closed

9 tasks

john-bodley added the review:checkpoint Last PR reviewed during the daily review standup label Nov 10, 2023

pull-request-size bot added size/XL and removed size/M labels Nov 13, 2023

michael-s-molina commented Nov 13, 2023

View reviewed changes

Adds tests

2f59321

michael-s-molina force-pushed the area-migration-logic branch from b6f8bc5 to 2f59321 Compare November 13, 2023 17:14

john-bodley removed the review:checkpoint Last PR reviewed during the daily review standup label Nov 13, 2023

Improves logging

bdafc01

michael-s-molina marked this pull request as ready for review November 15, 2023 13:22

michael-s-molina requested a review from a team as a code owner November 15, 2023 13:22

michael-s-molina requested review from john-bodley, jinghua-qa, justinpark and kgabryje November 15, 2023 13:23

john-bodley reviewed Nov 15, 2023

View reviewed changes

Removes weird method

60997e2

michael-s-molina commented Nov 16, 2023

View reviewed changes

Added the migration when importing

2675bfd

michael-s-molina commented Nov 16, 2023

View reviewed changes

Fixes import test

e5bc541

michael-s-molina commented Nov 16, 2023

View reviewed changes

john-bodley reviewed Nov 16, 2023

View reviewed changes

john-bodley approved these changes Nov 16, 2023

View reviewed changes

michael-s-molina merged commit 97d89d7 into apache:master Nov 16, 2023
31 checks passed

john-bodley changed the title ~~feat: Adds Area chart migration logic~~ feat: Add Area chart migration and tweaks the Timeseries chart migration Nov 16, 2023

josedev-union pushed a commit to Ortege-xyz/studio that referenced this pull request Jan 22, 2024

feat: Adds Area chart migration logic (apache#25952)

b6f464e

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024

feat: Adds Area chart migration logic (apache#25952)

0ec6add

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

sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024

feat: Adds Area chart migration logic (apache#25952)

cb85362

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

feat: Adds Area chart migration logic (apache#25952)

6a0f407

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Area chart migration and tweaks the Timeseries chart migration #25952

feat: Add Area chart migration and tweaks the Timeseries chart migration #25952

michael-s-molina commented Nov 10, 2023 •

edited

Loading

michael-s-molina Nov 13, 2023

john-bodley left a comment

john-bodley Nov 15, 2023

michael-s-molina Nov 16, 2023

john-bodley Nov 15, 2023

michael-s-molina Nov 16, 2023

michael-s-molina commented Nov 16, 2023

michael-s-molina Nov 16, 2023

michael-s-molina Nov 16, 2023 •

edited

Loading

michael-s-molina Nov 16, 2023 •

edited

Loading

michael-s-molina Nov 16, 2023

michael-s-molina Nov 16, 2023

michael-s-molina commented Nov 16, 2023 •

edited

Loading

michael-s-molina Nov 16, 2023

john-bodley Nov 16, 2023

michael-s-molina Nov 16, 2023

john-bodley Nov 16, 2023 •

edited

Loading

john-bodley Nov 16, 2023

michael-s-molina Nov 16, 2023 •

edited

Loading

john-bodley Nov 16, 2023

michael-s-molina Nov 16, 2023

		@@ -68,30 +69,4 @@ def test_migration() -> None:


		def upgrade_downgrade(source, target) -> None:

		@@ -36,40 +36,6 @@ def _pre_action(self) -> None:
		self.data["metric"] = self.data["metrics"][0]


		class MigrateAreaChart(MigrateViz):



		@with_feature_flags(GENERIC_CHART_AXES=False)
		def test_migration_without_generic_chart_axes() -> None:


		self.remove_keys.add("stacked_style")

		self.data["stack"] = self.stacked_map.get(

feat: Add Area chart migration and tweaks the Timeseries chart migration #25952

feat: Add Area chart migration and tweaks the Timeseries chart migration #25952

Conversation

michael-s-molina commented Nov 10, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

john-bodley left a 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 Nov 16, 2023

Choose a reason for hiding this comment

michael-s-molina Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

michael-s-molina Nov 16, 2023 • edited Loading

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 Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-s-molina Nov 16, 2023 • edited Loading

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 Nov 10, 2023 •

edited

Loading

michael-s-molina Nov 16, 2023 •

edited

Loading

michael-s-molina Nov 16, 2023 •

edited

Loading

michael-s-molina commented Nov 16, 2023 •

edited

Loading

john-bodley Nov 16, 2023 •

edited

Loading

michael-s-molina Nov 16, 2023 •

edited

Loading