aminghadersohi opened a new pull request, #43308: URL: https://github.com/apache/superset/pull/43308
### 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 derives the scale from the leaf level of the already-split `data` frames rather than the raw response, so the domain spans only the cells that actually get shaded. Totals cells themselves are unaffected — the renderer never colors them (`TableRenderers.tsx` deliberately omits `getCellColor` on `pvtTotal`). **Known limitation / scope:** this only corrects the *domain*. Charts whose metrics are all additive (`SUM`/`COUNT`/`MIN`/`MAX` as adhoc SIMPLE metrics) were never affected — their query returns leaf rows only — and their behaviour is byte-for-byte unchanged, which I verified explicitly before and after the patch. ### 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 Regression test: `conditional formatting scales over leaf cells only, not rollup totals` in `superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/transformProps.test.ts`. It 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. It fails on `master` with `#ACE1C46E` and passes with this change. ``` npx jest plugins/plugin-chart-pivot-table # 9 suites, 108 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 test. The piece worth scrutiny is how the leaf level is picked: `data.reduce` selects the frame with the most `groupby.rows + groupby.columns` dimensions. That relies on `buildGroupbyCombinations` always emitting the full-length prefix on both axes (it does — the leaf level is unconditional, and the `combineMetric` filters keep it). The `?? mainQuery.data` fallback only covers a hypothetically empty `data`, preserving the previous behaviour rather than throwing. I did not extend this to the totals cells themselves; they are intentionally uncolored today and changing that is a separate design question. ### 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 single commit 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]
