rusackas commented on code in PR #42761: URL: https://github.com/apache/superset/pull/42761#discussion_r3723285961
########## superset/migrations/versions/2026-08-05_00-00_1a27941d5352_restore_pivot_table_percent_display.py: ########## @@ -0,0 +1,163 @@ +# 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. +"""restore pivot table percent display from orphaned aggregateFunction + +PR #41184 (SIP-216) removed the Pivot Table's per-table "Aggregation +function" control (form_data field ``aggregateFunction``), including its +"Sum/Count as Fraction of Total/Rows/Columns" options, in favor of +DB-computed totals. Per that PR's own UPDATING.md note, saved charts that +had ``aggregateFunction`` set were deliberately left as-is rather than +migrated: "Saved charts that set aggregateFunction will ignore it; no +migration is required." The field has been fully unused dead weight in +``params``/``query_context`` ever since. + +PR #42761 reintroduces the fraction-display feature as a new, standalone +``showValuesAs`` field. This migration derives ``showValuesAs`` from any +still-present ``aggregateFunction`` fraction value on ``pivot_table_v2`` +charts, so a chart that had this display configured before #41184 shipped +gets it back automatically instead of requiring someone to reopen every +affected chart and reselect it by hand. Charts whose ``aggregateFunction`` +was a non-fraction value (Sum, Average, Count, ...) are left untouched -- +those were never broken by the removal and are out of scope here. + +Only the ``params``/``query_context`` snapshot stored on the slice is +patched. The stored ``query_context`` is a cache mainly used for reports/ +alerts; interactive Explore/dashboard rendering always rebuilds the query +fresh from the current form_data, so this has no effect there. A report or +alert that renders a migrated chart before it is next opened in Explore +will not reflect the restored percent display in its ``query_context`` +until then, but will not error -- ``showValuesAs`` is purely a display +transform for the additive metrics used by the vast majority of pivot +tables. + +Revision ID: 1a27941d5352 +Revises: f3a8c1d2e9b7 +Create Date: 2026-08-05 00:00:00.000000 + +""" + +from alembic import op +from sqlalchemy import Column, Integer, String, Text +from sqlalchemy.orm import declarative_base + +from superset import db +from superset.migrations.shared.utils import paginated_update +from superset.utils import json + +# revision identifiers, used by Alembic. +revision = "1a27941d5352" +down_revision = "f3a8c1d2e9b7" + +Base = declarative_base() + +_VIZ_TYPE = "pivot_table_v2" +_OLD_FIELD = "aggregateFunction" +_NEW_FIELD = "showValuesAs" + +# Old `aggregateFunction` fraction values -> new `showValuesAs` enum values +# (see ShowValuesAsEnum in superset-frontend/.../plugin-chart-pivot-table/src/types.ts). +_FRACTION_MAPPING = { + "Sum as Fraction of Total": "percent_total", + "Count as Fraction of Total": "percent_total", + "Sum as Fraction of Rows": "percent_row", + "Count as Fraction of Rows": "percent_row", + "Sum as Fraction of Columns": "percent_col", + "Count as Fraction of Columns": "percent_col", +} + + +class Slice(Base): # type: ignore + __tablename__ = "slices" + + id = Column(Integer, primary_key=True) + viz_type = Column(String(250)) + params = Column(Text) + query_context = Column(Text) + + +def _migrate_params(slc: Slice) -> bool: + """Derive showValuesAs from an orphaned fraction aggregateFunction in + params. Returns True if params changed.""" + if not slc.params: + return False + try: + params = json.loads(slc.params) + except Exception: + return False + + old_value = params.get(_OLD_FIELD) Review Comment: Good catch, added an `isinstance(params, dict)`/`isinstance(qc, dict)` guard in both helpers so a malformed row gets skipped instead of raising mid-migration. ########## superset/migrations/versions/2026-08-05_00-00_1a27941d5352_restore_pivot_table_percent_display.py: ########## @@ -0,0 +1,163 @@ +# 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. +"""restore pivot table percent display from orphaned aggregateFunction + +PR #41184 (SIP-216) removed the Pivot Table's per-table "Aggregation +function" control (form_data field ``aggregateFunction``), including its +"Sum/Count as Fraction of Total/Rows/Columns" options, in favor of +DB-computed totals. Per that PR's own UPDATING.md note, saved charts that +had ``aggregateFunction`` set were deliberately left as-is rather than +migrated: "Saved charts that set aggregateFunction will ignore it; no +migration is required." The field has been fully unused dead weight in +``params``/``query_context`` ever since. + +PR #42761 reintroduces the fraction-display feature as a new, standalone +``showValuesAs`` field. This migration derives ``showValuesAs`` from any +still-present ``aggregateFunction`` fraction value on ``pivot_table_v2`` +charts, so a chart that had this display configured before #41184 shipped +gets it back automatically instead of requiring someone to reopen every +affected chart and reselect it by hand. Charts whose ``aggregateFunction`` +was a non-fraction value (Sum, Average, Count, ...) are left untouched -- +those were never broken by the removal and are out of scope here. + +Only the ``params``/``query_context`` snapshot stored on the slice is +patched. The stored ``query_context`` is a cache mainly used for reports/ +alerts; interactive Explore/dashboard rendering always rebuilds the query +fresh from the current form_data, so this has no effect there. A report or +alert that renders a migrated chart before it is next opened in Explore +will not reflect the restored percent display in its ``query_context`` +until then, but will not error -- ``showValuesAs`` is purely a display +transform for the additive metrics used by the vast majority of pivot +tables. + +Revision ID: 1a27941d5352 +Revises: f3a8c1d2e9b7 +Create Date: 2026-08-05 00:00:00.000000 + +""" + +from alembic import op +from sqlalchemy import Column, Integer, String, Text +from sqlalchemy.orm import declarative_base + +from superset import db +from superset.migrations.shared.utils import paginated_update +from superset.utils import json + +# revision identifiers, used by Alembic. +revision = "1a27941d5352" +down_revision = "f3a8c1d2e9b7" + +Base = declarative_base() + +_VIZ_TYPE = "pivot_table_v2" +_OLD_FIELD = "aggregateFunction" +_NEW_FIELD = "showValuesAs" + +# Old `aggregateFunction` fraction values -> new `showValuesAs` enum values +# (see ShowValuesAsEnum in superset-frontend/.../plugin-chart-pivot-table/src/types.ts). +_FRACTION_MAPPING = { + "Sum as Fraction of Total": "percent_total", + "Count as Fraction of Total": "percent_total", Review Comment: Confirmed, Count-fraction divided a record count while the new `percent_*` modes divide the metric's own value. Dropped the three Count variants from the mapping, they're left un-migrated same as Sum/Average/Count. -- 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]
