bikash-barnwal opened a new pull request, #43181:
URL: https://github.com/apache/superset/pull/43181

   ### SUMMARY
   
   Conditional formatting on a Pivot Table produced background colors that 
don't track value order within a column: some cells were darker than larger 
cells, and subtotal cells — the largest values in their column — often had no 
background at all.
   
   The color scale was built from the **raw rows of the query response** rather 
than from the values the chart renders. `transformProps` passed 
`mainQuery.data` to `getColorFormatters`, which derives the reference 
distribution as `data.map(row => row[config.column])` and takes min/max from it 
(`getColorFormatters.ts:116-117`, `:325`). The rendered cells are the 
per-rollup-level pre-aggregated values, so the two sets disagree — and they 
disagree differently per query path:
   
   - **additive fast path** — returns leaf rows only, so every subtotal cell 
sits outside `[min, max]` and `Comparator.None` returns `false` → the cell gets 
no color (`getColorFormatters.ts:118-120`);
   - **GROUPING SETS path** — returns every rollup level, so the grand total 
inflates the domain and squashes the leaf cells into a narrow alpha band;
   - **bounded comparators** (`>`, `<`, …) fail symmetrically: the gradient 
extreme is a leaf row, so every cell beyond it clamps to `MAX_OPACITY` 
(`:62-71`) and the cells become indistinguishable.
   
   This is measurable in the reporter's screenshot. Pixel-sampling the colored 
cells gives alphas of 0.112 / 0.160 / 0.218 for −193k / −370k / −545k, which 
fits `0.05 + 0.95·|v|/3.06M` — the gradient extreme is −3.06M, a *subtotal* 
value present in the domain only because rollup rows were in the reference 
data, while those subtotal cells themselves render with no background.
   
   The fix builds the distribution from the per-level frames the renderer 
consumes, excluding the levels that only feed the uncolored "Total" row/column. 
Which axis decides that depends on `metricsLayout`, since the metric 
pseudo-dimension always occupies one axis.
   
   Checked and rejected as causes: formatter key matching (`keys = [...rowKey, 
...colKey]` works because `METRIC_KEY` is part of the axis) and non-numeric 
values poisoning the domain (possible in theory, not what happens here).
   
   **Scope:** this fixes only issue 1 (the formatting bug). Issue 2 in the same 
report — ellipsis truncation for long row labels — needs a new chart control 
plus styling and does not belong in a bug-fix PR, so the issue stays open for 
it. Hence *Addresses* rather than *Fixes*.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   
   Before: subtotal cells uncolored while smaller leaf cells are shaded; alphas 
don't follow value order.
   After: every rendered value is colored, and alpha increases with magnitude 
across leaf and subtotal cells alike.
   
   ### TESTING INSTRUCTIONS
   
   ```bash
   cd superset-frontend
   npm run test -- plugins/plugin-chart-pivot-table
   ```
   
   109 tests across 9 suites pass. Two new tests name #43084 and both were 
confirmed to **fail** on unpatched source: one asserts a `None` color scale 
colors every rendered value (leaves 3/5/10, subtotal 15, total 18) with 
non-decreasing alpha and the subtotal darkest; the other asserts a bounded 
(`>`) gradient is strictly increasing rather than saturating early (`Expected > 
255, Received 255` before the fix).
   
   Manually: build a Pivot Table with row subtotals enabled, add a conditional 
formatting rule on a metric column, and confirm the shading increases 
monotonically with the values, subtotals included.
   
   ### ADDITIONAL INFORMATION
   
   - [x] Has associated issue: Addresses #43084
   - [ ] 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
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)


-- 
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]

Reply via email to