feat(native-filters): Add legacy (filter-box) to native filter migration script #23269

john-bodley · 2023-03-03T00:23:59Z

SUMMARY

This PR addresses recommendation #⁠6 in #14383.

Specifically it adds a new CLI group command—which behaves in the same vein as an Alembic migration—for allowing admins to upgrade/degrade dashboards with legacy filters (filter-box charts and filter scopes) to native filters. i.e.,

superset native-filters upgrade --all  // All dashboards
superset native-filters upgrade --id <id-1> --id <id-2>  // Specific dashboards

superset native-filters downgrade --all  // All dashboards
superset native-filters downgrade --id <id-1> --id <id-2>  // Specific dashboards

superset native-filters cleanup // Removes filter-box assets and interim metadata which enables downgrading

This document (included in the PR) outlines the specific logic of the script and what is expected from dashboard owners.

This logic is based on #16992 (specifically the filterboxMigrationHelper.ts file).

Though I'm relatively confident on the logic—I can't really grasp JavaScript—there are some differences between @graceguo-supercat's script and my logic which maybe @ktmud, @nytai, @rusackas, @villebro, et al. could review:

Default values are defined either at: i) the dashboard level, or ii) the chart level. In @graceguo-supercat's script the default value is either: i) the dashboard default, ii) the chart default, or iii) the chart default if no dashboard default is defined. This varies by filter type and I'm unsure whether this was an oversight/error‡ or merely replicating Superset's logic. In my implementation precedence is first given to the dashboard level defaults (assuming they're valid per the chart configs) before falling back to the filter-box level defaults if undefined. The one caveat is pre-conditional filters which (after testing in Superset) it seems like these using the chart temporal defaults, i.e., the dashboard defaults are ignored.
The value filter types support pre-filters which can contain a time-range and associated time column (see screenshot). Thought @graceguo-supercat never set the time column, which per the tooltip indicates it will fallback to the default time column (I assume of the dataset), I have explicitly set this (regardless if it is the default) as i) I believe it improves the UX from a readability standpoint, i.e., it is evident which column the time range corresponds to, and ii) there's no guarantee that that a dataset will have a default temporal column.

‡ Per here there's logic to fallback to the chart level time_grain_sqla if there's no dashboard level default, i.e.,

dashboardDefaultValues || time_grain_sqla

however the prior if (!isEmpty(dashboardDefaultValues)) { condition would mean that the || would never be evaluated. I sense this was an error with the previous implementation, i.e., the logic should have been,

const defaultValues = dashboardDefaultValues || time_grain_sqla;

if (!isEmpty(defaultValues)) {
   ...
}

The same is true for the granularity_sqla column however the time_range column and value columns seem correct. Note in my current implementation I always fallback to the chart level default if not defined at the dashboard level.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE

AFTER

TESTING INSTRUCTIONS

Tested somewhat extensively using the example datasets.

ADDITIONAL INFORMATION

Has associated issue: [SIP-64] Migrate filter_box to Dashboard Native Filter Component #14383
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

john-bodley · 2023-03-07T20:09:41Z

superset/cli/main.py

            superset.add_command(attribute)

+            if isinstance(attribute, click.core.Group):


We need to ensure that group commands aren't added as a top-level command. Regrettably there's no way of inspecting a (sub-)command to see if it's part of a group, but given how groups are defined, i.e., prior to the sub-commands it seems valid to simply short circuit.

I sense the only other option is to remove the magic auto-detection logic and explicitly register all the commands.

john-bodley · 2023-03-07T20:12:03Z

superset/dashboards/commands/importers/v0.py

@@ -131,7 +131,7 @@ def alter_positions(
        for value in position_json:
            if (
                isinstance(value, dict)
-                and value.get("meta")


This is not required per like #135.

john-bodley · 2023-03-07T20:14:44Z

superset/dashboards/commands/importers/v0.py

@@ -131,7 +131,7 @@ def alter_positions(
        for value in position_json:
            if (
                isinstance(value, dict)
-                and value.get("meta")
+                and value.get("type") == "CHART"


This is needed given how as part of the migration we're temporarily storing the old chartId in the metadata block for the new markdown elements (which serve as a replacement for the filter-box charts) as a way to revert.

Granted this is bastardizing the metadata somewhat, but it does seem prudent to also ensure that the type is CHART prior to fetching the chartId.

john-bodley · 2023-03-07T20:16:00Z

superset/dashboards/dao.py

@@ -200,7 +200,7 @@ def set_dash_metadata(  # pylint: disable=too-many-locals
            slice_ids = [
                value.get("meta", {}).get("chartId")
                for value in positions.values()
-                if isinstance(value, dict)
+                if isinstance(value, dict) and value.get("type") == "CHART"


See above. Without this logic, when editing a dashboard all the old filter-box charts would be re-added given that the markdown elements still contain the chartId field.

john-bodley · 2023-03-07T20:16:35Z

superset/utils/dashboard_filter_scopes_converter.py

 from superset.models.slice import Slice

 logger = logging.getLogger(__name__)


 def convert_filter_scopes(
-    json_metadata: Dict[Any, Any], filters: List[Slice]
+    json_metadata: Dict[Any, Any], filter_boxes: List[Slice]


Renaming adds clarity as to which type of charts/slices these are.

john-bodley · 2023-03-07T20:24:41Z

superset/views/core.py

-                if isinstance(value, dict) and value.get("meta", {}).get("chartId"):
+                if (
+                    isinstance(value, dict)
+                    and value.get("type") == "CHART"


codecov · 2023-03-09T00:30:29Z

Codecov Report

Merging #23269 (687642d) into master (5bec1a6) will decrease coverage by 0.21%.
The diff coverage is 1.28%.

❗ Current head 687642d differs from pull request most recent head a88e67a. Consider uploading reports for the commit a88e67a to get more accurate results

@@            Coverage Diff             @@
##           master   #23269      +/-   ##
==========================================
- Coverage   68.08%   67.88%   -0.21%     
==========================================
  Files        1923     1924       +1     
  Lines       74135    74364     +229     
  Branches     8103     8103              
==========================================
+ Hits        50478    50481       +3     
- Misses      21580    21806     +226     
  Partials     2077     2077

Flag	Coverage Δ
hive	`52.92% <1.28%> (-0.34%)`	⬇️
mysql	`78.74% <1.28%> (-0.50%)`	⬇️
postgres	`78.82% <1.28%> (-0.50%)`	⬇️
presto	`52.84% <1.28%> (-0.34%)`	⬇️
python	`82.65% <1.28%> (-0.53%)`	⬇️
sqlite	`77.32% <1.28%> (-0.49%)`	⬇️
unit	`52.72% <1.28%> (-0.34%)`	⬇️

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

Impacted Files	Coverage Δ
superset/cli/main.py	`0.00% <0.00%> (ø)`
superset/cli/native_filters.py	`0.00% <0.00%> (ø)`
...uperset/utils/dashboard_filter_scopes_converter.py	`15.78% <2.70%> (-21.42%)`	⬇️
superset/dashboards/schemas.py	`99.01% <100.00%> (+<0.01%)`	⬆️

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

michael-s-molina

Thank you for the PR @john-bodley. I left some first-pass comments.

michael-s-molina · 2023-04-10T19:48:35Z

docs/docs/miscellaneous/native-filter-migration.mdx

+
+Even though both legacy and native filters can coexist the overall user experience (UX)
+is substandard as the already convoluted filter space becomes overly complex. After
+enabling the `DASHBOARD_NATIVE_FILTERS` it strongly advised to run the migration ASAP to


Suggested change

enabling the `DASHBOARD_NATIVE_FILTERS` it strongly advised to run the migration ASAP to

enabling the `DASHBOARD_NATIVE_FILTERS` it's strongly advised to run the migration ASAP to

michael-s-molina · 2023-04-10T19:50:48Z

docs/docs/miscellaneous/native-filter-migration.mdx

+
+- Replaces every filter-box chart within the dashboard with a markdown element which
+provides a link to the deprecated chart. This preserves the layout whilst simultaneously
+providing context to aid with owners review/verify said change.


Suggested change

providing context to aid with owners review/verify said change.

providing context to help owners review/verify said change.

michael-s-molina · 2023-04-10T19:57:27Z

docs/docs/miscellaneous/native-filter-migration.mdx

+command—which provides the option to target either specific dashboard(s) or all
+dashboards. Note this operation is irreversible.
+
+Specifically the command performs the following:


Suggested change

Specifically the command performs the following:

Specifically, the command performs the following:

michael-s-molina · 2023-04-10T20:00:14Z

docs/docs/miscellaneous/native-filter-migration.mdx

+Specifically the command performs the following:
+
+- Removes the temporary dashboard metadata.
+- Deletes the filter-box charts†.


Suggested change

- Deletes the filter-box charts†.

- Deletes the filter-box charts associated with the dashboard†.

Previously I had logic which actually deleted all filter-box charts if the --all option was included (irrespective of whether they were part of a dashboard) but later changed said logic.

michael-s-molina · 2023-04-10T20:05:59Z

requirements/base.txt

@@ -40,12 +40,15 @@ click==8.0.4
    #   apache-superset
    #   celery
    #   click-didyoumean
+    #   click-option-group


@john-bodley Should we consider the CLI a development tool and move these dependencies to requirements/development.txt?

@michael-s-molina I think the challenge with that is the superset CLI will be borked even if you ran superset run etc. Also as you can see there are other CLI dependencies (click-didyoumean) which have been included in the base requirements.

michael-s-molina · 2023-04-10T20:19:05Z

superset/cli/native_filters.py

+    multiple=True,
+    type=int,
+)
+def upgrade(


Could you move each command into its own file? (upgrade.py, downgrade.py, cleanup.py)

These commands aren't significantly large (in terms of lines of code) combined with the fact that they're somewhat related, thus I think they can reside in the same file. This is somewhat akin to Alembic migrations where the upgrade and downgrade functions coexist.

michael-s-molina · 2023-04-10T20:28:32Z

superset/utils/dashboard_filter_scopes_converter.py

+            if "filterType" in fltr:
+                filter_by_key_and_field[key][field] = fltr
+
+    # Ancestors of filter-box charts.


Maybe you could extract some sub-functions to improve readability? I was thinking one function for assembling the filters and other for establishing the hierarchy. WDYT?

@michael-s-molina I generally agree that long functions aren't great and though I took a pass at extracting this into sub functions the refactor wasn't great, i.e., assembling the filters really contains the hierarchical wiring.

I also previous refactored fetching the defaults into a function, but there needed to be lots of special case handling due to where/when the defaults were used, i.e., a filter_select filter may require the time range/granularity but these defaults are at the chart rather than dashboard level.

The TL;DR is granted the formulation isn't great I sense it's acceptable, especially in the context of a migration.

michael-s-molina · 2023-04-10T20:31:01Z

@villebro I could use a second pair of eyes here if you have some time 👀 🙏🏼

john-bodley · 2023-04-19T23:17:13Z

Thanks @michael-s-molina for the review. I've hopefully addressed your comments.

michael-s-molina

LGTM

…ion script (apache#23269) (cherry picked from commit d0fda60)

…ion script (apache#23269)

pull-request-size bot added the size/XL label Mar 3, 2023

john-bodley force-pushed the john-bodley--filter-migration branch 4 times, most recently from 9b605a2 to 74bb8af Compare March 7, 2023 18:07

pull-request-size bot added size/L and removed size/XL labels Mar 7, 2023

john-bodley changed the title ~~feature: Add filter-box/native filter migration script~~ feat: Add filter-box/native filter migration script Mar 7, 2023

john-bodley commented Mar 7, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--filter-migration branch from 74bb8af to d0fadf2 Compare March 7, 2023 20:25

pull-request-size bot added size/XL and removed size/L labels Mar 7, 2023

john-bodley force-pushed the john-bodley--filter-migration branch from d0fadf2 to 74e9e88 Compare March 7, 2023 20:30

john-bodley changed the title ~~feat: Add filter-box/native filter migration script~~ feat(native-filters): Add legacy (filter-box) to native filter migration script Mar 8, 2023

john-bodley force-pushed the john-bodley--filter-migration branch from 74e9e88 to 8668e65 Compare March 8, 2023 23:46

john-bodley marked this pull request as ready for review March 9, 2023 00:23

john-bodley force-pushed the john-bodley--filter-migration branch 11 times, most recently from e5b2827 to 199cf9b Compare March 15, 2023 02:24

john-bodley force-pushed the john-bodley--filter-migration branch 2 times, most recently from 7e12073 to 6d7d740 Compare March 20, 2023 20:38

john-bodley force-pushed the john-bodley--filter-migration branch from 6d7d740 to 52da5f0 Compare March 21, 2023 17:12

john-bodley force-pushed the john-bodley--filter-migration branch from 52da5f0 to 8c140da Compare March 31, 2023 19:02

john-bodley requested review from michael-s-molina, villebro, kgabryje and rusackas April 4, 2023 18:46

john-bodley force-pushed the john-bodley--filter-migration branch 2 times, most recently from d7365e4 to 7ae7089 Compare April 4, 2023 19:17

michael-s-molina reviewed Apr 10, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--filter-migration branch 2 times, most recently from a080803 to 2fdfe98 Compare April 19, 2023 23:16

michael-s-molina approved these changes Apr 20, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--filter-migration branch from 2fdfe98 to 0535b28 Compare April 20, 2023 21:33

feature: Add filter-box/native filter migration script

a88e67a

john-bodley force-pushed the john-bodley--filter-migration branch from 0535b28 to a88e67a Compare April 21, 2023 02:06

john-bodley merged commit d0fda60 into apache:master Apr 21, 2023

john-bodley added a commit to airbnb/superset-fork that referenced this pull request Apr 21, 2023

feat(native-filters): Add legacy (filter-box) to native filter migrat…

fda2d5d

…ion script (apache#23269) (cherry picked from commit d0fda60)

arkid15r mentioned this pull request Apr 21, 2023

fix(utils): evaluate date parser multiple holiday results correctly #23685

Merged

9 tasks

john-bodley mentioned this pull request Apr 21, 2023

fix(native-filters): Fix encoding of non-temporal default values #23779

Merged

9 tasks

john-bodley added a commit to airbnb/superset-fork that referenced this pull request Apr 26, 2023

feat(native-filters): Add legacy (filter-box) to native filter migrat…

0b22bfb

…ion script (apache#23269) (cherry picked from commit d0fda60)

sebastianliebscher pushed a commit to sebastianliebscher/superset that referenced this pull request Apr 28, 2023

feat(native-filters): Add legacy (filter-box) to native filter migrat…

48eb8fc

…ion script (apache#23269)

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(native-filters): Add legacy (filter-box) to native filter migration script #23269

feat(native-filters): Add legacy (filter-box) to native filter migration script #23269

john-bodley commented Mar 3, 2023 •

edited

Loading

john-bodley Mar 7, 2023

john-bodley Mar 7, 2023

john-bodley Mar 7, 2023

john-bodley Mar 7, 2023

john-bodley Mar 7, 2023

john-bodley Mar 7, 2023

codecov bot commented Mar 9, 2023 •

edited

Loading

michael-s-molina left a comment

michael-s-molina Apr 10, 2023

michael-s-molina Apr 10, 2023

michael-s-molina Apr 10, 2023

michael-s-molina Apr 10, 2023

john-bodley Apr 14, 2023

michael-s-molina Apr 10, 2023

john-bodley Apr 14, 2023

michael-s-molina Apr 10, 2023

john-bodley Apr 14, 2023

michael-s-molina Apr 10, 2023

john-bodley Apr 19, 2023

michael-s-molina commented Apr 10, 2023

john-bodley commented Apr 19, 2023

michael-s-molina left a comment

		superset.add_command(attribute)

		if isinstance(attribute, click.core.Group):

	enabling the `DASHBOARD_NATIVE_FILTERS` it strongly advised to run the migration ASAP to
	enabling the `DASHBOARD_NATIVE_FILTERS` it's strongly advised to run the migration ASAP to

	providing context to aid with owners review/verify said change.
	providing context to help owners review/verify said change.

	Specifically the command performs the following:
	Specifically, the command performs the following:

	- Deletes the filter-box charts†.
	- Deletes the filter-box charts associated with the dashboard†.

feat(native-filters): Add legacy (filter-box) to native filter migration script #23269

feat(native-filters): Add legacy (filter-box) to native filter migration script #23269

Conversation

john-bodley commented Mar 3, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE

AFTER

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

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

codecov bot commented Mar 9, 2023 • edited Loading

Codecov Report

michael-s-molina 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

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 Apr 10, 2023

john-bodley commented Apr 19, 2023

michael-s-molina left a comment

Choose a reason for hiding this comment

john-bodley commented Mar 3, 2023 •

edited

Loading

codecov bot commented Mar 9, 2023 •

edited

Loading