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/16 18:43:53 UTC
[GitHub] [incubator-superset] graceguo-supercat opened a new pull request
#9311: [cache warm_up] warm_up slice with dashboard default_filters
graceguo-supercat opened a new pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311
### CATEGORY
Choose one
- [x] Bug Fix
- [x] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
Superset offers a cache warm_up endpoint:
https://github.com/apache/incubator-superset/blob/8764ae385206c8bdccba1e7aa42def1929f6fba7/superset/views/core.py#L1645
It will generate a query like `form_data={slice_id: _slice_id_}` so that we can warm_up query saved with a given slice_id.
Now a lot dashboard are having default filters (and filters can have scopes), warm_up single slice without dashboard context make our cache hit rate declining. Currently in airbnb we have 10% dashboard with default filters, but 20% of dashboards are landed with default_filters.
This PR is to add dashboard context into the cache warm_up call. You can pass extra dashboard_id parameter to `wam_up` API, indicating the slice is called with a given dashboard. It should generate a query like
```
form_data = {"slice_id":926, "extra_filters": [
{
"col": "region",
"op": "in",
"val": [
"East Asia & Pacific",
"Latin America & Caribbean"
]
}
]}
```
### TEST PLAN
added new unit tests
### REVIEWERS
@john-bodley @serenajiang @etr2460
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9311: [cache warm_up] warm_up slice with dashboard
default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393986510
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ slc = session.query(Slice).filter_by(id=slice_id).one_or_none()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slc not in dashboard.slices
+ ):
+ return []
+
+ # is this dashboard has default filters?
+ json_metadata = json.loads(dashboard.json_metadata)
+ default_filters = json.loads(json_metadata.get("default_filters", "null"))
+ if not default_filters:
+ return []
+
+ # is default filters applicable to the given slice?
+ filter_scopes = json_metadata.get("filter_scopes", {})
+ layout = json.loads(dashboard.position_json or "{}")
+
+ return build_extra_filters(layout, filter_scopes, default_filters, slice_id)
+
+
+def build_extra_filters(
+ layout: Dict,
+ filter_scopes: Dict,
+ default_filters: Dict[str, Dict[str, List]],
+ slice_id: int,
+) -> List[Dict[str, Any]]:
+ extra_filters = []
+
+ # do not apply filters if chart if chart not in filter's scope or
Review comment:
fixed
----------------------------------------------------------------
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] graceguo-supercat commented on issue #9311:
[cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on issue #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#issuecomment-600327324
> What i'm saying is if you default it to `[]` with or then you don't even need to check if it's falsey. The empty array will simply bypass the for loop
i didn't check the value is falsey or not. My question is this comment:
`after looking through this and not knowing potential values for the filter object, i'm wondering if most of the .get("value", {}) and .get("value", []) statements should be .get("value") or {} and .get("value") or []`
Why should change most of `dict.get(key, default_value)` to `dict.get(key) or default_value`?
----------------------------------------------------------------
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] etr2460 commented on issue #9311: [cache
warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#issuecomment-600318023
The main difference here applies if it's possible for a key to be set to a falsey value. If you have the dict:
```python
d = {
"key": None,
"foo": False,
}
```
then:
```python
d.get("key", []) # equals None
d.get("key", []) # equals None
```
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9311: [cache warm_up] warm_up slice with dashboard
default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r394046702
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,80 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+# see all dashboard components type in
+# /superset-frontend/src/dashboard/util/componentTypes.js
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
Review comment:
here i check json_metadata exists and not None.
----------------------------------------------------------------
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 edited a comment on issue #9311:
[cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#issuecomment-599720147
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9311?src=pr&el=h1) Report
> Merging [#9311](https://codecov.io/gh/apache/incubator-superset/pull/9311?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/b1916a190e12a4da4b9afaca414ed8bd59a024c8&el=desc) will **increase** coverage by `0.00%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9311?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9311 +/- ##
=======================================
Coverage 59.08% 59.08%
=======================================
Files 374 374
Lines 12202 12205 +3
Branches 2986 2989 +3
=======================================
+ Hits 7209 7211 +2
- Misses 4814 4815 +1
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9311?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...plore/components/controls/FixedOrMetricControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9311/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaXhlZE9yTWV0cmljQ29udHJvbC5qc3g=) | `51.78% <0.00%> (+0.84%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9311?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/9311?src=pr&el=footer). Last update [b1916a1...9752ff4](https://codecov.io/gh/apache/incubator-superset/pull/9311?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
[GitHub] [incubator-superset] graceguo-supercat commented on a change in
pull request #9311: [cache warm_up] warm_up slice with dashboard
default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393986758
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ slc = session.query(Slice).filter_by(id=slice_id).one_or_none()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slc not in dashboard.slices
+ ):
+ return []
+
+ # is this dashboard has default filters?
+ json_metadata = json.loads(dashboard.json_metadata)
+ default_filters = json.loads(json_metadata.get("default_filters", "null"))
+ if not default_filters:
+ return []
+
+ # is default filters applicable to the given slice?
+ filter_scopes = json_metadata.get("filter_scopes", {})
+ layout = json.loads(dashboard.position_json or "{}")
+
+ return build_extra_filters(layout, filter_scopes, default_filters, slice_id)
+
+
+def build_extra_filters(
+ layout: Dict,
+ filter_scopes: Dict,
+ default_filters: Dict[str, Dict[str, List]],
+ slice_id: int,
+) -> List[Dict[str, Any]]:
+ extra_filters = []
+
+ # do not apply filters if chart if chart not in filter's scope or
+ # chart is immune to the filter
+ for filter_id, columns in default_filters.items():
+ scopes_by_filter_field = filter_scopes.get(filter_id, {})
+ for col, val in columns.items():
+ current_field_scopes = scopes_by_filter_field.get(col, {})
+ # scope is list of container ids
+ scope = current_field_scopes.get("scope", ["ROOT_ID"])
+ # immune is list of slice ids
+ immune = current_field_scopes.get("immune", [])
+
+ for container_id in scope:
+ if slice_id not in immune and is_slice_in_container(
+ layout, container_id, slice_id
+ ):
+ extra_filters.append({"col": col, "op": "in", "val": val})
+
+ return extra_filters
+
+
+def is_slice_in_container(layout: Dict, container_id: str, slice_id: int) -> bool:
+ if container_id == "ROOT_ID":
+ return True
+
+ node = layout[container_id]
+ node_type = node.get("type")
+ if (
+ node_type == "CHART"
+ and node.get("meta")
+ and node.get("meta").get("chartId", 0) == slice_id
+ ):
+ return True
+
+ if node_type in CONTAINER_TYPES:
+ children = node.get("children", [])
+ if children:
Review comment:
fixed.
----------------------------------------------------------------
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] etr2460 commented on issue #9311: [cache
warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#issuecomment-600323126
What i'm saying is if you default it to `[]` with or then you don't even need to check if it's falsey. The empty array will simply bypass the for loop
----------------------------------------------------------------
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] graceguo-supercat commented on issue #9311:
[cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on issue #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#issuecomment-600316344
> looking good so far!
>
> after looking through this and not knowing potential values for the filter object, i'm wondering if most of the `.get("value", {})` and `.get("value", [])` statements should be `.get("value") or {}` and `.get("value") or []`
I follow documentation here:
https://docs.python.org/2/library/stdtypes.html#dict.get
and
https://stackoverflow.com/questions/33263623/dict-getkey-default-vs-dict-getkey-or-default
I would say most of my usage is for the dict that doesn't have key. Why do you think they should be `.get("value") or {}` and `.get("value") or []`?
----------------------------------------------------------------
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] etr2460 edited a comment on issue #9311:
[cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
etr2460 edited a comment on issue #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#issuecomment-600318023
The main difference here applies if it's possible for a key to be set to a falsey value. If you have the dict:
```python
d = {
"key": None,
"foo": False,
}
```
then:
```python
d.get("key", []) # equals None
d.get("foo", {}) # equals None
d.get("key") or [] # equals []
d.get("foo") or {} # equals {}
```
----------------------------------------------------------------
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] graceguo-supercat merged pull request #9311:
[cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat merged pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9311: [cache warm_up] warm_up slice with dashboard
default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r394056731
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,88 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+# see all dashboard components type in
+# /superset-frontend/src/dashboard/util/componentTypes.js
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slice_id not in [slc.id for slc in dashboard.slices]
+ ):
+ return []
+
+ try:
+ # does this dashboard have default filters?
+ json_metadata = json.loads(dashboard.json_metadata)
+ default_filters = json.loads(json_metadata.get("default_filters", "null"))
+ if not default_filters:
+ return []
+
+ # are default filters applicable to the given slice?
+ filter_scopes = json_metadata.get("filter_scopes", {})
+ layout = json.loads(dashboard.position_json or "{}")
+
+ return build_extra_filters(layout, filter_scopes, default_filters, slice_id)
+ except json.JSONDecodeError:
+ return []
+
+
+def build_extra_filters(
+ layout: Dict,
+ filter_scopes: Dict,
+ default_filters: Dict[str, Dict[str, List]],
+ slice_id: int,
+) -> List[Dict[str, Any]]:
Review comment:
added type check for layout, filter_scopes and default_filters, make sure they are Dictionary
----------------------------------------------------------------
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] graceguo-supercat edited a comment on issue
#9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on issue #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#issuecomment-600316344
> looking good so far!
>
> after looking through this and not knowing potential values for the filter object, i'm wondering if most of the `.get("value", {})` and `.get("value", [])` statements should be `.get("value") or {}` and `.get("value") or []`
@etr2460 Thanks for code review! I follow documentation here:
https://docs.python.org/2/library/stdtypes.html#dict.get
and
https://stackoverflow.com/questions/33263623/dict-getkey-default-vs-dict-getkey-or-default
I would say most of my usage is for the dict that doesn't have key. Why do you think they should be `.get("value") or {}` and `.get("value") or []`?
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9311: [cache warm_up] warm_up slice with dashboard
default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r394056155
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,88 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+# see all dashboard components type in
+# /superset-frontend/src/dashboard/util/componentTypes.js
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slice_id not in [slc.id for slc in dashboard.slices]
+ ):
+ return []
+
+ try:
Review comment:
add try catch exception for parsing dashboard metadata
----------------------------------------------------------------
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] john-bodley commented on a change in pull
request #9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393826986
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ slc = session.query(Slice).filter_by(id=slice_id).one_or_none()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slc not in dashboard.slices
+ ):
+ return []
+
+ # is this dashboard has default filters?
+ json_metadata = json.loads(dashboard.json_metadata)
+ default_filters = json.loads(json_metadata.get("default_filters", "null"))
+ if not default_filters:
+ return []
+
+ # is default filters applicable to the given slice?
+ filter_scopes = json_metadata.get("filter_scopes", {})
+ layout = json.loads(dashboard.position_json or "{}")
+
+ return build_extra_filters(layout, filter_scopes, default_filters, slice_id)
+
+
+def build_extra_filters(
+ layout: Dict,
+ filter_scopes: Dict,
+ default_filters: Dict[str, Dict[str, List]],
+ slice_id: int,
+) -> List[Dict[str, Any]]:
+ extra_filters = []
+
+ # do not apply filters if chart if chart not in filter's scope or
+ # chart is immune to the filter
+ for filter_id, columns in default_filters.items():
+ scopes_by_filter_field = filter_scopes.get(filter_id, {})
+ for col, val in columns.items():
+ current_field_scopes = scopes_by_filter_field.get(col, {})
+ # scope is list of container ids
+ scope = current_field_scopes.get("scope", ["ROOT_ID"])
+ # immune is list of slice ids
+ immune = current_field_scopes.get("immune", [])
+
+ for container_id in scope:
+ if slice_id not in immune and is_slice_in_container(
+ layout, container_id, slice_id
+ ):
+ extra_filters.append({"col": col, "op": "in", "val": val})
+
+ return extra_filters
+
+
+def is_slice_in_container(layout: Dict, container_id: str, slice_id: int) -> bool:
+ if container_id == "ROOT_ID":
+ return True
+
+ node = layout[container_id]
+ node_type = node.get("type")
+ if (
+ node_type == "CHART"
+ and node.get("meta")
+ and node.get("meta").get("chartId", 0) == slice_id
Review comment:
You could concat lines 336 and 337 to be,
```python
and node.get("meta", {}).get("chartId") == slice_id
```
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9311: [cache warm_up] warm_up slice with dashboard
default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393986632
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ slc = session.query(Slice).filter_by(id=slice_id).one_or_none()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slc not in dashboard.slices
+ ):
+ return []
+
+ # is this dashboard has default filters?
+ json_metadata = json.loads(dashboard.json_metadata)
+ default_filters = json.loads(json_metadata.get("default_filters", "null"))
+ if not default_filters:
+ return []
+
+ # is default filters applicable to the given slice?
+ filter_scopes = json_metadata.get("filter_scopes", {})
+ layout = json.loads(dashboard.position_json or "{}")
+
+ return build_extra_filters(layout, filter_scopes, default_filters, slice_id)
+
+
+def build_extra_filters(
+ layout: Dict,
+ filter_scopes: Dict,
+ default_filters: Dict[str, Dict[str, List]],
+ slice_id: int,
+) -> List[Dict[str, Any]]:
+ extra_filters = []
+
+ # do not apply filters if chart if chart not in filter's scope or
+ # chart is immune to the filter
+ for filter_id, columns in default_filters.items():
+ scopes_by_filter_field = filter_scopes.get(filter_id, {})
+ for col, val in columns.items():
+ current_field_scopes = scopes_by_filter_field.get(col, {})
+ # scope is list of container ids
+ scope = current_field_scopes.get("scope", ["ROOT_ID"])
+ # immune is list of slice ids
+ immune = current_field_scopes.get("immune", [])
Review comment:
fixed.
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9311: [cache warm_up] warm_up slice with dashboard
default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393258452
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
Review comment:
Celery task is here: https://github.com/apache/incubator-superset/blob/000a038af193f2770dfe23a053bd1ff5e9e72bd4/superset/tasks/cache.py#L39
It missed a logic to check default_filters's scope if the scope is not globally, but pretty close. So my plan is to add logic for warm_up endpoint, then update Celery task to re-use this function in another PR.
----------------------------------------------------------------
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] john-bodley commented on a change in pull
request #9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393242301
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
Review comment:
@graceguo-supercat I thought this logic already existed in the backend and the idea we discussed was possibly refactoring the logic to ensure that this as well as the Celery warmup logic was using the same dashboard filter logic.
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9311: [cache warm_up] warm_up slice with dashboard
default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393986902
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ slc = session.query(Slice).filter_by(id=slice_id).one_or_none()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slc not in dashboard.slices
+ ):
+ return []
+
+ # is this dashboard has default filters?
+ json_metadata = json.loads(dashboard.json_metadata)
+ default_filters = json.loads(json_metadata.get("default_filters", "null"))
+ if not default_filters:
+ return []
+
+ # is default filters applicable to the given slice?
+ filter_scopes = json_metadata.get("filter_scopes", {})
+ layout = json.loads(dashboard.position_json or "{}")
+
+ return build_extra_filters(layout, filter_scopes, default_filters, slice_id)
+
+
+def build_extra_filters(
+ layout: Dict,
+ filter_scopes: Dict,
+ default_filters: Dict[str, Dict[str, List]],
+ slice_id: int,
+) -> List[Dict[str, Any]]:
+ extra_filters = []
+
+ # do not apply filters if chart if chart not in filter's scope or
+ # chart is immune to the filter
+ for filter_id, columns in default_filters.items():
+ scopes_by_filter_field = filter_scopes.get(filter_id, {})
+ for col, val in columns.items():
+ current_field_scopes = scopes_by_filter_field.get(col, {})
+ # scope is list of container ids
+ scope = current_field_scopes.get("scope", ["ROOT_ID"])
+ # immune is list of slice ids
+ immune = current_field_scopes.get("immune", [])
+
+ for container_id in scope:
+ if slice_id not in immune and is_slice_in_container(
+ layout, container_id, slice_id
+ ):
+ extra_filters.append({"col": col, "op": "in", "val": val})
+
+ return extra_filters
+
+
+def is_slice_in_container(layout: Dict, container_id: str, slice_id: int) -> bool:
+ if container_id == "ROOT_ID":
+ return True
+
+ node = layout[container_id]
+ node_type = node.get("type")
+ if (
+ node_type == "CHART"
+ and node.get("meta")
+ and node.get("meta").get("chartId", 0) == slice_id
+ ):
+ return True
+
+ if node_type in CONTAINER_TYPES:
+ children = node.get("children", [])
+ if children:
+ # for child_id in children_ids:
+ if any(
Review comment:
fixed.
----------------------------------------------------------------
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] john-bodley commented on a change in pull
request #9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r394101165
##########
File path: superset/tasks/cache.py
##########
@@ -54,35 +56,27 @@ def get_form_data(chart_id, dashboard=None):
if not default_filters:
return form_data
- # do not apply filters if chart is immune to them
- immune_fields = []
filter_scopes = json_metadata.get("filter_scopes", {})
- if filter_scopes:
- for scopes in filter_scopes.values():
- for (field, scope) in scopes.items():
- if chart_id in scope.get("immune", []):
- immune_fields.append(field)
-
- extra_filters = []
- for filters in default_filters.values():
- for col, val in filters.items():
- if col not in immune_fields:
- extra_filters.append({"col": col, "op": "in", "val": val})
+ layout = json.loads(dashboard.position_json or "{}")
+ # do not apply filters if chart is immune to them
Review comment:
Nit. I would nix the comment as the following function has not reference to immunity.
----------------------------------------------------------------
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] etr2460 commented on a change in pull request
#9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393812392
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ slc = session.query(Slice).filter_by(id=slice_id).one_or_none()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slc not in dashboard.slices
+ ):
+ return []
+
+ # is this dashboard has default filters?
+ json_metadata = json.loads(dashboard.json_metadata)
+ default_filters = json.loads(json_metadata.get("default_filters", "null"))
+ if not default_filters:
+ return []
+
+ # is default filters applicable to the given slice?
+ filter_scopes = json_metadata.get("filter_scopes", {})
+ layout = json.loads(dashboard.position_json or "{}")
+
+ return build_extra_filters(layout, filter_scopes, default_filters, slice_id)
+
+
+def build_extra_filters(
+ layout: Dict,
+ filter_scopes: Dict,
+ default_filters: Dict[str, Dict[str, List]],
+ slice_id: int,
+) -> List[Dict[str, Any]]:
+ extra_filters = []
+
+ # do not apply filters if chart if chart not in filter's scope or
+ # chart is immune to the filter
+ for filter_id, columns in default_filters.items():
+ scopes_by_filter_field = filter_scopes.get(filter_id, {})
+ for col, val in columns.items():
+ current_field_scopes = scopes_by_filter_field.get(col, {})
+ # scope is list of container ids
+ scope = current_field_scopes.get("scope", ["ROOT_ID"])
+ # immune is list of slice ids
+ immune = current_field_scopes.get("immune", [])
+
+ for container_id in scope:
+ if slice_id not in immune and is_slice_in_container(
+ layout, container_id, slice_id
+ ):
+ extra_filters.append({"col": col, "op": "in", "val": val})
+
+ return extra_filters
+
+
+def is_slice_in_container(layout: Dict, container_id: str, slice_id: int) -> bool:
+ if container_id == "ROOT_ID":
+ return True
+
+ node = layout[container_id]
+ node_type = node.get("type")
+ if (
+ node_type == "CHART"
+ and node.get("meta")
+ and node.get("meta").get("chartId", 0) == slice_id
+ ):
+ return True
+
+ if node_type in CONTAINER_TYPES:
+ children = node.get("children", [])
+ if children:
Review comment:
this should be unnecessary, if you're concerned that children could be `None` or `0`, then i'd recommend making the line above `children = node.get("children") or []`
----------------------------------------------------------------
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] john-bodley commented on a change in pull
request #9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r394102501
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,90 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+# see all dashboard components type in
+# /superset-frontend/src/dashboard/util/componentTypes.js
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slice_id not in [slc.id for slc in dashboard.slices]
Review comment:
This is probably preferred as it should short circuit,
```python
or not any([slc for slc in dashboard.slices if slc.id == slice_id])
```
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9311: [cache warm_up] warm_up slice with dashboard
default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393986350
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ slc = session.query(Slice).filter_by(id=slice_id).one_or_none()
Review comment:
fixed.
----------------------------------------------------------------
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 #9311: [cache
warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#issuecomment-599720147
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9311?src=pr&el=h1) Report
> Merging [#9311](https://codecov.io/gh/apache/incubator-superset/pull/9311?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/b1916a190e12a4da4b9afaca414ed8bd59a024c8&el=desc) will **increase** coverage by `0.00%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9311?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9311 +/- ##
=======================================
Coverage 59.08% 59.08%
=======================================
Files 374 374
Lines 12202 12205 +3
Branches 2986 2989 +3
=======================================
+ Hits 7209 7211 +2
- Misses 4814 4815 +1
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9311?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...plore/components/controls/FixedOrMetricControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9311/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaXhlZE9yTWV0cmljQ29udHJvbC5qc3g=) | `51.78% <0.00%> (+0.84%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9311?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/9311?src=pr&el=footer). Last update [b1916a1...06b1d96](https://codecov.io/gh/apache/incubator-superset/pull/9311?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
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393805638
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ slc = session.query(Slice).filter_by(id=slice_id).one_or_none()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slc not in dashboard.slices
+ ):
+ return []
+
+ # is this dashboard has default filters?
+ json_metadata = json.loads(dashboard.json_metadata)
+ default_filters = json.loads(json_metadata.get("default_filters", "null"))
+ if not default_filters:
+ return []
+
+ # is default filters applicable to the given slice?
+ filter_scopes = json_metadata.get("filter_scopes", {})
+ layout = json.loads(dashboard.position_json or "{}")
+
+ return build_extra_filters(layout, filter_scopes, default_filters, slice_id)
+
+
+def build_extra_filters(
+ layout: Dict,
+ filter_scopes: Dict,
+ default_filters: Dict[str, Dict[str, List]],
+ slice_id: int,
+) -> List[Dict[str, Any]]:
+ extra_filters = []
+
+ # do not apply filters if chart if chart not in filter's scope or
Review comment:
looks like `if chart` is repeated accidentally here
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9311: [cache warm_up] warm_up slice with dashboard
default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393990454
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,80 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+# see all dashboard components type in
+# /superset-frontend/src/dashboard/util/componentTypes.js
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slice_id not in [slc.id for slc in dashboard.slices]
+ ):
+ return []
+
+ # does this dashboard have default filters?
+ json_metadata = json.loads(dashboard.json_metadata)
+ default_filters = json.loads(json_metadata.get("default_filters", "null"))
+ if not default_filters:
+ return []
+
+ # are default filters applicable to the given slice?
+ filter_scopes = json_metadata.get("filter_scopes", {})
+ layout = json.loads(dashboard.position_json or "{}")
+
+ return build_extra_filters(layout, filter_scopes, default_filters, slice_id)
+
+
+def build_extra_filters(
+ layout: Dict,
+ filter_scopes: Dict,
+ default_filters: Dict[str, Dict[str, List]],
+ slice_id: int,
+) -> List[Dict[str, Any]]:
+ extra_filters = []
+
+ # do not apply filters if chart is not in filter's scope or
+ # chart is immune to the filter
+ for filter_id, columns in default_filters.items():
+ scopes_by_filter_field = filter_scopes.get(filter_id, {})
+ for col, val in columns.items():
+ current_field_scopes = scopes_by_filter_field.get(col, {})
+ scoped_container_ids = current_field_scopes.get("scope", ["ROOT_ID"])
Review comment:
at this place, `current_field_scopes.get("scope")` the returned value could be [], which means this filter currently didn't apply to any tab. If i use `current_field_scopes.get("scope") or ["ROOT_ID"]`
the returned value will become `["ROOT_ID"]` (which means globally apply)
----------------------------------------------------------------
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] etr2460 commented on a change in pull request
#9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393805236
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ slc = session.query(Slice).filter_by(id=slice_id).one_or_none()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slc not in dashboard.slices
+ ):
+ return []
+
+ # is this dashboard has default filters?
+ json_metadata = json.loads(dashboard.json_metadata)
+ default_filters = json.loads(json_metadata.get("default_filters", "null"))
+ if not default_filters:
+ return []
+
+ # is default filters applicable to the given slice?
Review comment:
sp nit: `are default filters applicable to the given slice?`
----------------------------------------------------------------
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] graceguo-supercat commented on issue #9311:
[cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on issue #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#issuecomment-600319778
> The main difference here applies if it's possible for a key to be set to a falsey value. If you have the dict:
>
> ```python
> d = {
> "key": None,
> "foo": False,
> }
> ```
>
> then:
>
> ```python
> d.get("key", []) # equals None
> d.get("foo", {}) # equals False
> d.get("key") or [] # equals []
> d.get("foo") or {} # equals {}
> ```
>
> This is really important to get right if you try to iterate through an array after "getting" it like this, as iterating over False or None will throw
Correct. If the key exists, I should use false value, instead of assign a default value. See my example above.
----------------------------------------------------------------
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] etr2460 commented on issue #9311: [cache
warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#issuecomment-600329765
Here's a specific example:
You have `filter_scopes = json_metadata.get("filter_scopes", {})` in the code, and then you later assume `filter_scopes` is a Dict. However, because the json_metadata can be edited by the user, it could be `None` or `False`. This would break with your current code, but not with `json_metadata.get("filter_scopes") or {}`
In general, I think we need to be a lot more defensive with parsing the json_metadata blob here, because an invalid metadata field isn't validated on save and it would break the code here
----------------------------------------------------------------
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] etr2460 edited a comment on issue #9311:
[cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
etr2460 edited a comment on issue #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#issuecomment-600318023
The main difference here applies if it's possible for a key to be set to a falsey value. If you have the dict:
```python
d = {
"key": None,
"foo": False,
}
```
then:
```python
d.get("key", []) # equals None
d.get("foo", {}) # equals False
d.get("key") or [] # equals []
d.get("foo") or {} # equals {}
```
This is really important to get right if you try to iterate through an array after "getting" it like this, as iterating over False or None will throw
----------------------------------------------------------------
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] etr2460 commented on a change in pull request
#9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393800881
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
Review comment:
do we need to keep this in sync with a list on the client side? Maybe add a comment here to reference the client file if so
----------------------------------------------------------------
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] etr2460 commented on a change in pull request
#9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393810831
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ slc = session.query(Slice).filter_by(id=slice_id).one_or_none()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slc not in dashboard.slices
+ ):
+ return []
+
+ # is this dashboard has default filters?
+ json_metadata = json.loads(dashboard.json_metadata)
+ default_filters = json.loads(json_metadata.get("default_filters", "null"))
+ if not default_filters:
+ return []
+
+ # is default filters applicable to the given slice?
+ filter_scopes = json_metadata.get("filter_scopes", {})
+ layout = json.loads(dashboard.position_json or "{}")
+
+ return build_extra_filters(layout, filter_scopes, default_filters, slice_id)
+
+
+def build_extra_filters(
+ layout: Dict,
+ filter_scopes: Dict,
+ default_filters: Dict[str, Dict[str, List]],
+ slice_id: int,
+) -> List[Dict[str, Any]]:
+ extra_filters = []
+
+ # do not apply filters if chart if chart not in filter's scope or
+ # chart is immune to the filter
+ for filter_id, columns in default_filters.items():
+ scopes_by_filter_field = filter_scopes.get(filter_id, {})
+ for col, val in columns.items():
+ current_field_scopes = scopes_by_filter_field.get(col, {})
+ # scope is list of container ids
+ scope = current_field_scopes.get("scope", ["ROOT_ID"])
+ # immune is list of slice ids
+ immune = current_field_scopes.get("immune", [])
Review comment:
instead of using comments to describe these variable names, let's name them to something like `scoped_container_ids` and `immune_slice_ids`
----------------------------------------------------------------
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] etr2460 commented on a change in pull request
#9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393811243
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ slc = session.query(Slice).filter_by(id=slice_id).one_or_none()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slc not in dashboard.slices
+ ):
+ return []
+
+ # is this dashboard has default filters?
+ json_metadata = json.loads(dashboard.json_metadata)
+ default_filters = json.loads(json_metadata.get("default_filters", "null"))
+ if not default_filters:
+ return []
+
+ # is default filters applicable to the given slice?
+ filter_scopes = json_metadata.get("filter_scopes", {})
+ layout = json.loads(dashboard.position_json or "{}")
+
+ return build_extra_filters(layout, filter_scopes, default_filters, slice_id)
+
+
+def build_extra_filters(
+ layout: Dict,
+ filter_scopes: Dict,
+ default_filters: Dict[str, Dict[str, List]],
+ slice_id: int,
+) -> List[Dict[str, Any]]:
+ extra_filters = []
+
+ # do not apply filters if chart if chart not in filter's scope or
+ # chart is immune to the filter
+ for filter_id, columns in default_filters.items():
+ scopes_by_filter_field = filter_scopes.get(filter_id, {})
+ for col, val in columns.items():
+ current_field_scopes = scopes_by_filter_field.get(col, {})
+ # scope is list of container ids
+ scope = current_field_scopes.get("scope", ["ROOT_ID"])
+ # immune is list of slice ids
+ immune = current_field_scopes.get("immune", [])
+
+ for container_id in scope:
+ if slice_id not in immune and is_slice_in_container(
+ layout, container_id, slice_id
+ ):
+ extra_filters.append({"col": col, "op": "in", "val": val})
+
+ return extra_filters
+
+
+def is_slice_in_container(layout: Dict, container_id: str, slice_id: int) -> bool:
+ if container_id == "ROOT_ID":
+ return True
+
+ node = layout[container_id]
+ node_type = node.get("type")
+ if (
+ node_type == "CHART"
+ and node.get("meta")
+ and node.get("meta").get("chartId", 0) == slice_id
Review comment:
you don't need the default value of 0 for `get` here because `None` will never be equal to `slice_id`
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9311: [cache warm_up] warm_up slice with dashboard
default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393986461
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ slc = session.query(Slice).filter_by(id=slice_id).one_or_none()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slc not in dashboard.slices
+ ):
+ return []
+
+ # is this dashboard has default filters?
+ json_metadata = json.loads(dashboard.json_metadata)
+ default_filters = json.loads(json_metadata.get("default_filters", "null"))
+ if not default_filters:
+ return []
+
+ # is default filters applicable to the given slice?
Review comment:
fixed.
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9311: [cache warm_up] warm_up slice with dashboard
default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393986195
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ slc = session.query(Slice).filter_by(id=slice_id).one_or_none()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slc not in dashboard.slices
+ ):
+ return []
+
+ # is this dashboard has default filters?
Review comment:
fixed.
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9311: [cache warm_up] warm_up slice with dashboard
default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393258452
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
Review comment:
Celery task is here: https://github.com/apache/incubator-superset/blob/000a038af193f2770dfe23a053bd1ff5e9e72bd4/superset/tasks/cache.py#L39
It missed a logic to check default_filters's scope if the scope is not globally, but pretty close. So my plan is to add logic for warm_up endpoint, then update Celery task accordingly in another PR.
----------------------------------------------------------------
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] graceguo-supercat commented on a change in
pull request #9311: [cache warm_up] warm_up slice with dashboard
default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393990454
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,80 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+# see all dashboard components type in
+# /superset-frontend/src/dashboard/util/componentTypes.js
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slice_id not in [slc.id for slc in dashboard.slices]
+ ):
+ return []
+
+ # does this dashboard have default filters?
+ json_metadata = json.loads(dashboard.json_metadata)
+ default_filters = json.loads(json_metadata.get("default_filters", "null"))
+ if not default_filters:
+ return []
+
+ # are default filters applicable to the given slice?
+ filter_scopes = json_metadata.get("filter_scopes", {})
+ layout = json.loads(dashboard.position_json or "{}")
+
+ return build_extra_filters(layout, filter_scopes, default_filters, slice_id)
+
+
+def build_extra_filters(
+ layout: Dict,
+ filter_scopes: Dict,
+ default_filters: Dict[str, Dict[str, List]],
+ slice_id: int,
+) -> List[Dict[str, Any]]:
+ extra_filters = []
+
+ # do not apply filters if chart is not in filter's scope or
+ # chart is immune to the filter
+ for filter_id, columns in default_filters.items():
+ scopes_by_filter_field = filter_scopes.get(filter_id, {})
+ for col, val in columns.items():
+ current_field_scopes = scopes_by_filter_field.get(col, {})
+ scoped_container_ids = current_field_scopes.get("scope", ["ROOT_ID"])
Review comment:
at this place, current_field_scopes.get("scope"), the returned value could be [], which means this filter currently didn't apply to any tab. If i use `current_field_scopes.get("scope") or ["ROOT_ID"]`
the returned value will become `["ROOT_ID"]` (which means globally apply)
----------------------------------------------------------------
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] etr2460 commented on a change in pull request
#9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393805096
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ slc = session.query(Slice).filter_by(id=slice_id).one_or_none()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slc not in dashboard.slices
+ ):
+ return []
+
+ # is this dashboard has default filters?
Review comment:
sp nit: `does this dashboard have default 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] graceguo-supercat commented on a change in
pull request #9311: [cache warm_up] warm_up slice with dashboard
default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393266748
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
Review comment:
oh never mind, the Celery warmup change is very simple. So i just commit Celery warmup changes here. And all unit tests can be re-used without any change!
----------------------------------------------------------------
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 edited a comment on issue #9311:
[cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#issuecomment-599720147
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9311?src=pr&el=h1) Report
> Merging [#9311](https://codecov.io/gh/apache/incubator-superset/pull/9311?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/b1916a190e12a4da4b9afaca414ed8bd59a024c8?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9311?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9311 +/- ##
==========================================
+ Coverage 59.08% 59.08% +<.01%
==========================================
Files 374 374
Lines 12202 12205 +3
Branches 2986 2989 +3
==========================================
+ Hits 7209 7211 +2
- Misses 4814 4815 +1
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9311?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...plore/components/controls/FixedOrMetricControl.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9311/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9GaXhlZE9yTWV0cmljQ29udHJvbC5qc3g=) | `51.78% <0%> (+0.84%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9311?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/9311?src=pr&el=footer). Last update [b1916a1...7de2aed](https://codecov.io/gh/apache/incubator-superset/pull/9311?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
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393804308
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ slc = session.query(Slice).filter_by(id=slice_id).one_or_none()
Review comment:
seems like you don't actually need `slc` here, instead you could look for a slice in `dashboard.slices` based on the `slice_id` passed into the function. That would save a db call
----------------------------------------------------------------
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] etr2460 commented on a change in pull request
#9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r394057964
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,88 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+# see all dashboard components type in
+# /superset-frontend/src/dashboard/util/componentTypes.js
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slice_id not in [slc.id for slc in dashboard.slices]
+ ):
+ return []
+
+ try:
+ # does this dashboard have default filters?
+ json_metadata = json.loads(dashboard.json_metadata)
+ default_filters = json.loads(json_metadata.get("default_filters", "null"))
+ if not default_filters:
+ return []
+
+ # are default filters applicable to the given slice?
+ filter_scopes = json_metadata.get("filter_scopes", {})
+ layout = json.loads(dashboard.position_json or "{}")
+
+ return build_extra_filters(layout, filter_scopes, default_filters, slice_id)
+ except json.JSONDecodeError:
+ return []
+
+
+def build_extra_filters(
+ layout: Dict,
+ filter_scopes: Dict,
+ default_filters: Dict[str, Dict[str, List]],
+ slice_id: int,
+) -> List[Dict[str, Any]]:
Review comment:
The typecheck here isn't quite right because you've already assumed the type is correct in the function statement. I'd recommend moving the type check into the parent function so that the types are correct here
----------------------------------------------------------------
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] serenajiang commented on a change in pull
request #9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
serenajiang commented on a change in pull request #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#discussion_r393862489
##########
File path: superset/views/utils.py
##########
@@ -262,3 +263,89 @@ def get_time_range_endpoints(
return (TimeRangeEndpoint(start), TimeRangeEndpoint(end))
return (TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE)
+
+
+CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+
+
+def get_dashboard_extra_filters(
+ slice_id: int, dashboard_id: int
+) -> List[Dict[str, Any]]:
+ session = db.session()
+ slc = session.query(Slice).filter_by(id=slice_id).one_or_none()
+ dashboard = session.query(Dashboard).filter_by(id=dashboard_id).one_or_none()
+
+ # is chart in this dashboard?
+ if (
+ dashboard is None
+ or not dashboard.json_metadata
+ or not dashboard.slices
+ or slc not in dashboard.slices
+ ):
+ return []
+
+ # is this dashboard has default filters?
+ json_metadata = json.loads(dashboard.json_metadata)
+ default_filters = json.loads(json_metadata.get("default_filters", "null"))
+ if not default_filters:
+ return []
+
+ # is default filters applicable to the given slice?
+ filter_scopes = json_metadata.get("filter_scopes", {})
+ layout = json.loads(dashboard.position_json or "{}")
+
+ return build_extra_filters(layout, filter_scopes, default_filters, slice_id)
+
+
+def build_extra_filters(
+ layout: Dict,
+ filter_scopes: Dict,
+ default_filters: Dict[str, Dict[str, List]],
+ slice_id: int,
+) -> List[Dict[str, Any]]:
+ extra_filters = []
+
+ # do not apply filters if chart if chart not in filter's scope or
+ # chart is immune to the filter
+ for filter_id, columns in default_filters.items():
+ scopes_by_filter_field = filter_scopes.get(filter_id, {})
+ for col, val in columns.items():
+ current_field_scopes = scopes_by_filter_field.get(col, {})
+ # scope is list of container ids
+ scope = current_field_scopes.get("scope", ["ROOT_ID"])
+ # immune is list of slice ids
+ immune = current_field_scopes.get("immune", [])
+
+ for container_id in scope:
+ if slice_id not in immune and is_slice_in_container(
+ layout, container_id, slice_id
+ ):
+ extra_filters.append({"col": col, "op": "in", "val": val})
+
+ return extra_filters
+
+
+def is_slice_in_container(layout: Dict, container_id: str, slice_id: int) -> bool:
+ if container_id == "ROOT_ID":
+ return True
+
+ node = layout[container_id]
+ node_type = node.get("type")
+ if (
+ node_type == "CHART"
+ and node.get("meta")
+ and node.get("meta").get("chartId", 0) == slice_id
+ ):
+ return True
+
+ if node_type in CONTAINER_TYPES:
+ children = node.get("children", [])
+ if children:
+ # for child_id in children_ids:
+ if any(
Review comment:
Since this is within the last if-statement, I think you can just write `return any(...)` instead of `if any(...): return True`.
----------------------------------------------------------------
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] graceguo-supercat edited a comment on issue
#9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on issue #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#issuecomment-600319778
> The main difference here applies if it's possible for a key to be set to a falsey value. If you have the dict:
>
> ```python
> d = {
> "key": None,
> "foo": False,
> }
> ```
>
> then:
>
> ```python
> d.get("key", []) # equals None
> d.get("foo", {}) # equals False
> d.get("key") or [] # equals []
> d.get("foo") or {} # equals {}
> ```
>
> This is really important to get right if you try to iterate through an array after "getting" it like this, as iterating over False or None will throw
Correct. If the key exists, I should use falsy value, instead of assign a default value. See my example above.
----------------------------------------------------------------
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] graceguo-supercat edited a comment on issue
#9311: [cache warm_up] warm_up slice with dashboard default_filters
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on issue #9311: [cache warm_up] warm_up slice with dashboard default_filters
URL: https://github.com/apache/incubator-superset/pull/9311#issuecomment-600327324
> What i'm saying is if you default it to `[]` with or then you don't even need to check if it's falsey. The empty array will simply bypass the for loop
My question is this comment:
`after looking through this and not knowing potential values for the filter object, i'm wondering if most of the .get("value", {}) and .get("value", []) statements should be .get("value") or {} and .get("value") or []`
Why should change most of `dict.get(key, default_value)` to `dict.get(key) or default_value`?
----------------------------------------------------------------
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