rusackas commented on code in PR #43027:
URL: https://github.com/apache/superset/pull/43027#discussion_r3761198596


##########
superset-frontend/plugins/plugin-chart-table/src/buildQuery.ts:
##########
@@ -350,6 +351,7 @@ export const buildQuery: BuildQuery<TableChartFormData> = (
       extraQueries.push({
         ...queryObject,
         columns: [],
+        metrics: getTotalsMetrics(metrics, formData.totals_aggregate ?? 'SUM'),

Review Comment:
   Added a clamp in both plugins' `buildQuery.ts` so anything other than 
`'AVG'` falls back to `'SUM'` before it reaches `getTotalsMetrics`. Backend 
already rejects an unknown aggregate via `sqla_aggregations`, but this keeps 
the frontend from ever sending one.



##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -694,6 +695,7 @@ export const buildQueryUncached: 
BuildQuery<TableChartFormData> = (
       formData.show_totals &&
       queryMode === QueryMode.Aggregate,
     );
+    const totalsAggregate = formData.totals_aggregate ?? 'SUM';

Review Comment:
   Same clamp added here, same story as the table plugin thread. Falls back to 
`'SUM'` for anything other than `'AVG'`.



##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/buildQuery.ts:
##########
@@ -728,14 +730,18 @@ export const buildQueryUncached: 
BuildQuery<TableChartFormData> = (
       extraQueries.push({
         ...queryObject,
         columns: [],
-        ...(rawSummaryColumns.length > 0 && {
-          metrics: rawSummaryColumns.map(columnName => ({
-            expressionType: 'SIMPLE' as const,
-            aggregate: 'SUM' as const,
-            column: { column_name: columnName },
-            label: columnName,
-          })),
-        }),
+        ...(rawSummaryColumns.length > 0
+          ? {
+              metrics: rawSummaryColumns.map(columnName => ({
+                expressionType: 'SIMPLE' as const,
+                aggregate: totalsAggregate,
+                column: { column_name: columnName },
+                label: columnName,
+              })),
+            }
+          : showAggregateTotals
+            ? { metrics: getTotalsMetrics(metrics ?? [], totalsAggregate) }
+            : {}),

Review Comment:
   Pulled the metrics computation out into a single `totalsMetrics` variable 
above the query object and spread it once. Should read cleaner now.



-- 
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]

Reply via email to