fix(plugin-chart-echarts): exclude extra metrics from stacked totals - #42855
fix(plugin-chart-echarts): exclude extra metrics from stacked totals#42855krishn1301 wants to merge 2 commits into
Conversation
extractDataTotalValues summed every numeric column in each row, so a timeseries_limit_metric that is not rendered as a series still inflated the Only Total value. With metrics A=32, B=0 and a sort metric of 2, the total showed 34 instead of 32. Threads extraMetricLabels into extractDataTotalValues and excludes those columns from both the total and the derived threshold. Fixes apache#42701 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dosubot
Bot
added
viz:charts:echarts
Code Review Agent Run #05c1ebActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@krishn1301 Glad to see your contribution to the project! Can you add before and after screenshots of your fix into the PR description? |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if (extraMetricLabelsSet.has(curr)) { | ||
| return prev; | ||
| } |
There was a problem hiding this comment.
Suggestion: The exclusion only compares the raw column key against extraMetricLabels, but extractDataTotalValues receives rebasedData, whose metric keys may be renamed through verboseMap or suffixed for time-comparison series. In those cases a hidden sort metric such as Sort__1 day ago or its verbose label is not matched and is still added to the stacked total and threshold, so the reported inflation persists for forecast/time-comparison charts. Normalize the extra labels to the same rendered key format, or match derived keys consistently with extractSeries. [api mismatch]
Severity Level: Major ⚠️
- ❌ Time-comparison stacked totals include hidden sort metrics.
- ❌ Verbose-mapped extra metrics inflate displayed totals.
- ⚠️ Incorrect totals affect threshold-based label visibility.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
**Line:** 414:416
**Comment:**
*Api Mismatch: The exclusion only compares the raw column key against `extraMetricLabels`, but `extractDataTotalValues` receives `rebasedData`, whose metric keys may be renamed through `verboseMap` or suffixed for time-comparison series. In those cases a hidden sort metric such as `Sort__1 day ago` or its verbose label is not matched and is still added to the stacked total and threshold, so the reported inflation persists for forecast/time-comparison charts. Normalize the extra labels to the same rendered key format, or match derived keys consistently with `extractSeries`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct. The current implementation only filters based on the raw metric key, which fails when the data has been transformed (e.g., via To implement this, you can update the superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts |
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue in the ECharts timeseries stacked “Only Total” label calculation where non-rendered “extra metrics” (e.g., timeseries_limit_metric used only for sorting/limiting) were incorrectly included in the stacked totals, inflating both displayed totals and the derived thresholdValues.
Changes:
- Extend
extractDataTotalValuesto accept an optionalextraMetricLabelslist and exclude those columns from stacked total accumulation. - Compute
extraMetricLabelsearlier intransformProps.tsand pass them intoextractDataTotalValues. - Add unit tests covering exclusion behavior, the default (backward-compatible) behavior when no extra metrics are passed, and interactions with
legendState/thresholdValues.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts | Adds focused unit tests for extractDataTotalValues covering the reported regression and expected edge cases. |
| superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts | Updates stacked-total computation to optionally exclude extra (non-rendered) metric columns. |
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts | Moves extra-metric label derivation earlier and threads it into total/threshold computation. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42855 +/- ##
=======================================
Coverage 66.37% 66.37%
=======================================
Files 2857 2857
Lines 161048 161052 +4
Branches 37046 37048 +2
=======================================
+ Hits 106889 106893 +4
Misses 52143 52143
Partials 2016 2016
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
SUMMARY
Fixes #42701.
With Stacked Style: Stack and Only Total enabled, the total shown above each bar included the sort metric. In the reported case, metrics
A = 32andB = 0with atimeseries_limit_metricof2displayed a total of 34 instead of 32.extractDataTotalValuessums every numeric column in each row:A sort metric is present in the query result but is never rendered as a series, so it silently inflated the total — and the
thresholdValuesderived from that total, which decides which labels are shown.This threads the extra metric labels into
extractDataTotalValuesand skips those columns when accumulating. The labels were already being computed intransformProps.tsviaextractExtraMetrics(...).map(getMetricLabel); that computation just happened after theextractDataTotalValuescall, so it's moved above and passed in. The new option is optional, so other callers ofextractDataTotalValuesare unaffected.TESTING INSTRUCTIONS
Four tests were added under
extractDataTotalValues, using the exact numbers from the issue:A=32, B=0, Sort=2gives[32](fails without this change)extraMetricLabelsare passed — the same row gives[34]thresholdValuesfrom the total excluding extra metrics (fails without this change)legendStatealongside the exclusionManually: build a Stacked Timeseries Bar chart with two metrics, set a Sort By metric that differs from both, enable Only Total, and confirm the total equals the sum of the visible segments.
ADDITIONAL INFORMATION