sadpandajoe commented on code in PR #44657: URL: https://github.com/apache/superset/pull/44657#discussion_r4154322949
########## superset-frontend/src/explore/components/LegacyAggregationAlert.tsx: ########## @@ -0,0 +1,106 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useEffect, useState, useCallback } from 'react'; +import { useSelector } from 'react-redux'; +import { t } from '@apache-superset/core/translation'; +import { logging } from '@apache-superset/core/utils'; +import { css } from '@apache-superset/core/theme'; +import { TagType } from 'src/components'; +import { fetchTags, deleteTaggedObjects } from 'src/features/tags/tags'; +import { LEGACY_AGGREGATION_TAG } from 'src/explore/constants'; +import { ExploreAlert } from './ExploreAlert'; + +interface LegacyAggregationAlertProps { + sliceId?: number; +} + +export const LegacyAggregationAlert = ({ + sliceId, +}: LegacyAggregationAlertProps) => { + const [tag, setTag] = useState<TagType | null>(null); + // A same-slice save is the other way (alongside this component's own + // "Accept" button) a legacy aggregation tag can go away -- saveModalActions + // deletes it server-side as part of a pivot_table_v2 save. This object + // changes identity on every successful save, so including it below + // re-fetches after a save and drops a tag the save already cleared, + // instead of leaving this component's local state stale until the user + // navigates away and back. + const lastSaveResult = useSelector( + (state: { saveModal?: { data?: unknown } }) => state.saveModal?.data, + ); + + useEffect(() => { + setTag(null); + if (!sliceId) { + return undefined; + } + // Guards the async callbacks below against a stale in-flight request -- + // e.g. the user switches charts (`sliceId` changes) or saves again + // (`lastSaveResult` changes) before the first request resolves. + let cancelled = false; + fetchTags( + { objectType: 'chart', objectId: sliceId }, + (tags: TagType[]) => { + if (cancelled) { + return; + } + setTag(tags.find(t => t.name === LEGACY_AGGREGATION_TAG) ?? null); + }, + error => { + if (!cancelled) { + logging.warn('Failed to fetch chart tags', error); + } + }, + ); + return () => { + cancelled = true; + }; + }, [sliceId, lastSaveResult]); + + const removeTag = useCallback(() => { + if (!sliceId || !tag) { + return; + } + deleteTaggedObjects( + { objectType: 'chart', objectId: sliceId }, + tag, + () => setTag(null), Review Comment: If Accept starts a slow DELETE for chart A and the user switches to tagged chart B before it completes, this callback clears B’s newly fetched notice even though only A’s tag was removed. Could the completion be guarded against a changed slice/tag, with a delayed-DELETE chart-switch regression test? ########## superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx: ########## @@ -243,6 +250,34 @@ const config: ControlPanelConfig = { }, }, ], + [ + { + name: 'aggregateFunction', Review Comment: UPDATING.md still says this control is removed and saved non-fraction aggregateFunction values are unaffected, but this restoration reactivates them automatically. An operator upgrading a saved Median chart would be told its totals cannot change; could the upgrade guidance describe the restored behavior and the need to validate tagged charts? ########## superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts: ########## @@ -796,20 +912,82 @@ 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) { + let denominatorAggregator: any = data.getAggregator(selRow, selCol); + // The depth-gated tree only has a node at the substituted + // position above when the corresponding axis's subtotals happen + // to be on -- e.g. `type: 'row'` with Metric on columns needs a + // (row, metric) tree node that only exists if column subtotals + // are enabled. `rowGroupMetricTotals`/`colGroupMetricTotals` (see + // PivotData) track exactly that scope independently of subtotal + // visibility, so they're the fallback for 'row'/'col'. + // `rowMetricTotals`/`colMetricTotals` are the dataset-wide + // equivalent, for 'total'. Metric-definition mode + // (`showValuesAs`) never populates any of these maps regardless. + if ( + (!denominatorAggregator || !denominatorAggregator.inner) && + this.metricAxis Review Comment: With Combine metrics and column subtotals enabled, a category value matching a metric label makes this lookup use the category subtotal instead of that metric’s total. For categories sales/other and metrics sales/cost, sales values 10 and 20 plus cost=90 in category sales yield 10/(10+90)=10% rather than 10/(10+20)=33.3%; could the substituted metric scope use the per-metric maps directly? ########## superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts: ########## @@ -1065,15 +1302,48 @@ class PivotData { // themselves to read 100%. This needs no new query and no per-metric // aggregator-override control (that control is gone, see SIP.md); it's a // pure display transform over values that are already DB-correct. - this.aggregator = fractionType + const plainAggregator = fractionType ? aggregatorTemplates.fractionOf( cellValue(), fractionType, usFmtPct, )(vals) : cellValue(this.props.defaultFormatter as Formatter)(vals); + this.aggregator = resultFactory ?? plainAggregator; + // A result aggregation reduces the *rollup* scopes (subtotals, row/col + // totals, the grand total) over a metric's own leaf-level results -- it + // never reduces the leaf cells themselves (see resultAggregation.ts), so + // `leafAggregator` stays this plain, DB-verbatim aggregator even when + // `this.aggregator` above is a reducer. + this.leafAggregator = plainAggregator; + this.isFractionResult = isFractionResultAggregation(resultAggregation); Review Comment: Selecting a percent display and then Average leaves the hidden showValuesAs value active in this leaf aggregator. Its denominator is now an Average aggregator without an inner, so ordinary leaf cells render blank; could result aggregation also suppress the stale percent transform when choosing the leaf aggregator? ########## superset/charts/client_processing.py: ########## @@ -1069,11 +1069,20 @@ def pivot_table_v2( percent_mode = ( show_values_as if show_values_as in SHOW_VALUES_AS_PERCENT_MODES 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). + # Treat it, and any other value this map doesn't recognize, the same way + # an absent field always has been: fall back to "Sum". + aggregate_function = form_data.get("aggregateFunction") Review Comment: The pivot computation now ignores the stale percent setting, but the XLSX writer still selects EXCEL_PERCENT_FORMAT from raw showValuesAs at client_processing.py:1476–1481. With Average and a saved percent_row setting, a numeric export value of 10 therefore displays as 1000.0% in Excel; could that format decision use the same effective-percent guard? ########## superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts: ########## @@ -1053,6 +1230,41 @@ 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, + ); + const resultFactory = resultAggregation + ? (...args: unknown[]): Aggregator => { + const build = aggregators[resultAggregation] as ( Review Comment: Per-metric overrides are preserved now, but the chart-wide valueFormat/currencyFormat still falls back to the fixed US formatter for summaries when no metric override exists: resultFactory builds from aggregators and only buildCustomFormattedAggregator replaces format. A chart-wide currency format therefore remains on leaves but disappears from Average/Median summaries; could the non-fraction reducer also inherit defaultFormatter? -- 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]
