rusackas commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4179797154
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -796,20 +933,81 @@ const baseAggregatorTemplates = {
string[],
string[],
];
+ // `type`'s selector (above) collapses one or both axes to `[]`,
+ // meaning "sum across everything on that axis". When Metric
+ // itself lives on the collapsed axis, "everything" would mean
+ // "every metric", silently adding unlike units together (e.g.
+ // SUM and MAX in the same denominator) -- substitute the
+ // metric's own key back in so the lookup stays scoped to this
+ // cell's own metric, the same way it's already scoped to this
+ // cell's own row/column. This applies to 'row'/'col' just as
+ // much as 'total': a row-fraction denominator still needs to
+ // stay within one metric, not sum across the metrics sharing
+ // that row.
+ let metricSubstituted: 'row' | 'col' | undefined;
if (this.metricAxis) {
if (this.metricAxis.axis === 'col' && selCol.length === 0) {
selCol = [this.metricAxis.value];
+ metricSubstituted = 'col';
} else if (
this.metricAxis.axis === 'row' &&
selRow.length === 0
) {
selRow = [this.metricAxis.value];
+ metricSubstituted = 'row';
}
}
- const denominatorAggregator = data.getAggregator(selRow, selCol);
- if (!denominatorAggregator.inner) {
+ // A metric-substituted lookup must go straight to the per-metric
+ // maps (`rowMetricTotals`/`colMetricTotals` for 'total',
+ // `rowGroupMetricTotals`/`colGroupMetricTotals` for 'row'/'col'),
+ // never through the generic keyed tree below: the tree's flat
+ // keys aren't namespaced by dimension, so a real category value
+ // that happens to equal the metric's own name (e.g. a "sales"
+ // metric alongside a "sales" category) flattens to the same key
+ // as the substituted metric address and would silently return
+ // that category's subtotal instead of the metric's total.
+ // `processRecord` (the DB-precomputed path `showValuesAs` drives)
+ // populates these same maps itself -- see its "Metric-collapse
+ // totals" section -- so they resolve there too, not just under
+ // `processResultRecord`'s result-aggregation reducer.
+ let denominatorAggregator: any;
+ if (metricSubstituted === 'col') {
+ denominatorAggregator =
+ type === 'total'
+ ? data.colMetricTotals[this.metricAxis!.value]
+ : data.rowGroupMetricTotals[flatKey(selRow)]?.[
+ this.metricAxis!.value
+ ];
+ } else if (metricSubstituted === 'row') {
+ denominatorAggregator =
+ type === 'total'
Review Comment:
Good catch, applied your suggestion (and the mirrored `selCol` case for `%
of Rows`). Added a test for the blue/red Total column and Total row.
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -1051,8 +1273,57 @@ class PivotData {
);
const vals = this.props.vals as string[];
- const fractionType =
- FRACTION_TYPE_BY_SHOW_VALUES_AS[this.props.showValuesAs as string];
+ // Result aggregation (see resultAggregation.ts): a second aggregation pass
+ // over a metric's own grouped results (e.g. the median of a set of
+ // per-store SUM(sales) values), restoring the pre-SIP-216 "Aggregation
+ // function" choice, computed correctly this time -- `processResultRecord`
+ // below feeds each scope its own original contributing leaf records,
+ // never another scope's already-computed output. `aggregators` already
+ // has a real template for every choice (it's the same dict the
+ // pre-SIP-216 pivot table used); wrap whichever one is selected so a
+ // shared Total/corner slot that ends up seeing more than one metric (see
+ // `makeMixedMetricTracker`) blanks instead of quietly mixing them.
+ const resultAggregation = getResultAggregation(
+ this.props.aggregateFunction,
+ );
+ // `showValuesAs`'s control is hidden once a result aggregation is active
+ // (see controlPanel.tsx) because a result aggregation has its own
+ // "... as Fraction of ..." choices and takes over the computation
+ // entirely -- but hiding the control doesn't reset its stored value, so
+ // a percent choice selected before switching to e.g. "Average" stays in
+ // `this.props.showValuesAs`. Gate `fractionType` on `resultAggregation`
+ // being unset so that stale value can't leak in: left ungated, a leaf
+ // cell would wrap in `fractionOf` and look up a denominator built from
+ // the (non-fraction) result aggregator, which has no `.inner`, and
+ // render blank instead of its actual value.
+ const fractionType = resultAggregation
+ ? undefined
+ : FRACTION_TYPE_BY_SHOW_VALUES_AS[this.props.showValuesAs as string];
+ const resultFactory = resultAggregation
+ ? (...args: unknown[]): Aggregator => {
+ const build = aggregators[resultAggregation] as (
+ v: string[],
+ ) => (...a: unknown[]) => Aggregator;
+ const inner = build(vals)(...args);
+ const innerValue = inner.value.bind(inner);
+ const innerPush = inner.push.bind(inner);
+ const tracker = makeMixedMetricTracker();
+ let mixed = false;
+ return {
+ ...inner,
+ push(record: PivotRecord) {
+ mixed = tracker.sawMixedMetric(record);
+ innerPush(record);
+ },
+ value() {
+ return mixed ? null : innerValue();
+ },
+ sortValue() {
+ return innerValue();
Review Comment:
Added the same blue/red value-sort test under aggregateFunction 'Sum', so
blanking that sortValue now fails it.
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -482,10 +548,28 @@ const baseAggregatorTemplates = {
sum: 0 as any,
currencySet: new Set<string>(),
push(record: PivotRecord) {
- if (Number.isNaN(Number(record[attr]))) {
- this.sum = record[attr];
+ const val = record[attr];
+ // A metric's own value can be a real SQL NULL (e.g. SUM over an
+ // empty group). Number(null) coerces to 0, not NaN, so it would
+ // otherwise fall into the numeric branch below, where
+ // parseFloat(String(null)) ('parseFloat("null")') is NaN and
+ // silently poisons the running sum -- skip it entirely, the same
+ // way a group with no matching leaf record at all is excluded.
+ if (val === null || val === undefined) {
Review Comment:
@sadpandajoe done: the `Sum` aggregator now remembers when every input was a
SQL NULL, and the fraction stays blank for that scope (numerator or
denominator). Mixed scopes still skip the nulls. Test added for
blue=NULL/red=30.
##########
superset/charts/client_processing.py:
##########
@@ -1066,14 +1133,34 @@ def pivot_table_v2(
# totals.
df, rollup_levels = split_grouping_sets_levels(df)
show_values_as = form_data.get("showValuesAs")
+ # A result aggregation (anything but the default "Metric") takes over the
+ # cell/summary computation on the frontend and hides this control in
+ # Explore (see `aggregateFunction`'s `visibility` in controlPanel.tsx and
+ # `resultFactory ?? fractionType` in utilities.ts, where the result
+ # aggregation always wins) -- ignore a stale persisted `showValuesAs`
+ # the same way once a result aggregation is active, rather than applying
+ # a percent transform the live chart no longer shows.
+ aggregate_function_raw = form_data.get("aggregateFunction")
percent_mode = (
- show_values_as if show_values_as in SHOW_VALUES_AS_PERCENT_MODES else
None
+ show_values_as
+ if show_values_as in SHOW_VALUES_AS_PERCENT_MODES
+ and aggregate_function_raw in (None, "Metric")
+ else None
)
+ # "Metric" (the new result-aggregation control's default, meaning "use the
+ # metric's own definition, no second aggregation pass") isn't a key in
+ # pivot_v2_aggfunc_map -- it never needed to be, since this backend path
+ # has no result-aggregation support yet (see #44625's follow-up scope).
Review Comment:
Tracked in #44725, and #44730 brings the export path in line with the
blank-on-mixed behavior.
--
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]
