fix(pivot-table): exclude rollup totals from conditional formatting scale - #43308
Conversation
…cale The conditional-formatting color scale was derived from the raw query result. For non-additive metrics that result is a single GROUPING SETS response carrying the rollup levels alongside the leaf rows, so enabling "Show rows total"/"Show column total" pulled the subtotals and grand total into the color domain. Those aggregates dominate the max, leaving every detail cell nearly unshaded. Derive the scale from the leaf level instead, so it spans only the cells being shaded. Additive metrics are unaffected: their query already returns leaf rows only.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43308 +/- ##
==========================================
+ Coverage 66.15% 66.73% +0.58%
==========================================
Files 2876 2876
Lines 164228 164194 -34
Branches 37891 37887 -4
==========================================
+ Hits 108640 109578 +938
+ Misses 53427 52460 -967
+ Partials 2161 2156 -5
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:
|
There was a problem hiding this comment.
Pull request overview
Fixes Pivot Table conditional formatting gradients becoming washed out when row/column totals are enabled for non-additive metrics (where the backend returns leaf rows plus rollup totals in a single GROUPING SETS result). The color domain is now derived from the leaf-level frame in transformProps, ensuring the scale reflects only detail cells (the ones actually shaded).
Changes:
- Compute conditional formatting color scales from the leaf-level
QueryDataframe (highest groupby dimensionality), instead ofmainQuery.datawhich may include rollup totals. - Add a regression test that simulates a
GROUPING SETSresponse containing leaf cells + row/column totals + grand total and asserts the leaf max is fully saturated.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts | Build conditional-formatting color domain from leaf-level split/synthesized data to exclude rollup totals. |
| superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/transformProps.test.ts | Add regression test ensuring conditional formatting scales over leaf cells only (not totals in the query response). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
The suggestion to reword the comment or use a generic metric name like superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/transformProps.test.ts |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Code Review Agent Run #9367efActionable 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 |
Address review feedback: - The additive fast path now feeds conditional formatting the raw leaf query rows again instead of the synthesized leaf level. The synthesis coerces values through Number and drops non-numeric ones, so routing the additive path through it could shift the color domain -- the additive path is now provably untouched, matching the claim made for it. Its query returns leaf rows only, so no totals can leak in. - Select the non-additive leaf level by the level that groups every dimension (the splitter's own definition) rather than by counting dimensions. - Clarify in the regression test that a string metric is a saved-metric reference treated as non-additive whatever its label, so "SUM(sales)" is not mistaken for an additive adhoc metric. - Add the additive counterpart test pinning its domain to the leaf max.
Code Review Agent Run #ab5661Actionable 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 |
SUMMARY
Pivot table conditional formatting produced washed-out cells once Show rows total / Show column total were enabled.
transformPropsbuilt the color scale frommainQuery.data, i.e. the raw chart-data response. Since #41184 (SIP-216), a chart with a non-additive metric (a saved-metric reference,AVG,COUNT_DISTINCT, or any SQL/adhoc metric — seeisAdditiveMetric) issues oneGROUPING SETSquery whose result carries every rollup level alongside the leaf rows. Turning on a totals toggle adds the corresponding collapsed level togrouping_sets, so the subtotals and the grand total end up in that response — and therefore in the color domain.Those aggregates are sums of the very cells being shaded, so they dominate
Math.max(...allValues)and compress every real cell toward transparent. With the four-cell example in the new test (leaf max 40, grand total 100), the largest detail cell rendered at alpha0x6E(~43%) instead of fully saturated. Disabling the totals was the only workaround, which matches the reported behaviour: the totals levels are simply not queried in that case.The fix threads a single
colorScaleRowsvariable out of the two branches that already split the response, so the domain spans only the leaf (detail) cells:GROUPING SETSresult, identified as the level that groups every dimension (the same definitionsplitGroupingSetsResultitself uses).Totals cells themselves are unaffected — the renderer never colors them (
TableRenderers.tsxdeliberately omitsgetCellColoronpvtTotal).Known limitation / scope: this corrects the domain only. It does not start coloring the totals cells; they are intentionally uncolored today and changing that is a separate design question.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No screenshots. I don't have a running instance with the sample dataset in this environment, so rather than post a mock-up I captured the regression as an exact color assertion instead — the observable symptom is the alpha channel of the cell background:
#ACE1C46E— ~43% opacity, visibly unshaded#ACE1C4FF— fully saturatedTESTING INSTRUCTIONS
Two tests in
superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/transformProps.test.ts:conditional formatting scales over leaf cells only, not rollup totals— feeds a realisticGROUPING SETSresponse (leaf cells + row totals + column totals + grand total, with__superset_groupingmarkers) and asserts the largest leaf cell is fully saturated. Fails onmasterwith#ACE1C46E, passes with this change.conditional formatting on the additive path uses the raw leaf query rows— the additive counterpart, pinning that domain to the leaf max. Passes both before and after, documenting that the additive path is untouched.Manual:
AVG(...)/COUNT_DISTINCT(...).metric > 0with a color and gradient enabled.Review guidance
One hunk in
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts; read it first, then the tests.Both review points from the first round are addressed:
data, which on the additive path meant the synthesized leaf level.synthesizeAdditiveLevelscoerces throughNumberand drops non-numeric values, so that could shift the domain for additive metrics, contradicting my claim that the path was unchanged. It now uses the raw leaf query rows on that branch, so the claim holds by construction rather than by argument. Good catch.isAdditiveMetrictreats any string metric as a non-additive saved-metric reference regardless of its label, which is exactly why a saved metric namedSUM(sales)takes this path.The
?? []on the non-additive branch only covers afindIndexmiss, whichbuildGroupbyCombinationsmakes unreachable — the full-length prefix on both axes is always emitted and thecombineMetricfilters keep it.Risk & rollback
Frontend-only, no feature flag, no migration. Blast radius is the pivot table's conditional formatting color domain. The only behavioural change is for non-additive metrics with totals enabled, where the current output is wrong. Additive metrics are unchanged. Revert the commits to roll back.
ADDITIONAL INFORMATION