fix(pivot-table): exclude rollup totals from conditional formatting scale - #43481
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.
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.
dosubot
Bot
added
change:frontend
Code Review Agent Run #978b65Actionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. The current implementation filters the domain for conditional formatting to include only leaf rows, which prevents subtotals from skewing the color scale, but it inadvertently causes subtotals to lose their conditional formatting because they are no longer part of the domain used by To resolve this, you should include the rendered subtotal values in the domain while ensuring they do not influence the detail scale. This can be achieved by calculating the color scale based only on leaf rows, but allowing the 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-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 #43481 +/- ##
==========================================
+ Coverage 78.93% 78.94% +0.01%
==========================================
Files 2877 2878 +1
Lines 165232 165115 -117
Branches 38184 38162 -22
==========================================
- Hits 130418 130356 -62
+ Misses 32352 32309 -43
+ Partials 2462 2450 -12
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:
|
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks for the thorough writeup @aminghadersohi, the PR body made this straightforward to verify.
I checked the claims empirically on this branch:
- The new regression test fails on master with exactly
#ACE1C46Eand passes here. The additive counterpart passes on both sides, pinning that path as unchanged. - Full plugin suite is green locally with this change: 9 suites, 110 tests.
buildQuery.tsconfirms both query shapes: the additive path issues a single leaf query with nogrouping_sets, soqueriesData[0].datacannot contain totals, and the non-additive path sends one grouping-sets level per displayed total.TableRenderers.tsxhas threegetCellColorcall sites (column headers, row headers,pvtValcells) and none of them touch thepvtTotalcells or the totals row, so totals rendering is indeed untouched.
On the CodeAnt thread about subtotals losing formatting: the delta is real, but only for the None comparator, and I would not treat it as a blocker. I ran the exact scenario on both sides of the fix (non-additive metric, rowSubTotals/colSubTotals on, leaf max 40, subtotal 70):
Nonecomparator: before,getColorFromValue(70)returned#ACE1C4AB; after, it returnsundefined, so a subtotal cell above the leaf max loses its tint.- Bounded comparators (
> 0and friends):getOpacityclamps at full opacity, so the same subtotal renders fully saturated before and after.
Three reasons I read that delta as the correct behavior rather than a regression:
- The additive path already behaves exactly this way on master. I ran the same subtotal scenario with a SIMPLE
SUMmetric against master andNonereturnsundefinedfor the subtotal there too. This PR makes the non-additive path consistent with it. - It restores the pre-SIP-216 behavior: before #41184 the color domain was
queriesData[0].data, which was the leaf-level query. - The Table chart keeps its totals in a separate query and feeds only the base query rows to
getColorFormatters, so a leaf-only domain is the established convention.
Whether subtotal and total cells should be colored against a leaf-derived scale at all is the separate design question you already scoped out, and I agree it does not belong in this fix.
LGTM.
|
Thank you @gabotorresruiz for the exceptionally thorough validation and approval — especially for reproducing the |
2 participants
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