sadpandajoe commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4171504423
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx:
##########
@@ -243,6 +250,34 @@ const config: ControlPanelConfig = {
},
},
],
+ [
+ {
+ name: 'aggregateFunction',
+ config: {
+ type: 'SelectControl',
+ label: () => t('Aggregation function'),
+ default: 'Metric',
Review Comment:
Saving an ordinary pivot now persists aggregateFunction: "Metric" in params
and query_context, but rolling back to the previous code makes its CSV/XLSX
processing look up that nonexistent aggregation and raise KeyError. Could the
default remain absent in persisted settings, or could downgrade normalize both
stored representations?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts:
##########
@@ -1051,8 +1252,54 @@ 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();
Review Comment:
With two metrics and Minimum selected, mixed-metric totals return null here
but inherit the extrema formatter, which returns null unchanged; TableRenderer
then displays the literal “null” instead of a blank. Could null be handled at
the formatting boundary too, including custom formatter overrides, with a
rendered mixed-metric Minimum total assertion?
##########
superset/charts/client_processing.py:
##########
@@ -817,9 +817,9 @@ def union_currency_context(
"Sum": pd.Series.sum,
"Average": pd.Series.mean,
"Median": pd.Series.median,
- "Sample Variance": lambda series: pd.series.var(series) if len(series) > 1
else 0,
+ "Sample Variance": lambda series: pd.Series.var(series) if len(series) > 1
else 0,
Review Comment:
The new backend tests never select either corrected sample-statistics
aggregation, so they cannot catch the pd.series error or the non-callable
standard-deviation tuple returning. Could a parameterized pivot_table_v2 test
use values 10 and 20 at one pivot address and assert variance 50 and standard
deviation approximately 7.071 with number formatting disabled?
##########
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:
The chart-switch race still remains: the migration attaches the same
globally unique tag to every affected chart, so A and B have the same tag ID
and this check still clears B’s notice. Could the callback guard the
chart/request identity, and could the delayed-DELETE test use the same tag ID
for both charts and await the callback before asserting?
##########
UPDATING.md:
##########
@@ -1519,9 +1517,25 @@ Fraction of ..." variants are **not** migrated: they
divided a record count,
while the new control divides the metric's own value, so translating them
automatically would silently change what the chart displays rather than
restore it; those charts need to be manually reconfigured if the value-based
-percentage is what's wanted. Charts that used any other non-fraction
-`aggregateFunction` value (Sum, Average, Count, ...) are unaffected, since
-that specific behavior remains unavailable per the above.
+percentage is what's wanted.
+
+The **"Aggregation function"** control itself (form_data field
+`aggregateFunction`) is also back, as a second aggregation pass over a
+metric's own grouped results -- e.g. the median of a set of per-store
+`SUM(sales)` values -- computed correctly this time: every scope (subtotal,
+row/column total, grand total) reduces its own original contributing query
+results, never another scope's already-displayed value, so the previous
+non-additive-metric bug this section originally removed the control for does
+not return. It reuses the exact same field name and value spellings as
+before, so a chart that still had e.g. `aggregateFunction: "Median"` sitting
+unused in its saved `params` starts computing totals with that aggregation
+again automatically, with no action required and no value to reconfigure. A
+one-time migration tags every such chart with a
`legacy-pivot-aggregation-restored`
+custom tag; opening a tagged chart in Explore shows a notice that its totals
+may now look different, which clears once you review and accept it (or save
+the chart). If you have alerts/reports built on one of these charts, validate
+its cached results after upgrading, since a scheduled report render does not
Review Comment:
Accept only removes the tag; it does not rebuild the saved query_context,
which scheduled consumers keep using even after their result cache is
refreshed. Could the upgrade instructions explicitly require re-saving affected
charts in Explore before validating scheduled outputs, rather than leaving
Accept and Save sounding interchangeable?
--
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]
