aminghadersohi commented on code in PR #43225:
URL: https://github.com/apache/superset/pull/43225#discussion_r3871782745
##########
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:
Good catch — and I think the object (OpenAPI) side is the correct one here;
it's the exported TS type that's stale, and it was already inconsistent before
this PR surfaced it.
Tracing all three sides at this head:
- **Runtime `rolling()`** takes `columns: dict[str, str]`
(`superset/utils/pandas_postprocessing/rolling.py:40`) and does `df.loc[:,
columns.keys()]` (`:69`). An array reaching it hits `list.keys()` →
`AttributeError`, exactly as you describe.
- **What the frontend actually emits:** `rollingWindowOperator.ts:48` builds
`const columnsMap = Object.fromEntries(columns.map(col => [col, col]))` and
sends `columns: columnsMap` (`:71`, and `:55` for `cum`) — a string→string
object, not a `string[]`. So Superset's own operator already produces the
mapping the OpenAPI object type describes.
- **The exported type** `_PostProcessingRolling.options.columns: string[]`
(`superset-ui-core/src/query/types/PostProcessing.ts:154`) is therefore the
stale side — it contradicts the operator that feeds it. `_PostProcessingCum`
(`:164`) has the same `string[]` while the operator emits `columnsMap` for
`cum` too.
This PR didn't introduce the divergence: `columns` was always `fields.Dict`;
the stray-tuple bug had dropped it from the published spec entirely, so the
conflict was invisible until the field was restored. The OpenAPI contract now
matches both `rolling()`'s signature and the operator's real output.
So I don't think anything needs to change in this Python-only PR — the
correct follow-up is a separate frontend change fixing `PostProcessingRolling`
(and `PostProcessingCum`) to `Record<string, string>`. Leaving that call to you
and the author.
--
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]
