rusackas commented on code in PR #44657:
URL: https://github.com/apache/superset/pull/44657#discussion_r4154848812
##########
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:
Good catch, fixed. The metric-substituted denominator now goes straight to
`colMetricTotals`/`rowMetricTotals` (or the group variants) instead of the
generic keyed tree, so a category value matching a metric name can't collide
with it anymore.
##########
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:
Good catch, fixed. `fractionType` is now gated on `resultAggregation` being
unset, so a stale percent choice can't leak into the leaf aggregator once
something like Average is selected.
##########
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:
Good catch, fixed. The Accept callback now only clears the tag it actually
deleted, so a slower DELETE can't wipe out a different chart's tag after a
switch.
--
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]
