-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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(filter-indicator): show filters handled by jinja as applied #17140
Conversation
45fabc4
to
f017bcf
Compare
Codecov Report
@@ Coverage Diff @@
## master #17140 +/- ##
==========================================
- Coverage 76.89% 76.87% -0.02%
==========================================
Files 1038 1038
Lines 55515 55528 +13
Branches 7564 7564
==========================================
- Hits 42690 42689 -1
- Misses 12575 12589 +14
Partials 250 250
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
ddd43be
to
05ee135
Compare
05ee135
to
7fc8528
Compare
/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true FEATURE_DASHBOARD_CROSS_FILTERS=true FEATURE_ENABLE_TEMPLATE_PROCESSING=true |
@villebro Ephemeral environment spinning up at http://34.221.208.55:8080. Credentials are |
FYI I added a test dashboard to the eph env |
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. this approach is better than before.
Ephemeral environment shutdown and build artifacts deleted. |
I'm using from_dttm and to_dttm in my dataset, standard date filter box. Should it work for from_dttm and to_dttm as well ? |
@villebro could you please help to figure out how to deal w/ 'Incompatible filters' message regarding time columns? |
Hello, I am having the same issue with the yellow warning showing when I use {{ from_dttm }} and {{ to_dttm }} in the query. |
SUMMARY
Currently only legacy charts support identifying filters applied via jinja templates. However, this only supports the
filter_values
function, and is done using regex on the virtual table query. Therefore, filters applied via theget_filters
function don't work on either v1 or legacy charts, as they aren't caught by the regex. In addition, this approach has the unlikely downside of false positives, if the regex would happen to match a parameter in the regular query, as it didn't check for opening/closing curly braces.This PR replaces the regex-based identification method with a similar approach to how removed filters are currently identified. The applied filters are added to a list during template rendering, which is made available to the code that is using the template processor. This way the collection of applied filters is automated and happens during template evaluation.
The changes proposed here are backwards compatible, and will display filters correctly when the cache is updated (for reviewers remember to force refresh the chart if caching is enabled).
BEFORE
On a dashboard with a virtual dataset referencing the
filter_values
jinja function, we can see that the ECharts plugin which uses the v1 chart data endpoint is unable to identify the applied filter value correctly due to the column referenced not being available in the dataset. Notice, however, the NVD3 line chart on the right which identifies the filter correctly due to the regex inviz.py
:AFTER
Now applied filters are identified correctly both on legacy and v1 charts:
When applying a filter whose column isn't 1) available in the dataset 2) referenced in the jinja code, the filter is also correctly identified as being incompatible:
TESTING INSTRUCTIONS
ENABLE_TEMPLATE_PROCESSING
feature flag.select *, '{{ filter_values('gender', 'unset')[0] }}' as series from random_time_series
ADDITIONAL INFORMATION