SEPURI-SAI-KRISHNA commented on code in PR #43225:
URL: https://github.com/apache/superset/pull/43225#discussion_r3861131513
##########
superset/charts/schemas.py:
##########
@@ -528,17 +529,15 @@ 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:
Agreed, and done, `columns` is now marked required.
`rolling()` takes `columns` positionally with no default, exactly as it
takes `rolling_type`, which this schema already marked required. Leaving it
optional let a spec-following client omit it and reach the untyped options
dispatch, as you describe.
From the generated OpenAPI:
Rolling.columns: required=True
One knock-on worth flagging:
`test_chart_data_rolling_options_schema_window_range` is a pre-existing test
whose base payload omitted `columns`, so it began failing with `{'columns':
['Missing data for required field.']}`. I added `"columns": {"y": "y"}` to that
payload rather than relaxing the constraint.
--
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]
