sadpandajoe commented on code in PR #42761: URL: https://github.com/apache/superset/pull/42761#discussion_r3722988374
########## 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: A chart whose `params` contains valid non-object JSON such as `[]` or `null` raises here, so one historically malformed pivot slice aborts `superset db upgrade` after earlier batches may already have committed. Should this verify the decoded value is a dict before calling `.get()` (and do the same for `query_context`)? ########## 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: The old Count fraction modes divided record counts, while the new `percent_*` modes divide the metric values, so this migration can change a saved chart from 50%/50% to 10%/90%. Should the Count variants be left unmigrated or translated in a way that preserves their prior meaning? ########## superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts: ########## @@ -242,6 +243,7 @@ export default function transformProps(chartProps: ChartProps<QueryFormData>) { columnFormats, currencyFormats, metricsLayout, + showValuesAs, Review Comment: This display mode reaches only the React renderer; POST_PROCESSED CSV/XLSX and scheduled reports still run `pivot_table_v2()` with `aggregateFunction` and therefore export raw values while Explore shows percentages. Should the backend export path also consume `showValuesAs`, or should the control make that discrepancy explicit? ########## superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts: ########## @@ -748,14 +748,67 @@ const baseAggregatorTemplates = { type ], inner: wrapped(...Array.from(x || []))(data, rowKey, colKey), + // The metric this cell belongs to, and which axis carries it (see the + // "Metric" pseudo-dimension in PivotTableChart). Captured from the + // first pushed record. With multiple metrics, the axis holding the + // metric is never actually empty, so collapsing it to `[]` (as the + // `selector` above does) would route every metric's lookup to the + // same shared total slot -- see `processRecord`'s "Metric-collapse + // totals". Keeping the metric's own key segment instead routes the + // lookup to the per-metric total that's already correctly split out. + metricAxis: undefined as + | { axis: 'row' | 'col'; value: string } + | null + | undefined, push(record: PivotRecord) { + if (this.metricAxis === undefined) { + const metricDim = record.__metricKey as unknown as + | string + | undefined; + const cols = data.props.cols as string[] | undefined; + const rows = data.props.rows as string[] | undefined; + if (metricDim && cols?.includes(metricDim)) { + this.metricAxis = { + axis: 'col', + value: String(record[metricDim]), + }; + } else if (metricDim && rows?.includes(metricDim)) { + this.metricAxis = { + axis: 'row', + value: String(record[metricDim]), + }; + } else { + this.metricAxis = null; + } + } this.inner.push(record); }, format: fmtNonString(formatter), value() { - const acc = data - .getAggregator(...Array.from(this.selector || [])) - .inner.value(); + // `buildGroupbyCombinations` requests the denominator's rollup + // level whenever a percent `showValuesAs` is selected, but fall + // back to `null` (rendered blank) instead of throwing if it is + // ever missing -- e.g. a denominator aggregator with no matching + // rows in the response. + let [selRow, selCol] = (this.selector || [[], []]) as [ + string[], + string[], + ]; + if (this.metricAxis) { + if (this.metricAxis.axis === 'col' && selCol.length === 0) { + selCol = [this.metricAxis.value]; + } else if ( + this.metricAxis.axis === 'row' && + selRow.length === 0 + ) { + selRow = [this.metricAxis.value]; + } + } + const denominatorAggregator = data.getAggregator(selRow, selCol); + if (!denominatorAggregator.inner) { Review Comment: A database `NULL` metric value is intentionally blank in actual mode, but `null / acc` becomes `0`, so fraction mode displays `0.0%` and turns an undefined value into a measured zero. Should this return `null` when `this.inner.value()` is null? ########## superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/utilities.ts: ########## @@ -748,14 +748,67 @@ const baseAggregatorTemplates = { type ], inner: wrapped(...Array.from(x || []))(data, rowKey, colKey), + // The metric this cell belongs to, and which axis carries it (see the + // "Metric" pseudo-dimension in PivotTableChart). Captured from the + // first pushed record. With multiple metrics, the axis holding the + // metric is never actually empty, so collapsing it to `[]` (as the + // `selector` above does) would route every metric's lookup to the + // same shared total slot -- see `processRecord`'s "Metric-collapse + // totals". Keeping the metric's own key segment instead routes the + // lookup to the per-metric total that's already correctly split out. + metricAxis: undefined as + | { axis: 'row' | 'col'; value: string } + | null + | undefined, push(record: PivotRecord) { + if (this.metricAxis === undefined) { Review Comment: This captures the first metric for shared Total slots, but `inner` keeps the last pushed metric, so multi-metric percent charts can render cross-metric totals such as `rowTotal(metricB) / rowTotal(metricA)`. Should shared Total/corner cells be blanked in fraction mode or keep the metric key consistent with the retained numerator? -- 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]
