SEPURI-SAI-KRISHNA commented on code in PR #43225:
URL: https://github.com/apache/superset/pull/43225#discussion_r3872548472
##########
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:
Thanks both, agreed with @aminghadersohi's read, and with keeping it out of
this PR.
I'd reached the same conclusion independently: the OpenAPI object is correct
and the exported TypeScript type is the stale side. Two details that I think
settle it beyond the signature trace:
Every assertion already in the repo expects an object, so the mapping isn't
a new contract, it's the one that was always in force:
rollingWindowOperator.test.ts columns: { 'count(*)': 'count(*)',
... }
MixedTimeseries/buildQuery.test.ts columns: { count: 'count', ... }
The only `string[]` anywhere is the `ROLLING_RULE` fixture in
`PostProcessing.test.ts`, which uses one solely because the type asks for it.
And the reason a contradiction this direct never failed the build:
`columns.map(col => [col, col])` widens each pair to `(string | undefined)[]`
rather than a tuple, which selects the untyped `Object.fromEntries` overload —
```ts
fromEntries<T = any>(entries: Iterable<readonly [PropertyKey, T]>): { [k:
string]: T };
fromEntries(entries: Iterable<readonly any[]>): any; // <- selected
```
so `columnsMap` is `any` and assigns to `string[]` silently. Forcing the
tuple surfaces the real error:
error TS2322: Type 'string[]' is not assignable to type 'Record<string,
string>'.
Index signature for type 'string' is missing in type 'string[]'.
So this PR stays Python-only as it stands, and I'll send the frontend fix
separately. I'll include `diff` alongside `rolling` and `cum`, `diff()` takes
`columns: dict[str, str]` and does `df[columns.keys()]` too, so
`_PostProcessingDiff` carries the identical mismatch even though no operator
emits it today.
@sadpandajoe the two earlier threads are addressed at this head as well,
whenever you get a chance to look.
--
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]
