WhoamiI00 commented on code in PR #43113:
URL: https://github.com/apache/superset/pull/43113#discussion_r3940787059
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -606,6 +607,47 @@ 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.
+ //
+ // `label_map` carries the structured identity behind a rendered series
name, and
+ // `renameOperator` puts the offset at the front of a derived row's entry:
+ //
+ // 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']
+ //
+ // so the leading column says which it is. Matching the rendered name
instead would
+ // misread a base series whose dimension value happens to equal the offset —
a region
+ // literally named "1 week ago" gives 'sum__num, 1 week ago', which reads as
derived.
+ const isDerivedComparisonSeries = (seriesKey: string) => {
+ const columns = labelMap?.[seriesKey];
+ // Without an entry there is nothing structured to go on, and the only
name that can
+ // stand alone is the bare offset of an ungrouped single-metric chart.
Review Comment:
Right again, and this one was self-inflicted — thanks for catching it. Fixed
in bfde7a85.
The normalization at the top of `transformProps` shifts the leading offset
off each entry when `timeCompare` is populated:
```ts
if (
entry[1].length > groupBy.length &&
Array.isArray(timeCompare) &&
timeCompare.includes(entry[1][0])
) {
entry[1].shift();
}
```
So by the time the formatters run, `'1 week ago, East'` maps to `['East']`,
and my `labelMap[key][0]` check saw the dimension value. The grouped Percentage
row fell back to the metric's currency and the Ratio row kept a currency format
— exactly the symptoms the lookup was supposed to fix.
My earlier fixtures missed it because they set `time_compare` but not
`timeCompare`, so the shift never ran. That's on me: I should have exercised
the same field the normalization reads.
The identity is now captured in that pass, which is the one place it is
still intact:
```ts
const derivedComparisonSeries = new Set<string>();
// ...
derivedComparisonSeries.add(entry[0]);
entry[1].shift();
```
The `labelMap` lookup stays as the path for charts where the shift never
runs — with `timeCompare` absent the entry still leads with the offset — so the
false positive you found earlier (a region named `1 week ago`) remains covered.
That test still passes.
Two fixtures added with `timeCompare` set, as requested: grouped Percentage
renders `25.00%` and grouped Ratio renders `1.25`, both with the base row
keeping `$ 100`. They fail on the previous commit with `Expected substring:
"25.00%"` and `Expected substring: not "$ 1.25"`.
123 tests green in `Timeseries/transformProps.test.ts`.
--
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]
