aminghadersohi opened a new pull request, #43481: URL: https://github.com/apache/superset/pull/43481
### SUMMARY Pivot table conditional formatting produced washed-out cells once **Show rows total** / **Show column total** were enabled. `transformProps` built the color scale from `mainQuery.data`, i.e. the raw chart-data response. Since [#41184 (SIP-216)](https://github.com/apache/superset/pull/41184), a chart with a **non-additive** metric (a saved-metric reference, `AVG`, `COUNT_DISTINCT`, or any SQL/adhoc metric — see `isAdditiveMetric`) issues one `GROUPING SETS` query whose result carries every rollup level alongside the leaf rows. Turning on a totals toggle adds the corresponding collapsed level to `grouping_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 alpha `0x6E` (~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 `colorScaleRows` variable out of the two branches that already split the response, so the domain spans only the leaf (detail) cells: - **non-additive** — take the leaf level out of the split `GROUPING SETS` result, identified as the level that groups every dimension (the same definition `splitGroupingSetsResult` itself uses). - **additive** — keep using the raw leaf query rows. That query returns leaf rows only, so no totals can leak in, and this path is therefore **completely unchanged**. Totals cells themselves are unaffected — the renderer never colors them (`TableRenderers.tsx` deliberately omits `getCellColor` on `pvtTotal`). **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: | | largest detail cell (leaf max 40, grand total 100) | |---|---| | Before | `#ACE1C46E` — ~43% opacity, visibly unshaded | | After | `#ACE1C4FF` — fully saturated | ### TESTING 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 realistic `GROUPING SETS` response (leaf cells + row totals + column totals + grand total, with `__superset_grouping` markers) and asserts the largest leaf cell is fully saturated. **Fails on `master` with `#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. ``` npx jest plugins/plugin-chart-pivot-table # 9 suites, 109 tests passed ``` Manual: 1. Pivot table on a dataset with a **non-additive** metric — a *saved* metric, or `AVG(...)`/`COUNT_DISTINCT(...)`. 2. Put a dimension on rows and another on columns. 3. Enable **Show rows total** and **Show column total**. 4. Add conditional formatting `metric > 0` with a color and gradient enabled. 5. Before: cells are barely tinted, and get fainter the more totals are shown. After: the gradient spans the detail cells, with the largest one fully saturated. ### 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: - **Additive path semantics** (@codeant-ai) — the original patch selected the leaf frame out of the already-split `data`, which on the additive path meant the *synthesized* leaf level. `synthesizeAdditiveLevels` coerces through `Number` and 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. - **Misleading test comment** (@Copilot) — reworded to state explicitly that `isAdditiveMetric` treats *any* string metric as a non-additive saved-metric reference regardless of its label, which is exactly why a saved metric named `SUM(sales)` takes this path. The `?? []` on the non-additive branch only covers a `findIndex` miss, which `buildGroupbyCombinations` makes unreachable — the full-length prefix on both axes is always emitted and the `combineMetric` filters 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 - [ ] Has associated issue: - [ ] Required feature flags: - [x] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
