You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "john-bodley (via GitHub)" <gi...@apache.org> on 2023/03/03 00:24:01 UTC
[GitHub] [superset] john-bodley opened a new pull request, #23269: feature: Add filter-box/native filter migration script
john-bodley opened a new pull request, #23269:
URL: https://github.com/apache/superset/pull/23269
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
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
[GitHub] [superset] john-bodley commented on a diff in pull request #23269: feat(native-filters): Add legacy (filter-box) to native filter migration script
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #23269:
URL: https://github.com/apache/superset/pull/23269#discussion_r1128542764
##########
superset/utils/dashboard_filter_scopes_converter.py:
##########
@@ -88,3 +90,160 @@ 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-locals,too-many-nested-blocks
Review Comment:
This logic is taken from https://github.com/apache/superset/pull/16992 (specifically the [ilterboxMigrationHelper.ts](https://github.com/graceguo-supercat/superset/blob/3a27565fe65f4f166ea7ade3d1ee8580ef8f9c17/superset-frontend/src/dashboard/util/filterboxMigrationHelper.ts) file). Note however the logic here is considerably simpler, i.e., there's no:
1. Druid logic as the Druid NoSQL connector has been deprecated.
2. Complex dependency logic†.
† The reason being is previously the legacy filter-box charts remained in the dashboards and thus there needed to be additional logic to wire up the parent/child relationships between the native filters and legacy filter-box charts. In the current formulation the filter-box charts have been removed (having been replaced with markdown elements) and thus said logic isn't required.
--
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
[GitHub] [superset] codecov[bot] commented on pull request #23269: feat(native-filters): Add legacy (filter-box) to native filter migration script
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #23269:
URL: https://github.com/apache/superset/pull/23269#issuecomment-1461080698
# [Codecov](https://codecov.io/gh/apache/superset/pull/23269?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#23269](https://codecov.io/gh/apache/superset/pull/23269?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f3363fe) into [master](https://codecov.io/gh/apache/superset/commit/e0d976dcca1420162a73dd8e97ebe157af4b1545?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e0d976d) will **decrease** coverage by `11.45%`.
> The diff coverage is `1.38%`.
> :exclamation: Current head f3363fe differs from pull request most recent head 8668e65. Consider uploading reports for the commit 8668e65 to get more accurate results
```diff
@@ Coverage Diff @@
## master #23269 +/- ##
===========================================
- Coverage 67.53% 56.08% -11.45%
===========================================
Files 1909 1910 +1
Lines 73491 73702 +211
Branches 7988 7988
===========================================
- Hits 49631 41337 -8294
- Misses 21810 30315 +8505
Partials 2050 2050
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.40% <1.38%> (-0.31%)` | :arrow_down: |
| python | `58.54% <1.38%> (-23.75%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `52.18% <1.38%> (-0.31%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/23269?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/cli/main.py](https://codecov.io/gh/apache/superset/pull/23269?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY2xpL21haW4ucHk=) | `0.00% <0.00%> (ø)` | |
| [superset/cli/native\_filters.py](https://codecov.io/gh/apache/superset/pull/23269?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY2xpL25hdGl2ZV9maWx0ZXJzLnB5) | `0.00% <0.00%> (ø)` | |
| [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/23269?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <ø> (-76.25%)` | :arrow_down: |
| [superset/dashboards/dao.py](https://codecov.io/gh/apache/superset/pull/23269?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9kYW8ucHk=) | `30.71% <ø> (-65.72%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/superset/pull/23269?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `35.96% <0.00%> (-38.53%)` | :arrow_down: |
| [...uperset/utils/dashboard\_filter\_scopes\_converter.py](https://codecov.io/gh/apache/superset/pull/23269?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ZpbHRlcl9zY29wZXNfY29udmVydGVyLnB5) | `10.52% <3.63%> (-26.69%)` | :arrow_down: |
| [superset/dashboards/schemas.py](https://codecov.io/gh/apache/superset/pull/23269?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9zY2hlbWFzLnB5) | `90.72% <100.00%> (-8.77%)` | :arrow_down: |
| [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/23269?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/tags/core.py](https://codecov.io/gh/apache/superset/pull/23269?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdGFncy9jb3JlLnB5) | `4.54% <0.00%> (-95.46%)` | :arrow_down: |
| [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/23269?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-90.91%)` | :arrow_down: |
| ... and [298 more](https://codecov.io/gh/apache/superset/pull/23269?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [superset] john-bodley commented on a diff in pull request #23269: feat(native-filters): Add legacy (filter-box) to native filter migration script
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #23269:
URL: https://github.com/apache/superset/pull/23269#discussion_r1167320890
##########
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:
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.
--
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
[GitHub] [superset] michael-s-molina commented on pull request #23269: feat(native-filters): Add legacy (filter-box) to native filter migration script
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #23269:
URL: https://github.com/apache/superset/pull/23269#issuecomment-1502282718
@villebro I could use a second pair of eyes here if you have some time 👀 🙏🏼
--
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
[GitHub] [superset] john-bodley commented on a diff in pull request #23269: feat: Add filter-box/native filter migration script
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #23269:
URL: https://github.com/apache/superset/pull/23269#discussion_r1128519681
##########
superset/cli/main.py:
##########
@@ -50,9 +50,12 @@ def make_shell_context() -> Dict[str, Any]:
):
module = importlib.import_module(module_name)
for attribute in module.__dict__.values():
- if isinstance(attribute, click.core.Command):
+ if isinstance(attribute, (click.core.Command, click.core.Group)):
superset.add_command(attribute)
+ if isinstance(attribute, click.core.Group):
Review Comment:
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.
##########
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"
Review Comment:
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.
##########
superset/views/core.py:
##########
@@ -1271,7 +1271,11 @@ def copy_dash( # pylint: disable=no-self-use
# update chartId of layout entities
for value in data["positions"].values():
- if isinstance(value, dict) and value.get("meta", {}).get("chartId"):
+ if (
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
Review Comment:
See above.
##########
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")
Review Comment:
This is not required per like #135.
##########
superset/utils/dashboard_filter_scopes_converter.py:
##########
@@ -88,3 +90,160 @@ 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-locals,too-many-nested-blocks
Review Comment:
This logic is taken from https://github.com/apache/superset/pull/16992 (specifically the [ilterboxMigrationHelper.ts](https://github.com/graceguo-supercat/superset/blob/3a27565fe65f4f166ea7ade3d1ee8580ef8f9c17/superset-frontend/src/dashboard/util/filterboxMigrationHelper.ts) file). Note however the logic here is considerably simpler, i.e., there's no:
1. Druid logic as the Druid NoSQL connector has been deprecated.
2. Complex dependency logic†.
† The reason being is previously the legacy filter-box charts remained in the dashboards and thus there needed to be additional logic to wire up the parent/child relationships between the native filters and legacy filter-box charts. In the current formulation the filter-box charts have been removed (having been replaced with markdown elements) and thus said logic isn't required.
##########
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"
Review Comment:
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`.
##########
superset/utils/dashboard_filter_scopes_converter.py:
##########
@@ -19,13 +19,15 @@
from collections import defaultdict
from typing import Any, Dict, List
+from shortid import ShortId
+
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]
Review Comment:
Renaming adds clarity as to which type of charts/slices these are.
--
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
[GitHub] [superset] john-bodley merged pull request #23269: feat(native-filters): Add legacy (filter-box) to native filter migration script
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley merged PR #23269:
URL: https://github.com/apache/superset/pull/23269
--
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
[GitHub] [superset] john-bodley commented on a diff in pull request #23269: feat(native-filters): Add legacy (filter-box) to native filter migration script
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #23269:
URL: https://github.com/apache/superset/pull/23269#discussion_r1171934332
##########
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:
@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.
--
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
[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
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
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
[GitHub] [superset] john-bodley commented on a diff in pull request #23269: feat(native-filters): Add legacy (filter-box) to native filter migration script
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #23269:
URL: https://github.com/apache/superset/pull/23269#discussion_r1167322383
##########
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:
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.
--
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
[GitHub] [superset] john-bodley commented on a diff in pull request #23269: feat(native-filters): Add legacy (filter-box) to native filter migration script
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #23269:
URL: https://github.com/apache/superset/pull/23269#discussion_r1167321694
##########
requirements/base.txt:
##########
@@ -40,12 +40,15 @@ click==8.0.4
# apache-superset
# celery
# click-didyoumean
+ # click-option-group
Review Comment:
@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.
--
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
[GitHub] [superset] john-bodley commented on pull request #23269: feat(native-filters): Add legacy (filter-box) to native filter migration script
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #23269:
URL: https://github.com/apache/superset/pull/23269#issuecomment-1515499385
Thanks @michael-s-molina for the review. I've hopefully addressed your comments.
--
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