fix(mixed-timeseries): stop duplicating first metric in multi-metric + group-by series names - #40146
Conversation
b9ef239 to
3cc68d1
Compare
rusackas
changed the title
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40146 +/- ##
=======================================
Coverage 65.24% 65.24%
=======================================
Files 2795 2795
Lines 157643 157649 +6
Branches 36061 36067 +6
=======================================
+ Hits 102853 102859 +6
Misses 52814 52814
Partials 1976 1976
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:
|
Code Review Agent Run #484e66Actionable 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 |
…p duplicating first metric (#37921) When Query A (or B) on a Mixed Chart has multiple metrics plus at least one Group By dimension, the display-name builder in transformProps.ts always prepended metrics[0]'s label: the entryName.includes(metricPart) guard is false for every series belonging to metrics[1+], so those series were renamed to e.g. 'score_one, score_two, A' instead of 'score_two, A' — the 'first metric duplicated in legend/tooltip' symptom reported in #37921 (residual follow-up to #37055). Derive each series' metric from its label_map tuple ([metric, ...dimensions]) instead, matching what the per-series formatter lookup already does. Single-element tuples (no metric part) fall back to the first metric, preserving existing behavior for single-metric charts and showQueryIdentifiers naming. Applied to both the Query A and Query B paths. Includes the regression test originally landed red on this branch, verified red on master without the fix and green with it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3cc68d1 to
32f252f
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. Using To resolve this, you can update the test to count occurrences: // Instead of just toContain, verify the exact count
expect(queryASeriesNames).toHaveLength(4);
expect(queryASeriesNames.filter(n => n === 'score_one, A')).toHaveLength(1);
expect(queryASeriesNames.filter(n => n === 'score_one, B')).toHaveLength(1);
expect(queryASeriesNames.filter(n => n === 'score_two, A')).toHaveLength(1);
expect(queryASeriesNames.filter(n => n === 'score_two, B')).toHaveLength(1);There are no other comments on this PR to address. Would you like me to implement this change for you? superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts |
Code Review Agent Run #e8fde8Actionable 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 |
…te series toContain only checked presence, so a regression emitting duplicate series with the same (correct) name could still pass the regression test. Compare the sorted array against the expected names instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #2a37bdActionable 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 |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Approving.
Crux — series-label correctness across the full matrix. The fix derives each series' metric from its label_map tuple [metric, ...dims] (getMetricDisplayName(labelMapValues[0], verboseMap) when the tuple has >1 element) instead of always using metrics[0]. Walked the matrix:
- multi-metric + groupby → each series gets its own correct prefix (
score_two, A), no first-metric duplication; - single-metric + groupby → length-1/absent tuple falls back to
metrics[0], behavior unchanged; - no-groupby → untouched
elsebranch; showQueryIdentifierscorrectly threads the per-series metric into the(Query A/B)form;- groupby value colliding with a metric name is handled, since
metricPartis now the real per-series metric so theincludesguard matches the actual prefix; - Query B path is a faithful mirror;
- the
displayLabelMap/ cross-filter map (#41622) stays consistent since the correcteddisplayNameis alsoseries.name.
No legitimate series is dropped, renamed, or collapsed in any cell. It reuses the existinggetMetricDisplayNamehelper keyed on the label-map string rather than positional indexing, so it's robust to A/B metric-array length differences.
Test (Rule-26). The regression test runs the real transformProps on real 4-column data + matching label_map and asserts sorted actual series names equal the four expected metric, dimension names plus not.toMatch(/score_one,\s+score_two/). Reverting only the production change makes metricPart = score_one for every series → score_one, score_two, A → fails both assertions. It specifically catches the duplication regression.
Threads / CI. codeant-ai's thread (swap toContain → sorted toEqual) is resolved and the change is present at HEAD; htcnokia approved; rusackas's comment resolves the codeant thread; bito reported 0 actionable suggestions. No unresolved review threads at HEAD 897f9164. Full gh pr checks all green (jest shards, playwright, cypress, lint, mypy, pre-commit, CodeQL); mergeable: MERGEABLE.
5 participants
SUMMARY
Fixes #37921 ("Residual" follow-up to #37055): in a Mixed Chart, when Query A (or B) has multiple metrics plus at least one Group By dimension, the first metric's label got prepended to every other metric's series, producing legend/tooltip entries like
score_one, score_two, Ainstead ofscore_two, A.Root cause: in
MixedTimeseries/transformProps.tsthe "metric, dimension" display-name builder always usedmetrics[0](MetricDisplayNameA/MetricDisplayNameB). The guard wasentryName.includes(metricPart), which is false for every series belonging tometrics[1+], so those series got the first metric's label prepended on top of their own.Fix: each series' metric is derived from its
label_maptuple ([metric, ...dimensions]), which the backend already provides and which the per-series formatter lookup was already using. When the tuple doesn't carry a metric part (single-element tuples), behavior falls back to the first metric exactly as before, so single-metric charts and theshowQueryIdentifiersnaming are unchanged. The same fix is applied to both the Query A and Query B paths.This PR originally landed as a deliberately-failing regression test; it has been reworked to include the actual fix, and the test passes. The test:
metrics: ['score_one', 'score_two']+groupby: ['category']on Query Ascore_one, A,score_one, B,score_two, A,score_two, B) with a matchinglabel_map(metric, dim_value)combo appears exactly once with the correct metric prefix/score_one,\s+score_two/)Verified red→green locally: on master without the fix the series come back as
["score_one, score_two, B", "score_one, score_two, A", "score_one, B", "score_one, A"]; with the fix they are the expected four distinctmetric, dimensionnames.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — string-level series naming in
transformProps; see the issue for the user-visible legend symptom.TESTING INSTRUCTIONS
All 37 MixedTimeseries tests (and the full 722-test plugin-chart-echarts suite) pass locally.
ADDITIONAL INFORMATION
🤖 Generated with Claude Code