rusackas commented on code in PR #43027: URL: https://github.com/apache/superset/pull/43027#discussion_r3761916175
########## superset-frontend/packages/superset-ui-chart-controls/src/utils/getTotalsMetrics.ts: ########## @@ -0,0 +1,43 @@ +/** + * 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 { isAdhocMetricSimple, QueryFormMetric } from '@superset-ui/core'; + +export type TotalsAggregate = 'SUM' | 'AVG'; + +/** + * Build the metrics for a chart's "Show summary" totals query, overriding + * each Simple (adhoc) metric's aggregate function with the user-chosen + * totals aggregate. The totals query has no GROUP BY, so the database + * evaluates each metric fresh over all rows -- swapping the aggregate here + * is a correct, independent computation, not a re-aggregation of + * already-aggregated per-row values. + * + * Custom-SQL metrics and saved (string) metrics pass through unchanged: + * there is no safe way to rewrite an arbitrary SQL expression's aggregate + * function without parsing it, so the totals row keeps their own native + * aggregate for those. + */ +export function getTotalsMetrics( + metrics: QueryFormMetric[], + aggregate: TotalsAggregate, +): QueryFormMetric[] { + return metrics.map(metric => + isAdhocMetricSimple(metric) ? { ...metric, aggregate } : metric, + ); Review Comment: Fair point, but it's not really AVG-specific, the SUM default has the same failure mode since this PR is what introduces the override at all (metrics used to keep their own native aggregate before this). Guarding it properly needs datasource column-type info that `getTotalsMetrics` doesn't have, just the metric shape. Real edge case, but more of a follow-up than a quick patch here. ########## tests/integration_tests/non_additive_totals_tests.py: ########## @@ -189,6 +189,48 @@ def test_backend_computes_percent_column_for_summary_query(self): assert totals_df["sum__num_pct"].iloc[0] == pytest.approx(1.0) [email protected]("load_birth_names_dashboard_with_slices") +class TestTableTotalsAggregateOverride(SupersetTestCase): + """ + #43021: "Show summary" lets a user choose the totals row's aggregation + (Sum or Average) independently of each metric's own aggregation. The + frontend (``getTotalsMetrics``) implements this by cloning a SIMPLE + metric with its ``aggregate`` swapped for just the totals query. Since + that query has no GROUP BY, the database evaluates the swapped + aggregate fresh over every row -- this guard pins that an AVG override + is a true row-level average (SUM / COUNT over all rows), not the + metric's own SUM aggregation and not a naive average of per-group sums. + """ + + def test_avg_totals_aggregate_matches_sum_over_count(self): + self.login("admin") + + sum_metric = { + "expressionType": "SIMPLE", + "column": {"column_name": "num"}, + "aggregate": "SUM", + "label": "sum__num", + } + count_metric = { + "expressionType": "SIMPLE", + "column": {"column_name": "num"}, + "aggregate": "COUNT", + "label": "count__num", + } + avg_metric = {**sum_metric, "aggregate": "AVG", "label": "avg__num"} + + total_sum = _result_df(_base_payload(sum_metric, []))["sum__num"].iloc[0] + total_count = _result_df(_base_payload(count_metric, []))["count__num"].iloc[0] + avg_total = _result_df(_base_payload(avg_metric, []))["avg__num"].iloc[0] Review Comment: The wiring itself is covered elsewhere: `getTotalsMetrics.test.ts` and both plugins' `buildQuery.test.ts` assert `totals_aggregate` flows through to the query. This test's job, per its own docstring, is narrower, pin that an AVG override is a true row-level average and not a naive per-group average, which needs a real DB to prove. So it's doing what it says on the tin. -- 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]
