rusackas commented on code in PR #42895:
URL: https://github.com/apache/superset/pull/42895#discussion_r3822274731
##########
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:
Redshift actually has a native `MEDIAN(x)` function (Postgres doesn't), so I
switched Redshift to emit that directly instead of inheriting Postgres's
`PERCENTILE_CONT ... WITHIN GROUP` spelling. A plain function call has no ORDER
BY clause, so the mixed-aggregate restriction shouldn't apply anymore.
##########
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:
Good catch, added `EXTENDED_METRIC_AGGREGATES` to the `OneOf` choices on
`ChartDataAdhocMetricSchema` so the enum matches what the compiler actually
accepts.
##########
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:
Fixed, the Custom SQL tab now prefills MEDIAN as `PERCENTILE_CONT(0.5)
WITHIN GROUP (ORDER BY col)` instead of the raw `MEDIAN(col)` that Postgres
would reject. The display label still shows the concise `MEDIAN(col)` form.
##########
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:
Added `MEDIAN`/`STDDEV_SAMP`/`VAR_SAMP` to `sqlaAutoGeneratedMetricRegex` so
they round-trip between Simple and Custom SQL like the original six.
--
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]
