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/07/10 12:02:12 UTC
[GitHub] [incubator-superset] villebro opened a new pull request #10286: feat: add contribution operation and fix cache_key bug
villebro opened a new pull request #10286:
URL: https://github.com/apache/incubator-superset/pull/10286
### SUMMARY
This PR adds a contribution operation to `QueryObject` post processing operations and fixes a bug that ignored post processing operations in cache key.
### TEST PLAN
New tests + CI
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request #10286: feat: add contribution operation and fix cache_key bug
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10286:
URL: https://github.com/apache/incubator-superset/pull/10286#discussion_r452865347
##########
File path: superset/common/query_object.py
##########
@@ -94,7 +94,7 @@ def __init__(
extras: Optional[Dict[str, Any]] = None,
columns: Optional[List[str]] = None,
orderby: Optional[List[List[str]]] = None,
- post_processing: Optional[List[Dict[str, Any]]] = None,
+ post_processing: Optional[List[Optional[Dict[str, Any]]]] = None,
Review comment:
Actually this is an intermediate structure; the `Optional`s are removed in the constructor, resulting in `List[Dict[str, Any]]]`. But you're right, this is becoming difficult to follow, will think about a good solution to adding types 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
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request #10286: feat: add contribution operation and fix cache_key bug
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10286:
URL: https://github.com/apache/incubator-superset/pull/10286#discussion_r452946333
##########
File path: superset/charts/schemas.py
##########
@@ -395,6 +395,19 @@ class ChartDataSortOptionsSchema(ChartDataPostProcessingOperationOptionsSchema):
aggregates = ChartDataAggregateConfigField()
+class ChartDataContributionOptionsSchema(ChartDataPostProcessingOperationOptionsSchema):
+ """
+ Contribution operation config.
+ """
+
+ orientation = fields.String(
+ description="Should cell values be calculated across the row or column.",
+ required=True,
+ validate=validate.OneOf(choices=("row", "column",)),
Review comment:
Yeah, with the introduction of marshmallow3, I think we can do a bunch of cleanup work on these. I'm going to look into cleaning these up next week.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request #10286: feat: add contribution operation and fix cache_key bug
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10286:
URL: https://github.com/apache/incubator-superset/pull/10286#discussion_r452949522
##########
File path: superset/utils/pandas_postprocessing.py
##########
@@ -517,3 +518,29 @@ def _parse_location(location: str) -> Tuple[float, float, float]:
return _append_columns(df, geodetic_df, columns)
except ValueError:
raise QueryObjectValidationError(_("Invalid geodetic string"))
+
+
+def contribution(
+ df: DataFrame, orientation: PostProcessingContributionOrientation
+) -> DataFrame:
+ """
+ Calculate cell contibution to row/column total.
+
+ :param df: DataFrame containing all-numeric data (temporal column ignored)
+ :param orientation: calculate by dividing cell with row/column total
+ :return: DataFrame with contributions, with temporal column at beginning if present
+ """
+ temporal_series: Optional[Series] = None
+ contribution_df = df.copy()
+ if DTTM_ALIAS in df.columns:
+ temporal_series = cast(Series, contribution_df.pop(DTTM_ALIAS))
+
+ if orientation == PostProcessingContributionOrientation.ROW:
+ contribution_dft = contribution_df.T
+ contribution_df = (contribution_dft / contribution_dft.sum()).T
+ else:
+ contribution_df = contribution_df / contribution_df.sum()
Review comment:
I was just lazy and more or less copied what was in `viz.py` 😛
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on a change in pull request #10286: feat: add contribution operation and fix cache_key bug
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10286:
URL: https://github.com/apache/incubator-superset/pull/10286#discussion_r452922238
##########
File path: superset/charts/schemas.py
##########
@@ -395,6 +395,19 @@ class ChartDataSortOptionsSchema(ChartDataPostProcessingOperationOptionsSchema):
aggregates = ChartDataAggregateConfigField()
+class ChartDataContributionOptionsSchema(ChartDataPostProcessingOperationOptionsSchema):
+ """
+ Contribution operation config.
+ """
+
+ orientation = fields.String(
+ description="Should cell values be calculated across the row or column.",
+ required=True,
+ validate=validate.OneOf(choices=("row", "column",)),
Review comment:
Can these two also based on the enum?
##########
File path: superset/utils/pandas_postprocessing.py
##########
@@ -517,3 +518,29 @@ def _parse_location(location: str) -> Tuple[float, float, float]:
return _append_columns(df, geodetic_df, columns)
except ValueError:
raise QueryObjectValidationError(_("Invalid geodetic string"))
+
+
+def contribution(
+ df: DataFrame, orientation: PostProcessingContributionOrientation
+) -> DataFrame:
+ """
+ Calculate cell contibution to row/column total.
+
+ :param df: DataFrame containing all-numeric data (temporal column ignored)
+ :param orientation: calculate by dividing cell with row/column total
+ :return: DataFrame with contributions, with temporal column at beginning if present
+ """
+ temporal_series: Optional[Series] = None
+ contribution_df = df.copy()
+ if DTTM_ALIAS in df.columns:
+ temporal_series = cast(Series, contribution_df.pop(DTTM_ALIAS))
+
+ if orientation == PostProcessingContributionOrientation.ROW:
+ contribution_dft = contribution_df.T
+ contribution_df = (contribution_dft / contribution_dft.sum()).T
+ else:
+ contribution_df = contribution_df / contribution_df.sum()
Review comment:
Could probably utilize "axis" parameter for `sum`
##########
File path: superset/utils/pandas_postprocessing.py
##########
@@ -517,3 +518,29 @@ def _parse_location(location: str) -> Tuple[float, float, float]:
return _append_columns(df, geodetic_df, columns)
except ValueError:
raise QueryObjectValidationError(_("Invalid geodetic string"))
+
+
+def contribution(
+ df: DataFrame, orientation: PostProcessingContributionOrientation
+) -> DataFrame:
Review comment:
Maybe it makes more things to keep things simple, but is it possible to allow specifying which column(s) to compute on? I could imagine this operator being used to support "percent metrics" in table charts and it’ll need the ability to select specific columns.
Maybe the API can be something like this
```
{
# compute row wise contribution and override the columns
"columns": ["foo", "bar"],
"orientation": "row"
}
{
# compute column wise computation and insert new columns
"columns": ["pct_abc"],
"sources": ["abc"]
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on a change in pull request #10286: feat: add contribution operation and fix cache_key bug
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #10286:
URL: https://github.com/apache/incubator-superset/pull/10286#discussion_r452949213
##########
File path: superset/utils/pandas_postprocessing.py
##########
@@ -517,3 +518,29 @@ def _parse_location(location: str) -> Tuple[float, float, float]:
return _append_columns(df, geodetic_df, columns)
except ValueError:
raise QueryObjectValidationError(_("Invalid geodetic string"))
+
+
+def contribution(
+ df: DataFrame, orientation: PostProcessingContributionOrientation
+) -> DataFrame:
Review comment:
The thought did occur to me. In this iteration I wanted to add functionality to get feature parity with `viz.py`, but I look forward to adding this optionality later. One significant feature that's currently missing is being able to get feedback from the operation on what happened, e.g. for pivots getting a list of all newly created columns. I have a basic idea of what this could look like, but didn't want to get into that before I actually need it.
FYI, this feature was added to create a line chart with similar features as the legacy one on `viz.py`. In this case I'm chaining two operations together: first a pivot, then a contribution if that option is selected. As I currently don't know what come out of the pivot, I'm unable to tell the contribution operation which columns it should be processing on. Hence I'm just assuming that we're calculating on all columns except the temporal one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] ktmud commented on a change in pull request #10286: feat: add contribution operation and fix cache_key bug
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10286:
URL: https://github.com/apache/incubator-superset/pull/10286#discussion_r452932892
##########
File path: superset/utils/pandas_postprocessing.py
##########
@@ -517,3 +518,29 @@ def _parse_location(location: str) -> Tuple[float, float, float]:
return _append_columns(df, geodetic_df, columns)
except ValueError:
raise QueryObjectValidationError(_("Invalid geodetic string"))
+
+
+def contribution(
+ df: DataFrame, orientation: PostProcessingContributionOrientation
+) -> DataFrame:
Review comment:
Maybe it makes more sense to keep things simple, but is it possible to allow specifying which column(s) to compute on? I could imagine this operator being used to support "percent metrics" in table charts and it’ll need the ability to select specific columns.
Maybe the API can be something like this
```
{
# compute row wise contribution and override the columns
"columns": ["foo", "bar"],
"orientation": "row"
}
{
# compute column wise computation and insert new columns
"columns": ["pct_abc"],
"sources": ["abc"]
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #10286: feat: add contribution operation and fix cache_key bug
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #10286:
URL: https://github.com/apache/incubator-superset/pull/10286#discussion_r452858402
##########
File path: superset/common/query_object.py
##########
@@ -94,7 +94,7 @@ def __init__(
extras: Optional[Dict[str, Any]] = None,
columns: Optional[List[str]] = None,
orderby: Optional[List[List[str]]] = None,
- post_processing: Optional[List[Dict[str, Any]]] = None,
+ post_processing: Optional[List[Optional[Dict[str, Any]]]] = None,
Review comment:
This is starting to deserve a type definition of it's own
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro merged pull request #10286: feat: add contribution operation and fix cache_key bug
Posted by GitBox <gi...@apache.org>.
villebro merged pull request #10286:
URL: https://github.com/apache/incubator-superset/pull/10286
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org