You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "michael-s-molina (via GitHub)" <gi...@apache.org> on 2023/04/10 20:30:07 UTC
[GitHub] [superset] michael-s-molina commented on a diff in pull request #23269: feat(native-filters): Add legacy (filter-box) to native filter migration script
michael-s-molina commented on code in PR #23269:
URL: https://github.com/apache/superset/pull/23269#discussion_r1162030979
##########
docs/docs/miscellaneous/native-filter-migration.mdx:
##########
@@ -0,0 +1,103 @@
+---
+title: Migrating from Legacy to Native Filters
+sidebar_position: 5
+version: 1
+---
+
+##
+
+The `superset native-filters` CLI command group—somewhat akin to an Alembic migration—
+comprises of a number of sub-commands which allows administrators to upgrade/downgrade
+existing dashboards which use the legacy filter-box charts—in combination with the
+filter scopes/filter mapping—to use the native filter dashboard component.
+
+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
Review Comment:
```suggestion
enabling the `DASHBOARD_NATIVE_FILTERS` it's strongly advised to run the migration ASAP to
```
##########
docs/docs/miscellaneous/native-filter-migration.mdx:
##########
@@ -0,0 +1,103 @@
+---
+title: Migrating from Legacy to Native Filters
+sidebar_position: 5
+version: 1
+---
+
+##
+
+The `superset native-filters` CLI command group—somewhat akin to an Alembic migration—
+comprises of a number of sub-commands which allows administrators to upgrade/downgrade
+existing dashboards which use the legacy filter-box charts—in combination with the
+filter scopes/filter mapping—to use the native filter dashboard component.
+
+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
+ensure users are not exposed to the hybrid state.
+
+### Upgrading
+
+The
+
+```
+superset native-filters upgrade
+```
+
+command—which provides the option to target either specific dashboard(s) or all
+dashboards—migrates the legacy filters to native filters.
+
+Specifically the command performs the following:
+
+- 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.
Review Comment:
```suggestion
providing context to help owners review/verify said change.
```
##########
docs/docs/miscellaneous/native-filter-migration.mdx:
##########
@@ -0,0 +1,103 @@
+---
+title: Migrating from Legacy to Native Filters
+sidebar_position: 5
+version: 1
+---
+
+##
+
+The `superset native-filters` CLI command group—somewhat akin to an Alembic migration—
+comprises of a number of sub-commands which allows administrators to upgrade/downgrade
+existing dashboards which use the legacy filter-box charts—in combination with the
+filter scopes/filter mapping—to use the native filter dashboard component.
+
+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
+ensure users are not exposed to the hybrid state.
+
+### Upgrading
+
+The
+
+```
+superset native-filters upgrade
+```
+
+command—which provides the option to target either specific dashboard(s) or all
+dashboards—migrates the legacy filters to native filters.
+
+Specifically the command performs the following:
+
+- 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.
+- Migrates the filter scopes/filter mappings to the native filter configuration.
+
+#### Quality Control
+
+Dashboard owners should:
+
+- Verify that the filter behavior is correct.
+- Consolidate any conflicting/redundant filters—this previously may not have been
+obvious given the embedded nature of the legacy filters and/or the non-optimal UX of the
+legacy filter mapping (scopes and immunity).
+- Rename the filters—which may not be uniquely named—to provide the necessary context
+which previously was likely provided by both the location of the filter-box and the
+corresponding filter-box title.
+
+Dashboard owners may:
+
+- Remove† the markdown elements from their dashboards and adjust the layout accordingly.
+
+† Note removing the markdown elements—which contain metadata relating to the replaced
+chart—prevents the dashboard from being fully restored and thus this operation should
+only be performed if it is evident that a downgrade is not necessary.
+
+### Downgrading
+
+Similarly the
+
+```
+superset native-filters downgrade
+```
+
+command reverses said migration, i.e., restores the dashboard to the previous state.
+
+
+### Cleanup
+
+The ability to downgrade/reverse the migration requires temporary storage of the
+dashboard metadata—relating to both positional composition and filter configuration.
+
+Once the upgrade has been verified it is recommended to run the
+
+```
+superset native-filters cleanup
+```
+
+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:
+
+- Removes the temporary dashboard metadata.
+- Deletes the filter-box charts†.
Review Comment:
```suggestion
- Deletes the filter-box charts associated with the dashboard†.
```
##########
docs/docs/miscellaneous/native-filter-migration.mdx:
##########
@@ -0,0 +1,103 @@
+---
+title: Migrating from Legacy to Native Filters
+sidebar_position: 5
+version: 1
+---
+
+##
+
+The `superset native-filters` CLI command group—somewhat akin to an Alembic migration—
+comprises of a number of sub-commands which allows administrators to upgrade/downgrade
+existing dashboards which use the legacy filter-box charts—in combination with the
+filter scopes/filter mapping—to use the native filter dashboard component.
+
+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
+ensure users are not exposed to the hybrid state.
+
+### Upgrading
+
+The
+
+```
+superset native-filters upgrade
+```
+
+command—which provides the option to target either specific dashboard(s) or all
+dashboards—migrates the legacy filters to native filters.
+
+Specifically the command performs the following:
+
+- 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.
+- Migrates the filter scopes/filter mappings to the native filter configuration.
+
+#### Quality Control
+
+Dashboard owners should:
+
+- Verify that the filter behavior is correct.
+- Consolidate any conflicting/redundant filters—this previously may not have been
+obvious given the embedded nature of the legacy filters and/or the non-optimal UX of the
+legacy filter mapping (scopes and immunity).
+- Rename the filters—which may not be uniquely named—to provide the necessary context
+which previously was likely provided by both the location of the filter-box and the
+corresponding filter-box title.
+
+Dashboard owners may:
+
+- Remove† the markdown elements from their dashboards and adjust the layout accordingly.
+
+† Note removing the markdown elements—which contain metadata relating to the replaced
+chart—prevents the dashboard from being fully restored and thus this operation should
+only be performed if it is evident that a downgrade is not necessary.
+
+### Downgrading
+
+Similarly the
+
+```
+superset native-filters downgrade
+```
+
+command reverses said migration, i.e., restores the dashboard to the previous state.
+
+
+### Cleanup
+
+The ability to downgrade/reverse the migration requires temporary storage of the
+dashboard metadata—relating to both positional composition and filter configuration.
+
+Once the upgrade has been verified it is recommended to run the
+
+```
+superset native-filters cleanup
+```
+
+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:
Review Comment:
```suggestion
Specifically, the command performs the following:
```
##########
superset/utils/dashboard_filter_scopes_converter.py:
##########
@@ -88,3 +90,249 @@ def copy_filter_scopes(
if int(slice_id) in old_to_new_slc_id_dict
]
return new_filter_scopes
+
+
+def convert_filter_scopes_to_native_filters( # pylint: disable=invalid-name,too-many-branches,too-many-locals,too-many-nested-blocks,too-many-statements
+ json_metadata: Dict[str, Any],
+ position_json: Dict[str, Any],
+ filter_boxes: List[Slice],
+) -> List[Dict[str, Any]]:
+ """
+ Convert the legacy filter scopes et al. to the native filter configuration.
+
+ Dashboard filter scopes are implicitly defined where an undefined scope implies
+ no immunity, i.e., they apply to all applicable charts. The `convert_filter_scopes`
+ method provides an explicit definition by extracting the underlying filter-box
+ configurations.
+
+ Hierarchical legacy filters are defined via non-exclusion of peer or children
+ filter-box charts whereas native hierarchical filters are defined via explicit
+ parental relationships, i.e., the inverse.
+
+ :param json_metata: The dashboard metadata
+ :param position_json: The dashboard layout
+ :param filter_boxes: The filter-box charts associated with the dashboard
+ :returns: The native filter configuration
+ :see: convert_filter_scopes
+ """
+
+ shortid = ShortId()
+ default_filters = json.loads(json_metadata.get("default_filters") or "{}")
+ filter_scopes = json_metadata.get("filter_scopes", {})
+ filter_box_ids = {filter_box.id for filter_box in filter_boxes}
+
+ filter_scope_by_key_and_field: Dict[str, Dict[str, Dict[str, Any]]] = defaultdict(
+ dict
+ )
+
+ filter_by_key_and_field: Dict[str, Dict[str, Dict[str, Any]]] = defaultdict(dict)
+
+ # Dense representation of filter scopes, falling back to chart level filter configs
+ # if the respective filter scope is not defined at the dashboard level.
+ for filter_box in filter_boxes:
+ key = str(filter_box.id)
+
+ filter_scope_by_key_and_field[key] = {
+ **(
+ convert_filter_scopes(
+ json_metadata,
+ filter_boxes=[filter_box],
+ ).get(filter_box.id, {})
+ ),
+ **(filter_scopes.get(key, {})),
+ }
+
+ # Contruct the native filters.
+ for filter_box in filter_boxes:
+ key = str(filter_box.id)
+ params = json.loads(filter_box.params or "{}")
+
+ for field, filter_scope in filter_scope_by_key_and_field[key].items():
+ default = default_filters.get(key, {}).get(field)
+
+ fltr: Dict[str, Any] = {
+ "cascadeParentIds": [],
+ "id": f"NATIVE_FILTER-{shortid.generate()}",
+ "scope": {
+ "rootPath": filter_scope["scope"],
+ "excluded": [
+ id_
+ for id_ in filter_scope["immune"]
+ if id_ not in filter_box_ids
+ ],
+ },
+ "type": "NATIVE_FILTER",
+ }
+
+ if field == "__time_col" and params.get("show_sqla_time_column"):
+ fltr.update(
+ {
+ "filterType": "filter_timecolumn",
+ "name": "Time Column",
+ "targets": [{"datasetId": filter_box.datasource_id}],
+ }
+ )
+
+ if not default:
+ default = params.get("granularity_sqla")
+
+ if default:
+ fltr["defaultDataMask"] = {
+ "extraFormData": {"granularity_sqla": default},
+ "filterState": {"value": [default]},
+ }
+ elif field == "__time_grain" and params.get("show_sqla_time_granularity"):
+ fltr.update(
+ {
+ "filterType": "filter_timegrain",
+ "name": "Time Grain",
+ "targets": [{"datasetId": filter_box.datasource_id}],
+ }
+ )
+
+ if not default:
+ default = params.get("time_grain_sqla")
+
+ if default:
+ fltr["defaultDataMask"] = {
+ "extraFormData": {"time_grain_sqla": default},
+ "filterState": {"value": [default]},
+ }
+ elif field == "__time_range" and params.get("date_filter"):
+ fltr.update(
+ {
+ "filterType": "filter_time",
+ "name": "Time Range",
+ "targets": [{}],
+ }
+ )
+
+ if not default:
+ default = params.get("time_range")
+
+ if default and default != "No filter":
+ fltr["defaultDataMask"] = {
+ "extraFormData": {"time_range": default},
+ "filterState": {"value": default},
+ }
+ else:
+ for config in params.get("filter_configs") or []:
+ if config["column"] == field:
+ fltr.update(
+ {
+ "controlValues": {
+ "defaultToFirstItem": False,
+ "enableEmptyFilter": not config.get(
+ "clearable",
+ True,
+ ),
+ "inverseSelection": False,
+ "multiSelect": config.get(
+ "multiple",
+ False,
+ ),
+ "searchAllOptions": config.get(
+ "searchAllOptions",
+ False,
+ ),
+ },
+ "filterType": "filter_select",
+ "name": config.get("label") or field,
+ "targets": [
+ {
+ "column": {"name": field},
+ "datasetId": filter_box.datasource_id,
+ },
+ ],
+ }
+ )
+
+ if "metric" in config:
+ fltr["sortMetric"] = config["metric"]
+ fltr["controlValues"]["sortAscending"] = config["asc"]
+
+ if params.get("adhoc_filters"):
+ fltr["adhoc_filters"] = params["adhoc_filters"]
+
+ # Pre-filter available values based on time range/column.
+ time_range = params.get("time_range")
+
+ if time_range and time_range != "No filter":
+ fltr.update(
+ {
+ "time_range": time_range,
+ "granularity_sqla": params.get("granularity_sqla"),
+ }
+ )
+
+ if not default:
+ default = config.get("defaultValue")
+
+ if default:
+ if config["multiple"]:
+ default = default.split(";")
+ else:
+ default = [default]
+
+ if default:
+ fltr["defaultDataMask"] = {
+ "extraFormData": {
+ "filters": [
+ {
+ "col": field,
+ "op": "IN",
+ "val": default,
+ }
+ ],
+ },
+ "filterState": {"value": default},
+ }
+
+ break
+
+ if "filterType" in fltr:
+ filter_by_key_and_field[key][field] = fltr
+
+ # Ancestors of filter-box charts.
Review Comment:
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?
##########
requirements/base.txt:
##########
@@ -40,12 +40,15 @@ click==8.0.4
# apache-superset
# celery
# click-didyoumean
+ # click-option-group
Review Comment:
@john-bodley Should we consider the CLI a development tool and move these dependencies to `requirements/development.txt`?
##########
superset/cli/native_filters.py:
##########
@@ -0,0 +1,398 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import json
+from copy import deepcopy
+from textwrap import dedent
+from typing import Set, Tuple
+
+import click
+from click_option_group import optgroup, RequiredMutuallyExclusiveOptionGroup
+from flask.cli import with_appcontext
+from sqlalchemy import Column, ForeignKey, Integer, String, Table, Text
+from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy.orm import relationship
+
+from superset import db, is_feature_enabled
+
+Base = declarative_base()
+
+
+dashboard_slices = Table(
+ "dashboard_slices",
+ Base.metadata,
+ Column("id", Integer, primary_key=True),
+ Column("dashboard_id", Integer, ForeignKey("dashboards.id")),
+ Column("slice_id", Integer, ForeignKey("slices.id")),
+)
+
+
+slice_user = Table(
+ "slice_user",
+ Base.metadata,
+ Column("id", Integer, primary_key=True),
+ Column("slice_id", Integer, ForeignKey("slices.id")),
+)
+
+
+class Dashboard(Base): # type: ignore # pylint: disable=too-few-public-methods
+ __tablename__ = "dashboards"
+
+ id = Column(Integer, primary_key=True)
+ json_metadata = Column(Text)
+ slices = relationship("Slice", secondary=dashboard_slices, backref="dashboards")
+ position_json = Column()
+
+ def __repr__(self) -> str:
+ return f"Dashboard<{self.id}>"
+
+
+class Slice(Base): # type: ignore # pylint: disable=too-few-public-methods
+ __tablename__ = "slices"
+
+ id = Column(Integer, primary_key=True)
+ datasource_id = Column(Integer)
+ params = Column(Text)
+ slice_name = Column(String(250))
+ viz_type = Column(String(250))
+
+ def __repr__(self) -> str:
+ return f"Slice<{self.id}>"
+
+
+@click.group()
+def native_filters() -> None:
+ """
+ Perform native filter operations.
+ """
+
+
+@native_filters.command()
+@with_appcontext
+@optgroup.group(
+ "Grouped options",
+ cls=RequiredMutuallyExclusiveOptionGroup,
+)
+@optgroup.option(
+ "--all",
+ "all_",
+ default=False,
+ help="Upgrade all dashboards",
+ is_flag=True,
+)
+@optgroup.option(
+ "--id",
+ "dashboard_ids",
+ help="Upgrade the specific dashboard. Can be supplied multiple times.",
+ multiple=True,
+ type=int,
+)
+def upgrade(
Review Comment:
Could you move each command into its own file? (`upgrade.py`, `downgrade.py`, `cleanup.py`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org