You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/03/23 10:42:46 UTC
[GitHub] [incubator-superset] villebro opened a new pull request #9348: fix:
filter_values in jinja_context not processing adhoc_filters
villebro opened a new pull request #9348: fix: filter_values in jinja_context not processing adhoc_filters
URL: https://github.com/apache/incubator-superset/pull/9348
### CATEGORY
Choose one
- [x] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [x] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
Currently the `filter_values` function in `jinja_context.py` is not able to process `adhoc_filters` when a chart is rendered in the Explore View. This PR moves filter preprocessing logic from `viz.py` into `utlis/core.py` and applies said logic to `form_data` in the Jinja context.
### TEST PLAN
Local testing + new and old tests
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: closes #9328 , closes #9301
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
@sahiljain001 @lilila
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9348: fix: filter_values in jinja_context not processing adhoc_filters
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9348: fix: filter_values in jinja_context not processing adhoc_filters
URL: https://github.com/apache/incubator-superset/pull/9348#discussion_r396934586
##########
File path: superset/jinja_context.py
##########
@@ -93,18 +94,16 @@ def filter_values(column: str, default: Optional[str] = None) -> List[str]:
:return: returns a list of filter values
"""
form_data = json.loads(request.form.get("form_data", "{}"))
+ utils.harmonize_query_filters(form_data)
+
return_val = []
- for filter_type in ["filters", "extra_filters"]:
- if filter_type not in form_data:
- continue
-
- for f in form_data[filter_type]:
- if f["col"] == column:
- if isinstance(f["val"], list):
- for v in f["val"]:
- return_val.append(v)
- else:
- return_val.append(f["val"])
+ for f in form_data.get("filters", {}):
Review 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.py` has no logic for handling `adhoc_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.
- The new QueryObject used 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 that `adhoc_filters` are allowed in the constructor signature and are converted to base filters in `to_dict()`.
Anyway, in the short term, I think this fix is the right solution.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] lilila commented on issue #9348: fix:
filter_values in jinja_context not processing adhoc_filters
Posted by GitBox <gi...@apache.org>.
lilila commented on issue #9348: fix: filter_values in jinja_context not processing adhoc_filters
URL: https://github.com/apache/incubator-superset/pull/9348#issuecomment-603135589
WOW , it works very well. Thank you @villebro <3
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] lilila commented on issue #9348: fix:
filter_values in jinja_context not processing adhoc_filters
Posted by GitBox <gi...@apache.org>.
lilila commented on issue #9348: fix: filter_values in jinja_context not processing adhoc_filters
URL: https://github.com/apache/incubator-superset/pull/9348#issuecomment-604304459
Yes @villebro it is on public role.
Do you know how to preserve label_colors when displayed in "standalone" mode?
When standalone is not set to true, the plot has the colors defined in the dashboard, but unfortunately the standard airbnb colors come back when standalone is set to true. (However, I notice all labels colors are sent in the URL)
Thank you for your help
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9348: fix: filter_values in jinja_context not processing adhoc_filters
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9348: fix: filter_values in jinja_context not processing adhoc_filters
URL: https://github.com/apache/incubator-superset/pull/9348#discussion_r396934586
##########
File path: superset/jinja_context.py
##########
@@ -93,18 +94,16 @@ def filter_values(column: str, default: Optional[str] = None) -> List[str]:
:return: returns a list of filter values
"""
form_data = json.loads(request.form.get("form_data", "{}"))
+ utils.harmonize_query_filters(form_data)
+
return_val = []
- for filter_type in ["filters", "extra_filters"]:
- if filter_type not in form_data:
- continue
-
- for f in form_data[filter_type]:
- if f["col"] == column:
- if isinstance(f["val"], list):
- for v in f["val"]:
- return_val.append(v)
- else:
- return_val.append(f["val"])
+ for f in form_data.get("filters", {}):
Review 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.py` has no logic for handling `adhoc_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.
- `QueryObject` used 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 that `adhoc_filters` are allowed in the constructor signature and are converted to base filters in `to_dict()`.
Anyway, in the short term, I think this fix is the right solution.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9348: fix:
filter_values in jinja_context not processing adhoc_filters
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9348: fix: filter_values in jinja_context not processing adhoc_filters
URL: https://github.com/apache/incubator-superset/pull/9348#issuecomment-604306782
@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!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] lilila edited a comment on issue #9348: fix:
filter_values in jinja_context not processing adhoc_filters
Posted by GitBox <gi...@apache.org>.
lilila edited a comment on issue #9348: fix: filter_values in jinja_context not processing adhoc_filters
URL: https://github.com/apache/incubator-superset/pull/9348#issuecomment-604328385
I'll do this
- problem of colors (https://github.com/apache/incubator-superset/issues/9395)
- warning message (https://github.com/apache/incubator-superset/issues/9394)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] lilila commented on issue #9348: fix:
filter_values in jinja_context not processing adhoc_filters
Posted by GitBox <gi...@apache.org>.
lilila commented on issue #9348: fix: filter_values in jinja_context not processing adhoc_filters
URL: https://github.com/apache/incubator-superset/pull/9348#issuecomment-603794932
This is a great improvement, thank you again.
Just a comment @villebro , if I display a plot in a an iFrame (using the html code provided in the iframe tab in the explore chart page) I have a message ("Action required: Preview ....") which appears every time the plot is loaded as shown in the pic bellow
<img width="868" alt="Screenshot 2020-03-25 at 12 41 16" src="https://user-images.githubusercontent.com/607430/77533004-55b07980-6e96-11ea-83d6-2fcf53d34ca2.png">
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on a change in pull
request #9348: fix: filter_values in jinja_context not processing
adhoc_filters
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #9348: fix: filter_values in jinja_context not processing adhoc_filters
URL: https://github.com/apache/incubator-superset/pull/9348#discussion_r396637763
##########
File path: superset/jinja_context.py
##########
@@ -93,18 +94,16 @@ def filter_values(column: str, default: Optional[str] = None) -> List[str]:
:return: returns a list of filter values
"""
form_data = json.loads(request.form.get("form_data", "{}"))
+ utils.harmonize_query_filters(form_data)
+
return_val = []
- for filter_type in ["filters", "extra_filters"]:
- if filter_type not in form_data:
- continue
-
- for f in form_data[filter_type]:
- if f["col"] == column:
- if isinstance(f["val"], list):
- for v in f["val"]:
- return_val.append(v)
- else:
- return_val.append(f["val"])
+ for f in form_data.get("filters", {}):
Review comment:
I was working on filters recently and I'm pretty sure these are now under `adhoc_filters`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9348: fix:
filter_values in jinja_context not processing adhoc_filters
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9348: fix: filter_values in jinja_context not processing adhoc_filters
URL: https://github.com/apache/incubator-superset/pull/9348#issuecomment-604038716
@mistercrunch is this ok to merge?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on a change in pull
request #9348: fix: filter_values in jinja_context not processing
adhoc_filters
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #9348: fix: filter_values in jinja_context not processing adhoc_filters
URL: https://github.com/apache/incubator-superset/pull/9348#discussion_r396926131
##########
File path: superset/jinja_context.py
##########
@@ -93,18 +94,16 @@ def filter_values(column: str, default: Optional[str] = None) -> List[str]:
:return: returns a list of filter values
"""
form_data = json.loads(request.form.get("form_data", "{}"))
+ utils.harmonize_query_filters(form_data)
+
return_val = []
- for filter_type in ["filters", "extra_filters"]:
- if filter_type not in form_data:
- continue
-
- for f in form_data[filter_type]:
- if f["col"] == column:
- if isinstance(f["val"], list):
- for v in f["val"]:
- return_val.append(v)
- else:
- return_val.append(f["val"])
+ for f in form_data.get("filters", {}):
Review comment:
Oh wow. This is confusing. Looks like you looked into it deeper than I have. 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9348: fix:
filter_values in jinja_context not processing adhoc_filters
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9348: fix: filter_values in jinja_context not processing adhoc_filters
URL: https://github.com/apache/incubator-superset/pull/9348#issuecomment-603858945
@lilila I assume this is on the Public role?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request
#9348: fix: filter_values in jinja_context not processing adhoc_filters
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9348: fix: filter_values in jinja_context not processing adhoc_filters
URL: https://github.com/apache/incubator-superset/pull/9348#discussion_r396646245
##########
File path: superset/jinja_context.py
##########
@@ -93,18 +94,16 @@ def filter_values(column: str, default: Optional[str] = None) -> List[str]:
:return: returns a list of filter values
"""
form_data = json.loads(request.form.get("form_data", "{}"))
+ utils.harmonize_query_filters(form_data)
+
return_val = []
- for filter_type in ["filters", "extra_filters"]:
- if filter_type not in form_data:
- continue
-
- for f in form_data[filter_type]:
- if f["col"] == column:
- if isinstance(f["val"], list):
- for v in f["val"]:
- return_val.append(v)
- else:
- return_val.append(f["val"])
+ for f in form_data.get("filters", {}):
Review 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9348: fix: filter_values in jinja_context not processing adhoc_filters
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9348:
URL: https://github.com/apache/incubator-superset/pull/9348#issuecomment-616399040
I'm closing this in favor of #9582 .
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] lilila commented on issue #9348: fix:
filter_values in jinja_context not processing adhoc_filters
Posted by GitBox <gi...@apache.org>.
lilila commented on issue #9348: fix: filter_values in jinja_context not processing adhoc_filters
URL: https://github.com/apache/incubator-superset/pull/9348#issuecomment-604328385
I'll do this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] lilila edited a comment on issue #9348: fix:
filter_values in jinja_context not processing adhoc_filters
Posted by GitBox <gi...@apache.org>.
lilila edited a comment on issue #9348: fix: filter_values in jinja_context not processing adhoc_filters
URL: https://github.com/apache/incubator-superset/pull/9348#issuecomment-604328385
I'll do this
- problem of colors is described [here](https://github.com/apache/incubator-superset/issues/9395#issue-588290411)
- warning message is [here] (https://github.com/apache/incubator-superset/issues/9394)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9348: fix:
filter_values in jinja_context not processing adhoc_filters
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9348: fix: filter_values in jinja_context not processing adhoc_filters
URL: https://github.com/apache/incubator-superset/pull/9348#issuecomment-602531055
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9348?src=pr&el=h1) Report
> Merging [#9348](https://codecov.io/gh/apache/incubator-superset/pull/9348?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/5e6662ab12e4b5d3c392e8cc1bfd37afc5b20b63&el=desc) will **not change** coverage by `%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9348?src=pr&el=tree)
```diff
@@ 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](https://codecov.io/gh/apache/incubator-superset/pull/9348?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9348?src=pr&el=footer). Last update [5e6662a...b1e66d3](https://codecov.io/gh/apache/incubator-superset/pull/9348?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org