aminghadersohi commented on code in PR #43308:
URL: https://github.com/apache/superset/pull/43308#discussion_r3811669972
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts:
##########
@@ -219,9 +219,23 @@ export default function transformProps(chartProps:
ChartProps<QueryFormData>) {
config.colorScheme !== ColorSchemeEnum.Green &&
config.colorScheme !== ColorSchemeEnum.Red,
);
+ // Scale conditional formatting over the leaf (detail) cells only. A
+ // GROUPING SETS result carries the rollup levels alongside the leaf rows, so
+ // using it directly would let the grand total -- an aggregate of the very
+ // cells being shaded -- dominate the color domain and leave every detail
+ // cell nearly unshaded once the totals toggles are on.
+ const leafLevel = data.reduce<QueryData | undefined>(
+ (leaf, level) =>
+ leaf === undefined ||
+ level.groupby.rows.length + level.groupby.columns.length >
+ leaf.groupby.rows.length + leaf.groupby.columns.length
+ ? level
+ : leaf,
+ undefined,
+ );
const metricColorFormatters = getColorFormatters(
pivotConditionalFormatting,
- mainQuery.data,
+ leafLevel?.data ?? mainQuery.data,
theme,
Review Comment:
Good catch — this was a real gap between what the code did and what I
claimed for it. Fixed in 0089b20.
The additive branch no longer goes through the synthesized leaf level.
`transformProps` now threads an explicit `colorScaleRows` out of each branch:
- additive → the raw leaf query rows (`queriesData[0].data`), exactly what
was passed before this PR. That query returns leaf rows only, so no totals can
leak in, and the `Number` coercion / non-numeric dropping in
`synthesizeAdditiveLevels` is out of the path entirely.
- non-additive → the leaf level of the split `GROUPING SETS` result.
So the additive path is now unchanged by construction rather than by
argument. I also added `conditional formatting on the additive path uses the
raw leaf query rows` to pin that domain, and verified against `master` that the
non-additive regression test still fails there (`#ACE1C46E`) while the additive
one passes both before and after.
##########
superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/transformProps.test.ts:
##########
@@ -452,3 +452,82 @@ test('additive metrics: synthesizes rollup levels from a
single leaf query', ()
{ region: 'EU', v: 5 },
]);
});
+
+test('conditional formatting scales over leaf cells only, not rollup totals',
() => {
+ const gm = (col: string) => `${col}__superset_grouping`;
+ // A saved-metric reference is non-additive, so buildQuery issues a single
+ // GROUPING SETS query whose result carries the rollup levels alongside the
+ // leaf rows. Both totals toggles are on, so the grand total (100) is part of
+ // that result.
Review Comment:
Agreed, the comment was misleading. Reworded in 0089b20.
I kept the `SUM(sales)` label deliberately — it is the metric name from the
report this regression comes from, and the confusing part *is* the point:
`isAdditiveMetric` treats any string as a non-additive saved-metric reference
regardless of its label, because form data does not reveal the aggregate behind
a saved metric. So a saved metric named `SUM(sales)` takes the non-additive
path even though the name suggests otherwise, which is precisely how a user
hits this bug. The comment now says that explicitly instead of just calling it
"a saved-metric reference".
--
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]
