fix(plugin-chart-echarts): Apply temporary filters to Query B in explore #18998

kgabryje · 2022-03-02T16:26:08Z

SUMMARY

When going from the dashboard to Explore view with extra filters (native filters or filter box), the filters are only appended to the first query. This PR iterates through form data and adds extra filters to every field that matches pattern adhoc_filters.*.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TESTING INSTRUCTIONS

Create a mixed timeseries chart and add it to a dashboard
Go to that dashboard and apply some native filters
Click View chart in explore on mixed timeseries chart
Verify that native filter is applied in both Query A and Query B adhoc filter control

ADDITIONAL INFORMATION

Has associated issue: fixes [chart][mixed time series chart] Mixed time series chart only show dashboard filter from query A #15858
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

DB migration stats:

Current: 0.42 s
10+: 0.44 s
100+: 0.26 s
1000+: 0.25 s

This PR replaces #18740

https://app.shortcut.com/preset/story/37242/explore-mixed-time-series-chart-native-filter-only-apply-to-query-a-but-not-query-b-when-open-mixed-time-series-chart-from

codecov · 2022-03-02T17:03:30Z

Codecov Report

Merging #18998 (26d9363) into master (299b5dc) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #18998   +/-   ##
=======================================
  Coverage   66.56%   66.56%           
=======================================
  Files        1641     1641           
  Lines       63495    63497    +2     
  Branches     6425     6425           
=======================================
+ Hits        42265    42267    +2     
  Misses      19550    19550           
  Partials     1680     1680

Flag	Coverage Δ
hive	`52.59% <0.00%> (-0.01%)`	⬇️
javascript	`51.37% <ø> (ø)`
mysql	`81.81% <100.00%> (+<0.01%)`	⬆️
postgres	`81.86% <100.00%> (+<0.01%)`	⬆️
presto	`52.43% <0.00%> (-0.01%)`	⬇️
python	`82.29% <100.00%> (+<0.01%)`	⬆️
sqlite	`81.55% <100.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...chart-controls/src/shared-controls/dndControls.tsx	`35.89% <ø> (ø)`
...et-ui-chart-controls/src/shared-controls/index.tsx	`36.36% <ø> (ø)`
superset/utils/core.py	`90.25% <100.00%> (+0.02%)`	⬆️

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 299b5dc...26d9363. Read the comment docs.

github-actions · 2022-03-02T18:21:17Z

⚠️ @kgabryje Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

superset/utils/core.py

zhaoyongjie · 2022-03-03T07:59:09Z

superset/utils/core.py

@@ -1056,6 +1056,16 @@ def form_data_to_adhoc(form_data: Dict[str, Any], clause: str) -> AdhocFilterCla
    return result


+def append_filters_to_adhoc_filters(


No place for reuse. We don't need to create functions.

good point!

ktmud · 2022-03-03T18:10:24Z

superset/utils/core.py

+        for key, value in form_data.items():
+            if re.match("adhoc_filter.*", key):
+                value.extend(
+                    simple_filter_to_adhoc({"isExtra": True, **fltr})  # type: ignore
+                    for fltr in append_filters
+                    if fltr
+                )


Suggested change

for key, value in form_data.items():

if re.match("adhoc_filter.*", key):

value.extend(

simple_filter_to_adhoc({"isExtra": True, **fltr}) # type: ignore

for fltr in append_filters

if fltr

)

for key in ["adhoc_filters", "adhoc_filters_b"]:

if key in form_data:

form_data[key] = (form_data.get(key) or []) + [

simple_filter_to_adhoc({"isExtra": True, **fltr}) # type: ignore

for fltr in append_filters if fltr

]

I think we can be more explicit about the field names to avoid looping through all fields?

@ktmud in offline discussions I was the person who proposed using a regex to loop through this, so don't blame @kgabryje for this 😄 My reasoning went something like this:

it's fairly uncommon for there to be more than 20 controls. So performance impact of looping through all control keys is negligible

While I know that this is put here mostly for the Mixed Timeseries chart, I don't see any reason why someone wouldn't consider having 3 (or more) query sections. Only supporting one additional control called adhoc_filters_b, but not, say, adhoc_filters_c or adhoc_filters_2, feels slightly arbitrary. The logic behind my reasoning here was, "any control that starts with adhoc_filters should be considered an adhoc filter control, and extra filters should apply to them all". While both solutions solve this specific problem, I still feel the regex solution is less coupled to the Mixed Timeseries chart than only matching ["adhoc_filters", "adhoc_filters_b"]

Having said that, this was really just a minor nit and likely irrelevant in the grand scheme of things, so I won't get hung up on this either way 🙂

Another argument for using more explicit field names is minimizing unintentional side-effects. While the probability is small, there is no mechanism preventing someone from naming a non adhoc-filter field with adhoc_filter in the name. The opposite is also true---one may name an adhoc filters field without it starting with adhoc_filter. We've seen it for metrics before, which is why we have the METRIC_KEYS constant.

This is true. However, naming a control that doesn't contain adhoc filters with the prefix "adhoc_filters" feels less likely than than the opposite.

Thanks for discussion. Do we agree that there's no action/change required here?

I still believe in general regex should be used sparingly and explicit is better than implicit. Since you can't stop someone from naming a new filter field things like "series_b_filters", the bug may still come back even with a regex.

In any case, we should need a consistent way of mapping field names to field types because most of the time what you want is "do something special based on filter type", not filed names. One obvious solution is to create an ADHOC_FILTER_KEYS constant and put it together with METRIC_KEYS in some constants.py file, but I agree this by itself probably doesn't make much difference on the grander scheme of things.

I agree that we should make all these things as explicit as possible, at a minimum documenting magic behavior like this. While on this subject, the whole notion of QueryFormData automatically mapping properties like metric and secondary_metric to metrics in QueryObject is really opaque, and should probably both 1) be more thoroughly documented 2) be phased out in coming major versions. But in the short term I propose adding documentation around this functionality to at least explain how this stuff works right now.

github-actions · 2022-03-03T20:18:13Z

⚠️ @kgabryje Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

github-actions · 2022-03-04T00:58:08Z

⚠️ @kgabryje Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/dndControls.tsx

villebro

LGTM

raags · 2022-03-11T14:49:56Z

How to get this backported to 1.4? Or check in which version this fix would be included in?

…ore (#18998) * fix(explore): Extra filters not applied to query b in mixed timeseries * Add return type * Apply review comment * Fix non-dnd filters (cherry picked from commit 9f834e8)

kgabryje requested a review from villebro March 2, 2022 16:26

kgabryje requested a review from a team as a code owner March 2, 2022 16:26

pull-request-size bot added the size/L label Mar 2, 2022

kgabryje mentioned this pull request Mar 2, 2022

fix(plugin-chart-echarts): Apply temporary filters to Query B in explore #18740

Closed

9 tasks

zhaoyongjie reviewed Mar 3, 2022

View reviewed changes

superset/utils/core.py Outdated Show resolved Hide resolved

zhaoyongjie reviewed Mar 3, 2022

View reviewed changes

kgabryje force-pushed the fix/mixed-timeseries-extra branch from 14a2553 to 013dfe1 Compare March 3, 2022 16:50

pull-request-size bot added size/M and removed size/L labels Mar 3, 2022

ktmud reviewed Mar 3, 2022

View reviewed changes

villebro reviewed Mar 4, 2022

View reviewed changes

superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/dndControls.tsx Show resolved Hide resolved

kgabryje added 4 commits March 4, 2022 13:20

fix(explore): Extra filters not applied to query b in mixed timeseries

6e9f3b5

Add return type

8a4f8dc

Apply review comment

4ee7e13

Fix non-dnd filters

26d9363

kgabryje force-pushed the fix/mixed-timeseries-extra branch from 013dfe1 to 26d9363 Compare March 4, 2022 12:33

pull-request-size bot added size/L and removed size/M labels Mar 4, 2022

villebro approved these changes Mar 4, 2022

View reviewed changes

kgabryje merged commit 9f834e8 into apache:master Mar 4, 2022

kgabryje mentioned this pull request Mar 10, 2022

fix(explore): remove extra filters from all adhoc_filters controls #16231

Closed

8 tasks

kgabryje added the lts-v1 label Mar 22, 2022

meyergin mentioned this pull request Aug 3, 2022

In chart view, the saved adhoc_filters in query_b is missing #20952

Closed

2 tasks

mistercrunch added the 🍒 1.5.0 label Mar 13, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(plugin-chart-echarts): Apply temporary filters to Query B in explore #18998

fix(plugin-chart-echarts): Apply temporary filters to Query B in explore #18998

kgabryje commented Mar 2, 2022 •

edited

Loading

codecov bot commented Mar 2, 2022 •

edited

Loading

github-actions bot commented Mar 2, 2022

zhaoyongjie Mar 3, 2022

kgabryje Mar 3, 2022

ktmud Mar 3, 2022 •

edited

Loading

villebro Mar 3, 2022

ktmud Mar 3, 2022

villebro Mar 3, 2022

ktmud Mar 3, 2022

kgabryje Mar 4, 2022

ktmud Mar 4, 2022

villebro Mar 4, 2022

github-actions bot commented Mar 3, 2022

github-actions bot commented Mar 4, 2022

villebro left a comment

raags commented Mar 11, 2022

		@@ -1056,6 +1056,16 @@ def form_data_to_adhoc(form_data: Dict[str, Any], clause: str) -> AdhocFilterCla
		return result


		def append_filters_to_adhoc_filters(

fix(plugin-chart-echarts): Apply temporary filters to Query B in explore #18998

fix(plugin-chart-echarts): Apply temporary filters to Query B in explore #18998

Conversation

kgabryje commented Mar 2, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Mar 2, 2022 • edited Loading

Codecov Report

github-actions bot commented Mar 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud Mar 3, 2022 • edited Loading

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

github-actions bot commented Mar 3, 2022

github-actions bot commented Mar 4, 2022

villebro left a comment

Choose a reason for hiding this comment

raags commented Mar 11, 2022

kgabryje commented Mar 2, 2022 •

edited

Loading

codecov bot commented Mar 2, 2022 •

edited

Loading

ktmud Mar 3, 2022 •

edited

Loading