sadpandajoe commented on code in PR #43225:
URL: https://github.com/apache/superset/pull/43225#discussion_r3994257594
##########
superset/charts/schemas.py:
##########
@@ -611,17 +613,18 @@ class
ChartDataRollingOptionsSchema(ChartDataPostProcessingOperationOptionsSchem
Rolling operation config.
"""
- columns = (
- fields.Dict(
- metadata={
- "description": "columns on which to perform rolling, mapping
source "
- "column to target column. For instance, `{'y': 'y'}` will
replace the "
- "column `y` with the rolling value in `y`, while `{'y': 'y2'}`
will add " # noqa: E501
- "a column `y2` based on rolling values calculated from `y`,
leaving the " # noqa: E501
- "original column `y` unchanged.",
- "example": {"weekly_rolling_sales": "sales"},
- },
- ),
+ columns = fields.Dict(
+ metadata={
+ "description": "columns on which to perform rolling, mapping
source "
+ "column to target column. For instance, `{'y': 'y'}` will replace
the "
+ "column `y` with the rolling value in `y`, while `{'y': 'y2'}`
will add "
+ "a column `y2` based on rolling values calculated from `y`,
leaving the "
+ "original column `y` unchanged.",
+ "example": {"weekly_rolling_sales": "sales"},
Review Comment:
This example reverses the source→target mapping described above. A client
with a `sales` column that copies it asks `rolling()` to read a nonexistent
`weekly_rolling_sales` source and is rejected before the new column can be
created; should this be `{"sales": "weekly_rolling_sales"}`?
##########
superset/charts/schemas.py:
##########
@@ -568,17 +570,18 @@ class
ChartDataRollingOptionsSchema(ChartDataPostProcessingOperationOptionsSchem
Rolling operation config.
"""
- columns = (
- fields.Dict(
- metadata={
- "description": "columns on which to perform rolling, mapping
source "
- "column to target column. For instance, `{'y': 'y'}` will
replace the "
- "column `y` with the rolling value in `y`, while `{'y': 'y2'}`
will add " # noqa: E501
- "a column `y2` based on rolling values calculated from `y`,
leaving the " # noqa: E501
- "original column `y` unchanged.",
- "example": {"weekly_rolling_sales": "sales"},
- },
- ),
+ columns = fields.Dict(
Review Comment:
The separate fix is still open, so merging this head first would publish an
object in OpenAPI while the exported client type still says `string[]`. Can
#43589 merge first, or can this PR include the type change, so the two public
contracts cannot ship out of order?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
