villebro commented on code in PR #45408:
URL: https://github.com/apache/superset/pull/45408#discussion_r4303793278
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/utilities.ts:
##########
@@ -36,13 +37,22 @@ const ADDITIVE_AGGREGATES = new Set(['SUM', 'COUNT', 'MIN',
'MAX']);
* Whether a metric's total can be derived additively from per-group results.
* Conservative: only SIMPLE metrics with a known additive aggregate qualify;
* saved-metric references (strings) and SQL/adhoc metrics are treated as
- * non-additive because their aggregate cannot be determined from form data.
+ * non-additive because their aggregate cannot be determined from form data,
+ * and MIN/MAX only qualify on a column known to be numeric (see below).
*/
export function isAdditiveMetric(metric: QueryFormMetric): boolean {
if (typeof metric === 'string' || metric.expressionType !== 'SIMPLE') {
return false;
}
- return !!metric.aggregate && ADDITIVE_AGGREGATES.has(metric.aggregate);
+ if (!metric.aggregate || !ADDITIVE_AGGREGATES.has(metric.aggregate)) {
+ return false;
+ }
+ // The fast-path reduces values with Number(), which would blank a MIN/MAX
+ // over a text or temporal column, so only numeric columns may take it.
+ if (metric.aggregate === 'MIN' || metric.aggregate === 'MAX') {
+ return metric.column?.type_generic === GenericDataType.Numeric;
+ }
+ return true;
Review Comment:
As MIN/MAX aren't really additive metrics, should we consider renaming the
helper to `isClientAggregatableMetric` or something similar?
--
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]
