aminghadersohi commented on code in PR #44657: URL: https://github.com/apache/superset/pull/44657#discussion_r4249214543
########## superset/migrations/versions/2026-09-25_00-00_141b8ada7731_tag_pivot_tables_with_restored_.py: ########## @@ -0,0 +1,264 @@ +# 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. +"""tag pivot tables with a restored aggregateFunction for review + +PR #41184 (SIP-216) removed the Pivot Table's per-table "Aggregation +function" control (form_data field ``aggregateFunction``) and left it as +orphaned, ignored dead data on any chart that had it set -- per that PR's +own UPDATING.md note, no migration was done at the time. + +A later change restores the same control as "result aggregation": a second +aggregation pass over a metric's own grouped results, computed correctly +this time (see docs/sip -- not repeating the pre-SIP-216 bug of +re-aggregating already-displayed values). It deliberately reuses the exact +same form_data field name and the exact same value spellings the old +control used, so no value needs to change for a saved chart to pick it back +up -- the moment that code ships, any chart with e.g. +``aggregateFunction: "Median"`` already sitting in ``params`` starts +computing its totals with the new, correct "Median" result aggregation +again, automatically. + +That is by design and does not need admin review to be *safe*, but a +chart's totals silently changing shape on upgrade is still worth a +customer-facing heads-up, not silence. This migration does not touch +``aggregateFunction`` or any other value -- there is nothing to migrate -- +it only: + +1. Tags every ``pivot_table_v2`` chart whose ``aggregateFunction`` is one of + the pre-SIP-216 values (i.e. would now activate result aggregation) with + a ``LEGACY_AGGREGATION_TAG`` custom tag, so affected charts are + queryable and the frontend can surface a one-time "these totals were + restored, please validate" notice (removed automatically the first time + the chart is opened and accepted, or saved). + +This migration deliberately leaves the chart's cached ``query_context`` +snapshot alone rather than clearing it. That cache predates the feature +entirely (it was built under the old, GROUPING-SETS-only query shape), so it +will keep serving a stale, pre-restoration result to a report or alert until +the chart is next opened and re-saved in Explore -- the same caveat #42761's +migration accepted for its own, narrower fraction-value migration. Unlike +that stale-but-working tradeoff, clearing ``query_context`` outright (an +earlier version of this migration did) turns that into a hard failure +instead: ``ChartWarmUpCacheCommand`` requires a query context to exist and +raises rather than falling back to a fresh build, so every tagged chart +would error out of scheduled cache warm-up -- which never opens Explore -- +until a human manually opens and re-saves it. A stale result is exactly what +the tag/notice already exist to prompt a human to fix; an outage isn't. + +Revision ID: 141b8ada7731 +Revises: 884a2115ebd3 +Create Date: 2026-09-25 00:00:00.000000 + +""" + +from datetime import datetime + +from alembic import op +from sqlalchemy import Column, DateTime, Enum, Integer, String, Text +from sqlalchemy.orm import declarative_base + +from superset import db +from superset.migrations.shared.utils import paginated_update +from superset.tags.models import ObjectType, TagType +from superset.utils import json + +# revision identifiers, used by Alembic. +revision = "141b8ada7731" +down_revision = "884a2115ebd3" + +Base = declarative_base() + +_VIZ_TYPE = "pivot_table_v2" +_FIELD = "aggregateFunction" +# The control's default, meaning "no second aggregation pass". +_DEFAULT_RESULT_AGGREGATION = "Metric" + +# Name of the custom tag applied to an affected chart. Kept in sync with +# `LEGACY_AGGREGATION_TAG` in +# superset-frontend/src/explore/components/LegacyAggregationAlert.tsx, +# which reads and clears it. +LEGACY_AGGREGATION_TAG = "legacy-pivot-aggregation-restored" + +# The pre-SIP-216 "Aggregation function" values -- kept in sync with +# RESULT_AGGREGATIONS in +# superset-frontend/.../plugin-chart-pivot-table/src/plugin/resultAggregation.ts. +# Any chart whose orphaned `aggregateFunction` is one of these will start +# computing its totals with that result aggregation again, automatically, +# the moment the restoring code ships -- this migration only tags it. +_LEGACY_AGGREGATE_FUNCTIONS = frozenset( + { + "Count", + "Count Unique Values", + "List Unique Values", + "Sum", Review Comment: The reworded notice no longer claims the setting was ignored, so it reads true both for 6.1 upgraders and for builds that orphaned the setting. ########## 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: Deferring is fine: at 4f46839 the export matches master for Median/Average/Sum and no longer raises for Count/Variance. One correction for https://github.com/apache/superset/issues/45215: every result aggregation is already in `pivot_v2_aggfunc_map`; the export re-applies it to every cell, leaves included (Count leaf 1, Variance leaf 0), rather than falling back to Sum. -- 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]
