rusackas commented on code in PR #43071:
URL: https://github.com/apache/superset/pull/43071#discussion_r3943101619
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -409,6 +409,10 @@ export function extractDataTotalValues(
if (excludedKeys.has(curr)) {
return prev;
}
+ // Exclude time-comparison derived columns of excluded keys (e.g.
SortMetric__1 year ago)
+ if (Array.from(excludedKeys).some(key => curr !== key &&
curr.startsWith(key + TIME_COMPARISON_SEPARATOR))) {
+ return prev;
+ }
Review Comment:
Agreed, the prefix match is broader than it needs to be. `timeCompare` is
already in scope in `transformProps` (the `matchSeriesKey` helper further down
builds exact `<label>__<offset>` names from it), so passing those offsets in
and adding the exact derived keys to `excludedKeys` seems like the safer
version of this. Metric names with `__` do show up in the wild (`sum__num` and
friends).
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -409,6 +409,10 @@ export function extractDataTotalValues(
if (excludedKeys.has(curr)) {
return prev;
}
+ // Exclude time-comparison derived columns of excluded keys (e.g.
SortMetric__1 year ago)
+ if (Array.from(excludedKeys).some(key => curr !== key &&
curr.startsWith(key + TIME_COMPARISON_SEPARATOR))) {
+ return prev;
Review Comment:
Agreed, and if the exact `<label>__<offset>` keys get precomputed into
`excludedKeys` per the codeant thread above, this one goes away for free.
--
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]
