WhoamiI00 commented on code in PR #43113:
URL: https://github.com/apache/superset/pull/43113#discussion_r3873382783
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -593,6 +594,20 @@ export default function transformProps(
const array = ensureIsArray(chartProps.rawFormData?.time_compare);
const inverted = invert(verboseMap);
+ // A Percentage time comparison replaces the derived series' values with a
+ // ratio, so that row is no longer in the source metric's units and must not
+ // inherit its currency/D3 format. `renameOperator` labels those series with
+ // the offset alone, or `<metric>, <offset>` when several metrics are
plotted.
+ const percentageComparisonSeries = new Set<string>(
+ chartProps.rawFormData?.comparison_type === ComparisonType.Percentage
+ ? array.flatMap(offset =>
+ rawValueMetricLabels.length > 1
Review Comment:
Good catch, and confirmed — the grouped case was still broken. Fixed in
c3d550a5.
Reproduced it first: with `groupby: ['region']` and a Percentage comparison
the derived row comes back as `1 week ago, East`, which isn't one of the two
dimensionless names I was matching, so it resolved back to the source metric
and rendered `$ 0.25`.
Rather than extend the name list, I dropped the hand-built set and now
recognise these rows with `getTimeOffset` — the helper the derived-series
styling in this same file already uses:
```ts
const isPercentageComparisonSeries = (seriesKey: string) =>
chartProps.rawFormData?.comparison_type === ComparisonType.Percentage &&
(array.includes(seriesKey) ||
getTimeOffset({ name: seriesKey }, array) !== undefined);
```
It already knows all three shapes the backend produces (`<offset>, <dims>`,
`<metric>__<offset>`, `<metric>, <offset>`), so grouped and ungrouped both fall
out of it rather than being enumerated here. The one form it does not cover is
the bare offset of an ungrouped single-metric chart — which is exactly why
`isDerivedSeries` pairs it with an exact-match check, and `array.includes` does
that job here.
Added the grouped regression you asked for. It fails against the previous
commit with `Expected substring: "25.00%"` and passes now; the ungrouped
fixture still passes alongside it. 85 tests green in
`Timeseries/transformProps.test.ts`.
`Ratio` is still untouched and has the same unitless problem, but the right
output there is a plain number rather than a percent, so I have left it — happy
to fold it into this PR if you would rather it not wait.
--
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]
