feat(dashboard): Native filters - add type to native filter configuration #16549

m-ajay · 2021-09-01T16:35:50Z

SUMMARY

In a future PR, we will add a "Section" component to the native filters. We will need a new key to distinguish between them.

TESTING INSTRUCTIONS

Testing existing filters

Run migration using superset db upgrade
Verify in the dashboards DB table that all the filters have a type (Check json_metadata column in dashboards table)
Testing new filters
Add a new filter
Make sure that the type key appears in the new filter (Check json_metadata column in dashboards table)

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

codecov · 2021-09-01T17:02:31Z

Codecov Report

Merging #16549 (cea8a7d) into master (c5a5cf7) will decrease coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #16549      +/-   ##
==========================================
- Coverage   76.71%   76.55%   -0.16%     
==========================================
  Files        1002     1004       +2     
  Lines       53784    53964     +180     
  Branches     6859     7332     +473     
==========================================
+ Hits        41259    41312      +53     
- Misses      12288    12412     +124     
- Partials      237      240       +3

Flag	Coverage Δ
hive	`?`
javascript	`71.08% <100.00%> (+0.08%)`	⬆️
mysql	`81.54% <ø> (+<0.01%)`	⬆️
postgres	`81.60% <ø> (+0.04%)`	⬆️
presto	`?`
python	`81.69% <ø> (-0.38%)`	⬇️
sqlite	`81.25% <ø> (+0.08%)`	⬆️

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

Impacted Files	Coverage Δ
...mponents/nativeFilters/FiltersConfigModal/utils.ts	`72.22% <ø> (-1.39%)`	⬇️
...nd/src/dashboard/components/nativeFilters/utils.ts	`56.25% <ø> (ø)`
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx	`73.20% <100.00%> (+0.07%)`	⬆️
...nd/src/dashboard/components/nativeFilters/types.ts	`100.00% <100.00%> (ø)`
superset/db_engines/hive.py	`0.00% <0.00%> (-82.15%)`	⬇️
superset/db_engine_specs/hive.py	`69.80% <0.00%> (-16.87%)`	⬇️
superset/db_engine_specs/presto.py	`83.47% <0.00%> (-6.91%)`	⬇️
superset/views/database/mixins.py	`81.03% <0.00%> (-1.73%)`	⬇️
superset/connectors/sqla/models.py	`88.04% <0.00%> (-1.66%)`	⬇️
... and 34 more

Continue to review full report at Codecov.

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

graceguo-supercat · 2021-09-02T16:14:05Z

Hi @m-ajay this PR is good. After this PR is merged and migration is executed, all exist native filter components should have a type property, but how about new filter component in the new dashboard, which will be created in the near future?
Besides migration, should we also add default type in the code at the same time?

m-ajay · 2021-09-02T18:43:27Z

Hi @m-ajay this PR is good. After this PR is merged and migration is executed, all exist native filter components should have a type property, but how about new filter component in the new dashboard, which will be created in the near future?
Besides migration, should we also add default type in the code at the same time?

@Grace Thanks for the review.
I'm adding type as NATIVE_FILTER when creating a new filter. In the future, when we add a section, we will add type as SECTION

john-bodley · 2021-09-02T23:51:51Z

superset/migrations/versions/021b81fe4fbb_add_type_to_native_filter_configuration.py

+            )
+            continue
+
+        json_meta = json.loads(dashboard.json_metadata)


You might want to wrap this in a try/except since some charts could store invalid JSON. You can see how other migrations handle this.

john-bodley · 2021-09-02T23:56:09Z

superset/migrations/versions/021b81fe4fbb_add_type_to_native_filter_configuration.py

+        json_meta = json.loads(dashboard.json_metadata)
+
+        for native_filter in json_meta.get("native_filter_configuration", []):
+            native_filter.setdefault("type", "NATIVE_FILTER")


Why not just native_filter["type"] = "NATIVE_FILTER" if type is a new key (which per line 90 it would indicate that it is)? I always forget how dict.setdefault works as it's rarely used.

john-bodley · 2021-09-03T00:00:11Z

superset/migrations/versions/021b81fe4fbb_add_type_to_native_filter_configuration.py

+
+        for native_filter in json_meta.get("native_filter_configuration", []):
+            native_filter.setdefault("type", "NATIVE_FILTER")
+        dashboard.json_metadata = json.dumps(json_meta)


For efficiency we likely should only be updating dashboard.json_metadata for those dashboards which were modified, so maybe something of the form,

if "native_filter_configuration" in json_meta: for native_filter in json_meta["native_filter_configuration"]: native_filter["type"] = "NATIVE_FILTER" dashboard.json_metadata = json.dumps(json_meta)

etr2460

this lgtm, i love the verbose logging in the db migration too, it makes it easy to understand exactly what's happening

…tion (apache#16549) * iSort fixes * Add type key to the new filters * Fix wrong attribute * PR comments * PR comments * Fix failing tests

m-ajay added 2 commits August 31, 2021 17:40

iSort fixes

016e270

Add type key to the new filters

8827468

m-ajay requested a review from a team as a code owner September 1, 2021 16:35

pull-request-size bot added the size/M label Sep 1, 2021

Fix wrong attribute

0e91825

john-bodley reviewed Sep 3, 2021

View reviewed changes

PR comments

91b9061

pull-request-size bot added size/L and removed size/M labels Sep 3, 2021

PR comments

45b3c5f

m-ajay requested a review from john-bodley September 7, 2021 17:12

Fix failing tests

cea8a7d

etr2460 approved these changes Sep 7, 2021

View reviewed changes

etr2460 merged commit 4b70d46 into apache:master Sep 9, 2021

m-ajay deleted the feat/migration-add-type-to-native-filter branch September 9, 2021 20:50

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dashboard): Native filters - add type to native filter configuration #16549

feat(dashboard): Native filters - add type to native filter configuration #16549

m-ajay commented Sep 1, 2021

codecov bot commented Sep 1, 2021 •

edited

graceguo-supercat commented Sep 2, 2021 •

edited

m-ajay commented Sep 2, 2021

john-bodley Sep 2, 2021

john-bodley Sep 2, 2021

john-bodley Sep 3, 2021

etr2460 left a comment

feat(dashboard): Native filters - add type to native filter configuration #16549

feat(dashboard): Native filters - add type to native filter configuration #16549

Conversation

m-ajay commented Sep 1, 2021

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Sep 1, 2021 • edited

Codecov Report

graceguo-supercat commented Sep 2, 2021 • edited

m-ajay commented Sep 2, 2021

john-bodley Sep 2, 2021

Choose a reason for hiding this comment

john-bodley Sep 2, 2021

Choose a reason for hiding this comment

john-bodley Sep 3, 2021

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 1, 2021 •

edited

graceguo-supercat commented Sep 2, 2021 •

edited