sadpandajoe commented on code in PR #42895:
URL: https://github.com/apache/superset/pull/42895#discussion_r3821444384
##########
superset-frontend/src/explore/constants.ts:
##########
@@ -23,11 +23,22 @@ export const AGGREGATES = {
COUNT: 'COUNT',
COUNT_DISTINCT: 'COUNT_DISTINCT',
MAX: 'MAX',
+ MEDIAN: 'MEDIAN',
Review Comment:
Adding `MEDIAN` here also makes Simple metrics prefill `MEDIAN(column)` when
users switch to Custom SQL. PostgreSQL support added by this PR requires
`PERCENTILE_CONT(0.5) WITHIN GROUP (...)`, so editing and saving that prefilled
SQL creates a raw metric PostgreSQL rejects. Should this conversion avoid
emitting an invalid generic MEDIAN expression?
##########
superset/db_engine_specs/postgres.py:
##########
@@ -192,6 +194,24 @@ class PostgresBaseEngineSpec(BaseEngineSpec):
TimeGrain.YEAR: "DATE_TRUNC('year', {col})",
}
+ # Verified against a live postgres:16 instance, including under GROUPING
+ # SETS (the pivot table's non-additive-total rollup pattern): the grand
+ # total correctly reflects every row, not an aggregate-of-aggregates.
+ # Inherited by Redshift (a Postgres fork); its SQL function reference
+ # documents the same PERCENTILE_CONT/STDDEV_SAMP/VAR_SAMP support, but
+ # that has not been separately verified against a live Redshift instance.
+ # Also inherited by TimescaleDB (a Postgres extension, not a forked query
+ # engine -- it runs unmodified Postgres aggregate execution) and by
+ # Aurora PostgreSQL / its Data API variant (AWS's wire- and
+ # SQL-compatible managed Postgres). Engines that share the SQL dialect
+ # but run a materially different query engine (CockroachDB, Greenplum,
+ # SAP HANA) reset this to `{}` instead -- see those engine specs.
+ _extended_aggregations: dict[str, Callable[[ColumnElement],
ColumnElement]] = {
+ "MEDIAN": lambda col: sa.func.percentile_cont(0.5).within_group(col),
Review Comment:
This enables `MEDIAN` on Redshift even though the new implementation emits
`PERCENTILE_CONT ... WITHIN GROUP`. A chart with `MEDIAN(sales)` and
`MEDIAN(margin)` produces different `ORDER BY` clauses, which Redshift rejects
for sort-based aggregates. Should Redshift opt out of `MEDIAN` until query
construction can satisfy that restriction?
##########
superset/utils/core.py:
##########
@@ -184,6 +186,15 @@ class AdhocMetricExpressionType(StrEnum):
SQL = "SQL"
+# Aggregates with no safe, universal cross-dialect spelling -- unlike
+# SUM/COUNT/AVG/MIN/MAX/COUNT_DISTINCT, whose SQL is generated the same way on
+# every engine. Support for these is opt-in per `BaseEngineSpec` (see
+# `get_extended_aggregation_func`); used to distinguish a genuinely invalid
+# aggregate name from one that is valid but unsupported on the current
database,
+# for a clearer user-facing error.
+EXTENDED_METRIC_AGGREGATES = frozenset({"MEDIAN", "STDDEV_SAMP", "VAR_SAMP"})
Review Comment:
The chart-data REST schema still restricts `aggregate` to the original six
values, so Swagger/generated clients reject requests using these newly
supported aggregates even though the compiler accepts them. Should
`ChartDataAdhocMetricSchema` and the generated OpenAPI artifact be updated with
the same enum?
##########
superset-frontend/src/explore/constants.ts:
##########
@@ -23,11 +23,22 @@ export const AGGREGATES = {
COUNT: 'COUNT',
COUNT_DISTINCT: 'COUNT_DISTINCT',
MAX: 'MAX',
+ MEDIAN: 'MEDIAN',
MIN: 'MIN',
+ STDDEV_SAMP: 'STDDEV_SAMP',
Review Comment:
These new aggregates can be selected in Simple mode, but
`sqlaAutoGeneratedMetricRegex` still only recognizes `SUM|AVG|MAX|MIN|COUNT`.
Opening `STDDEV_SAMP(amount)` as Custom SQL and switching to Simple leaves both
selectors blank instead of round-tripping the metric. Should the inference
regex include the new simple aggregates?
--
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]
