fix(echarts): enable cross-filtering for pie chart "Other" slice - #43088
fix(echarts): enable cross-filtering for pie chart "Other" slice#43088omsn2 wants to merge 8 commits into
Conversation
…ements Fixes invalid cross-filters being emitted when non-category pie chart elements (Total graphic text, Other slice, empty name events) are clicked. getCrossFilterDataMask now returns undefined when any selected value has no labelMap entry, and clickEventHandler guards against empty name events. Fixes apache#42340
The 'Other' slice in a pie chart aggregates multiple rows into a single segment. Its labelMap entry is a 2D array (string[][]) — one row per aggregated data point — whereas all other slices use a 1D array (string[]). The previous getCrossFilterDataMask guard required groupbyValues.length to equal values.length. Because flatMap on a 2D entry expands it into multiple rows, this check always failed for 'Other', silently suppressing the cross-filter emission. Changes: - transformProps.ts: populate labelMap['Other'] with a 2D array (one string[] per aggregated row) so the event handler has the raw dimension values available. - eventHandlers.ts: replace the strict equality guard with a looser check (length === 0 && values.length > 0) and use flatMap to normalise both 1D and 2D labelMap entries into a uniform string[][] before building the IN-filter payload. - types.ts: make CrossFilterTransformedProps generic on its labelMap value type (default string[]) so the pie chart can declare string[] | string[][] without breaking other chart types. - Pie/types.ts: instantiate the generic for PieChartTransformedProps.
dosubot
Bot
added
change:frontend
Code Review Agent Run #3b3edbActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review 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. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if (otherDatum && otherRows.length > 0) { | ||
| labelMap[otherDatum.name] = otherRows.map(row => | ||
| groupbyLabels.map(col => row[col] as string), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Suggestion: The aggregated entry is stored under the rendered name Other, so it overwrites any real data row whose formatted groupby label is also Other. Since both slices are rendered with the same ECharts name, clicking the real row or the aggregate can then resolve to the aggregated rows and emit an incorrect cross-filter. Use a collision-safe key or otherwise disambiguate the aggregate from ordinary data labels. [logic error]
Severity Level: Major ⚠️
- ❌ Real `Other` category clicks filter aggregated rows.
- ❌ Aggregate and real slices share cross-filter selection state.
- ⚠️ Pie-to-chart cross-filter results become incorrect for colliding labels.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts
**Line:** 366:370
**Comment:**
*Logic Error: The aggregated entry is stored under the rendered name `Other`, so it overwrites any real data row whose formatted groupby label is also `Other`. Since both slices are rendered with the same ECharts name, clicking the real row or the aggregate can then resolve to the aggregated rows and emit an incorrect cross-filter. Use a collision-safe key or otherwise disambiguate the aggregate from ordinary data labels.
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 valid. The current implementation uses the rendered label (e.g., 'Other') as the key in To resolve this, you should disambiguate the keys in Since the issue is identified in Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43088 +/- ##
==========================================
- Coverage 66.62% 66.62% -0.01%
==========================================
Files 2866 2866
Lines 162586 162608 +22
Branches 37468 37481 +13
==========================================
+ Hits 108327 108338 +11
- Misses 52166 52177 +11
Partials 2093 2093
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:
|
…ng collisions - Applies a unique prefix '__other__' to the aggregated slice in labelMap - Updates Pie chart cross-filtering event handlers to reconstruct the key using data.isOther - Preserves accurate filtering behavior for real data rows named 'Other' without breaking UI highlighting
…slice key - Aborts cross-filter emission if clicked elements lack a valid name (e.g. empty labels) - Safely no-ops if any selected values cannot be strictly resolved in the labelMap (e.g. 'Total' text) - Applies a unique prefix '__other__' to the aggregated slice in labelMap to prevent cross-filtering collisions - Updates Pie chart event handlers to reconstruct the key using data.isOther, preserving accurate filtering behavior for real data rows named 'Other'
Code Review Agent Run #60522cActionable 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 |
1 participant
SUMMARY
The "Other" slice in a pie chart aggregates multiple rows into one visual
segment. In
transformProps.ts, itslabelMapentry is a 2D array(
string[][]— one row per aggregated data point), whereas all otherslices use a 1D array (
string[]).The previous
getCrossFilterDataMaskguard requiredgroupbyValues.length === values.length. BecauseflatMapon a 2Dentry expands into multiple rows, this check always failed silently for
"Other", suppressing cross-filter emission entirely (no API calls fired).
Root cause: The guard
groupbyValues.length !== values.lengthwastoo strict and did not account for the multi-row "Other" aggregation.
Fix:
transformProps.ts: PopulatelabelMap['Other']with a 2D array(one
string[]per aggregated row).eventHandlers.ts: Replace the strict equality guard withlength === 0 && values.length > 0, and useflatMapto normaliseboth 1D and 2D
labelMapentries into a uniformstring[][]beforebuilding the
IN-filter payload.types.ts: MakeCrossFilterTransformedPropsgeneric on itslabelMapvalue type (defaultstring[]) so the pie chart candeclare
string[] | string[][]without breaking other chart types.Pie/types.ts: Instantiate the generic forPieChartTransformedProps.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before Fix: When Clicked on the Other we get Null Data in Bar Chart
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
###Dataset Used
salesdata.xlsx