fix: filter_values in jinja_context not processing adhoc_filters #9348
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9348 +/- ##
=======================================
Coverage 58.96% 58.96%
=======================================
Files 374 374
Lines 12137 12137
Branches 2992 2992
=======================================
Hits 7156 7156
Misses 4802 4802
Partials 179 179 Continue to review full report at Codecov.
|
| return_val.append(v) | ||
| else: | ||
| return_val.append(f["val"]) | ||
| for f in form_data.get("filters", {}): |
There was a problem hiding this comment.
I was working on filters recently and I'm pretty sure these are now under adhoc_filters
There was a problem hiding this comment.
This wan't completely familiar territory to me, so I might be wrong here. But currently process_query_filters() in viz..py, which is the first thing query_obj() calls, goes through a long step of transformations that first changes everything into adhoc_filters, and finally changes those to regular filters. I'm unsure why this happens, but my takeaway was that if process_query_filters is run, you are left with nothing but filters, despite how they started out.
There was a problem hiding this comment.
Oh wow. This is confusing. Looks like you looked into it deeper than I have. 👍
There was a problem hiding this comment.
Probably something for the refactor backlog. I took another pass at how this works in practice, and these are my takeaways:
connectors/sqla/model.pyhas no logic for handlingadhoc_metrics, so all filters are expected to be in base filter form: https://github.com/apache/incubator-superset/blob/master/superset/connectors/sqla/models.py#L837-L877 . The same goes for the legacy Druid connector.QueryObjectused by the new Query API only supports base filters: https://github.com/apache/incubator-superset/blob/master/superset/common/query_object.py#L115 . I think we need to change this so thatadhoc_filtersare allowed in the constructor signature and are converted to base filters into_dict().
Anyway, in the short term, I think this fix is the right solution.
|
WOW , it works very well. Thank you @villebro <3 |
|
This is a great improvement, thank you again.
|
|
@lilila I assume this is on the Public role? |
|
@mistercrunch is this ok to merge? |
|
Yes @villebro it is on public role. Thank you for your help |
|
@lilila I expect these are bugs that surface due to the Public role which isn't as frequently used as the regular ones. Would you mind opening issues for these, preferably one per problem? It will make it easier to identify and fix the bugs. Thanks! |
|
I'll do this
|
|
I'm closing this in favor of #9582 . |
CATEGORY
Choose one
SUMMARY
Currently the
filter_valuesfunction injinja_context.pyis not able to processadhoc_filterswhen a chart is rendered in the Explore View. This PR moves filter preprocessing logic fromviz.pyintoutlis/core.pyand applies said logic toform_datain the Jinja context.TEST PLAN
Local testing + new and old tests
ADDITIONAL INFORMATION
REVIEWERS
@sahiljain001 @lilila