WhoamiI00 commented on code in PR #43113:
URL: https://github.com/apache/superset/pull/43113#discussion_r3921314766
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -606,6 +607,36 @@ export default function transformProps(
const array = ensureIsArray(chartProps.rawFormData?.time_compare);
const inverted = invert(verboseMap);
+ // A Percentage or Ratio time comparison replaces the derived series' values
with a
+ // dimensionless number, so that row is no longer in the source metric's
units and
+ // must not inherit its currency/D3 format. `renameOperator` names those
series with
+ // the offset alone or `<metric>, <offset>`, and a grouped chart appends its
dimension
+ // values on top ("1 week ago, East"). Recognise them with the same helper
the
+ // derived-series styling uses rather than matching exact names:
`getTimeOffset`
+ // covers every form except the bare offset of an ungrouped single-metric
chart,
+ // which the offsets themselves match.
+ const isDerivedComparisonSeries = (seriesKey: string) =>
+ array.includes(seriesKey) ||
+ getTimeOffset({ name: seriesKey }, array) !== undefined;
Review Comment:
Confirmed and fixed in ae3f6ebc — good catch, that was a real false positive
and I should have gone structural the first time rather than pattern-matching
the rendered name.
Reproduced it: with a region literally named `1 week ago`, the **base**
series is `sum__num, 1 week ago`, which `getTimeOffset` matches on its `,
${offset}` branch. Percentage mode then formatted that row's `$ 100` as a
percent.
`label_map` settles it without guessing, because `renameOperator` puts the
offset at the front of a derived row's entry while a base row leads with its
metric:
```
derived '1 week ago, East' -> ['1 week ago', 'East']
derived 'count, 1 year ago' -> ['1 year ago', 'count']
base 'sum__num, East' -> ['sum__num', 'East']
base 'sum__num, 1 week ago' -> ['sum__num', '1 week ago'] <- the case
you found
```
So the leading column is the whole test:
```ts
const isDerivedComparisonSeries = (seriesKey: string) => {
const columns = labelMap?.[seriesKey];
return columns?.length
? array.includes(columns[0])
: array.includes(seriesKey);
};
```
The fallback covers the one name that can stand alone — the bare offset of
an ungrouped single-metric chart, which has no dimensions to disambiguate and
cannot collide with a groupby value.
Added a regression with a region named `1 week ago`, asserting the base row
keeps `$ 100` **and** its genuinely derived counterpart still renders `25.00%`,
so the fix can't be satisfied by simply classifying nothing as derived. It
fails on the previous commit with `Expected substring: "$ 100"`.
121 tests green in `Timeseries/transformProps.test.ts`. `getTimeOffset` is
still used for the derived-series dash/colour styling further down; only the
formatter path moved to `label_map`.
--
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]
