-
Notifications
You must be signed in to change notification settings - Fork 13.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(plugin-chart-echarts): Apply temporary filters to Query B in explore #18998
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
❗ Please consider rebasing your branch to avoid db migration conflicts. |
superset/utils/core.py
Outdated
@@ -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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No place for reuse. We don't need to create functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point!
14a2553
to
013dfe1
Compare
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 | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
oradhoc_filters_2
, feels slightly arbitrary. The logic behind my reasoning here was, "any control that starts withadhoc_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 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for discussion. Do we agree that there's no action/change required here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
❗ Please consider rebasing your branch to avoid db migration conflicts. |
1 similar comment
❗ Please consider rebasing your branch to avoid db migration conflicts. |
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/dndControls.tsx
Show resolved
Hide resolved
013dfe1
to
26d9363
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
How to get this backported to 1.4? Or check in which version this fix would be included in? |
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:
![image](https://user-images.githubusercontent.com/15073128/156404079-ddf01b2d-6b4e-41e7-9616-42f0aae9a068.png)
After:
![image](https://user-images.githubusercontent.com/15073128/156403586-b0fac9cf-1dbe-4b8c-91e6-c108801f0ef8.png)
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
DB migration stats:
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