Copilot commented on code in PR #42895:
URL: https://github.com/apache/superset/pull/42895#discussion_r3761218041
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -790,10 +790,15 @@ class ColumnRef(UnknownFieldCheckMixin):
"MIN",
"MAX",
"COUNT_DISTINCT",
- "STDDEV",
- "VAR",
+ "STDDEV_SAMP",
+ "VAR_SAMP",
"MEDIAN",
"PERCENTILE",
+ # Pre-SIP shorthand, accepted and normalized to the names above by
+ # `chart_utils.create_metric_object`; kept here so schema
+ # validation doesn't reject them before that normalization runs.
+ "STDDEV",
+ "VAR",
Review Comment:
`ColumnRef.aggregate` still allows `PERCENTILE`, but the query compilation
path for SIMPLE metrics only supports `SqlaTable.sqla_aggregations` +
`BaseEngineSpec.get_extended_aggregation_func`; `PERCENTILE` is in neither, so
MCP requests can pass schema validation yet fail later with "Adhoc metric
aggregate is invalid". Since the guided prompt no longer advertises
`PERCENTILE` and the SIP doc notes it’s unimplemented, it’s safer to reject it
at the schema level until a parameterized percentile aggregate is implemented.
##########
superset/db_engine_specs/mysql.py:
##########
@@ -90,6 +92,17 @@ class MySQLEngineSpec(BasicParametersMixin, BaseEngineSpec):
supports_dynamic_schema = True
supports_multivalues_insert = True
+ # Verified against a live mysql:8.0 instance, including under GROUP BY ...
+ # WITH ROLLUP. `STDDEV_SAMP`/`VAR_SAMP` are native, correct sample
+ # statistics. MEDIAN is deliberately absent: MySQL has neither a `MEDIAN`
+ # function nor `PERCENTILE_CONT` (confirmed: both error). Its `VARIANCE()`
+ # function is population variance, not sample variance, so it is not a
+ # valid stand-in for VAR_SAMP either.
+ _extended_aggregations: dict[str, Callable[[ColumnElement],
ColumnElement]] = {
+ "STDDEV_SAMP": sa.func.stddev_samp,
+ "VAR_SAMP": sa.func.var_samp,
+ }
Review Comment:
Defining `_extended_aggregations` on `MySQLEngineSpec` implicitly enables
these aggregates for every `MySQLEngineSpec` subclass (e.g. MariaDB and Aurora
MySQL engine specs), even though the comment says the expressions were verified
against a live *MySQL* instance. If the intent is strict opt-in per concrete
engine (as the SIP text suggests), the MySQL-family subclasses should either
override `_extended_aggregations = {}` until verified, or there should be a
test explicitly asserting which subclasses are allowed to inherit.
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -610,12 +610,19 @@ def create_metric_object(col: ColumnRef) -> Dict[str,
Any] | str:
"MIN",
"MAX",
"COUNT_DISTINCT",
- "STDDEV",
- "VAR",
+ "STDDEV_SAMP",
+ "VAR_SAMP",
"MEDIAN",
"PERCENTILE",
}
Review Comment:
`create_metric_object` treats `PERCENTILE` as a valid SIMPLE metric
aggregate, but the backend metric compiler doesn’t implement `PERCENTILE` (it’s
not in `sqla_aggregations` and it isn’t part of `EXTENDED_METRIC_AGGREGATES`).
That means MCP-generated charts can still end up with an aggregate that will
never compile. Consider removing it from the MCP-valid aggregate set until it’s
actually supported.
##########
UPDATING.md:
##########
@@ -26,6 +26,20 @@ assists people when migrating to a new version.
- [42393](https://github.com/apache/superset/pull/42393): Exported dataset
YAML now carries a `uuid` for each metric and column so that custom folder
assignments (which reference metrics/columns by UUID) survive an import into
another workspace. This affects any export bundle that contains datasets, not
just a dataset export: chart, dashboard, database and full-asset exports all
embed the same dataset YAML, so a dashboard exported from this release also
fails to import into an older one even though no dataset was exported directly.
As with `folders` and `currency_code_column`, the affected `datasets/` files
fail schema validation (`Unknown field: uuid`) when imported into Superset
releases that predate this change; regenerate or hand-edit exports for older
targets in mixed-version fleets.
+### New metric aggregates: MEDIAN, Sample Standard Deviation, Sample Variance
+
+`MEDIAN`, `STDDEV_SAMP`, and `VAR_SAMP` are now available anywhere a metric
+aggregate is chosen (every chart type, SQL Lab, MCP), not only in Pivot
+Table's controls. Support is opt-in per database engine, verified against a
+live instance before being enabled: Postgres, MySQL (`STDDEV_SAMP`/`VAR_SAMP`
+only, no `MEDIAN`), DuckDB, and Redshift (inherits Postgres's support, not yet
+separately verified) ship enabled in this release. Picking one of these
Review Comment:
The UPDATING entry says extended aggregates ship enabled for Postgres,
MySQL, DuckDB, and Redshift, but the implementation enables them on the *engine
spec classes* (e.g. `MySQLEngineSpec` / `PostgresBaseEngineSpec`). As a result,
MySQL/Postgres-compatible engine specs that inherit from those (e.g. MariaDB,
Aurora MySQL/Postgres, TimescaleDB) may also be enabled unless they override
`_extended_aggregations = {}`. Please either clarify the wording here (e.g.
“MySQL/Postgres and compatible engines inheriting their specs”) or adjust the
engine-spec opt-in to match the documented list.
--
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]
