fix(pivot-table): include synthesized subtotal/grand-total values in conditional-formatting extremes - #43457
Conversation
…conditional-formatting extremes metricColorFormatters was built from mainQuery.data, which is only the raw leaf-level backend query result. Subtotal and grand-total values are synthesized separately on the client (synthesizeAdditiveLevels / splitGroupingSetsResult) into a different data array that is what actually gets rendered, and those values never appeared in mainQuery.data. As a result, the color scale's min/max was calibrated only against leaf-level magnitudes. A grand-total value large enough to exceed that range would fall outside the comparator's bounds and silently receive no color, while smaller leaf-level values within range rendered correctly -- exactly the anomaly reported in apache#43084 (large negative grand-total values left uncolored while smaller negative values in the same column were highlighted). Fix: compute the color formatters from every synthesized rollup level (data.flatMap(level => level.data)) instead of mainQuery.data alone. Fixes apache#43084
Code Review Agent Run #ac04c4Actionable 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. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| // fall outside the scale's range and silently fail to receive a color, | ||
| // while smaller leaf-level values within range render as expected. | ||
| // See #43084. | ||
| const allLevelRecords = data.flatMap(level => level.data); |
There was a problem hiding this comment.
Suggestion: data also contains rollup levels that are forced solely to calculate percentage denominators when the corresponding total or subtotal is disabled. Those denominator records are not rendered as cells, but including them in getColorFormatters changes the min/max used by Comparator.None and threshold-based formatters, causing visible cells to receive colors calibrated against hidden raw totals. Build the color-scale input from levels that are actually rendered, or otherwise exclude calculation-only denominator levels. [logic error]
Severity Level: Major ⚠️
- ⚠️ Percentage pivot views include hidden denominator values in color calibration.
- ⚠️ Visible leaf cells can receive misleading conditional colors.
- ⚠️ Disabled total rows or columns still affect formatting ranges.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts
**Line:** 231:231
**Comment:**
*Logic Error: `data` also contains rollup levels that are forced solely to calculate percentage denominators when the corresponding total or subtotal is disabled. Those denominator records are not rendered as cells, but including them in `getColorFormatters` changes the min/max used by `Comparator.None` and threshold-based formatters, causing visible cells to receive colors calibrated against hidden raw totals. Build the color-scale input from levels that are actually rendered, or otherwise exclude calculation-only denominator levels.
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. Including calculation-only denominator records in the color scale calibration causes the min/max range to be skewed by hidden values, leading to incorrect conditional coloring for visible cells. To resolve this, you should filter superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43457 +/- ##
=======================================
Coverage 78.86% 78.86%
=======================================
Files 2876 2876
Lines 164601 164602 +1
Branches 38015 38015
=======================================
+ Hits 129806 129807 +1
Misses 32348 32348
Partials 2447 2447
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:
|
1 participant
SUMMARY
Fixes the conditional-formatting coloring anomaly described in #43084, where cells within the same column receive background colors that don't correctly reflect their relative magnitude.
Root cause:
metricColorFormattersintransformProps.tscomputed the color scale's min/max ("extremes") frommainQuery.data, which only contains the raw leaf-level backend query result. Subtotal and grand-total values are synthesized separately on the client (synthesizeAdditiveLevels/splitGroupingSetsResult) into a differentdataarray — the one that's actually rendered — and those values never fed into the color scale calculation.As a result, a grand-total value large enough to exceed the leaf-level range would fall outside the comparator's calibrated bounds and silently receive no color, while smaller leaf-level values within range rendered correctly. This is visible in the issue's screenshot:
-3.06Mand-2.67M(grand-total row) were left uncolored while smaller negative values like-193kand-370kin the same columns were correctly highlighted.Fix: build the color formatters from every synthesized rollup level (
data.flatMap(level => level.data)) instead ofmainQuery.dataalone, so the scale's extremes account for every value that actually gets rendered — leaf rows, subtotals, and grand totals.BEFORE/AFTER
Before: grand-total cells with large magnitudes were skipped by conditional formatting while smaller leaf-level cells in the same column were colored (see screenshot in #43084).
After: all cells, including subtotals/grand totals, are evaluated against a color scale calibrated on the full range of rendered values.
TESTING INSTRUCTIONS
transformProps.test.ts: builds a pivot table with a grand total (15) larger than any leaf value (10, 5), and asserts the grand total receives a color under a full-range (Comparator.None) conditional-formatting rule — the exact scenario that silently failed before this fix.npm test -- plugins/plugin-chart-pivot-table→ 9 suites passed, 109 tests passed.Note: this PR addresses only the conditional-formatting anomaly (Issue #43084, part 1). The row-label-truncation feature request in the same issue is a separate concern and is not included here.