rusackas commented on code in PR #42895:
URL: https://github.com/apache/superset/pull/42895#discussion_r3740370527
##########
superset/db_engine_specs/base.py:
##########
@@ -607,6 +608,33 @@ class BaseEngineSpec: # pylint:
disable=too-many-public-methods
# issuing one query per level. Conservative default of False; engines opt
in.
supports_grouping_sets = False
+ # SQL-generating callables for metric aggregates that have no safe,
universal
+ # cross-dialect spelling -- unlike SUM/COUNT/AVG/MIN/MAX/COUNT_DISTINCT
(see
+ # `SqlaTable.sqla_aggregations`), which SQLAlchemy's generic `sa.func` can
emit
+ # unchanged on every engine. Keyed by `Aggregate` name (see
+ # `superset-frontend/packages/superset-ui-core/src/query/types/Metric.ts`);
+ # each value takes a SQLAlchemy column and returns the aggregate
expression.
+ # Absent by default: an aggregate not present here is unsupported on this
+ # engine, and callers must surface a clear "not supported" error rather
than
+ # emit unverified SQL (a wrong statistic returned silently is worse than an
+ # error). Engines opt in via `get_extended_aggregation_func` below once the
+ # expression has been verified against real engine behavior, not assumed
+ # from syntax alone -- see the MySQL engine spec for a concrete example of
+ # why this distinction matters (its `VARIANCE()` computes the *population*
+ # variance, not the *sample* variance `VAR_SAMP` denotes).
+ _extended_aggregations: dict[str, Callable[[ColumnElement],
ColumnElement]] = {}
+
+ @classmethod
+ def get_extended_aggregation_func(
+ cls, aggregate: str
+ ) -> Callable[[ColumnElement], ColumnElement] | None:
+ """
+ SQL-generating callable for an aggregate not handled by the generic
+ `sa.func` mapping (e.g. MEDIAN, STDDEV_SAMP, VAR_SAMP). Returns None if
+ this engine has no verified, correct expression for it.
+ """
+ return cls._extended_aggregations.get(aggregate)
Review Comment:
Fair catch. `_extended_aggregations` lived on `PostgresBaseEngineSpec`, so
Vertica, Netezza, HANA, Snowflake, CockroachDB, Greenplum, RisingWave,
YugabyteDB, and Hologres all silently inherited it despite never being
verified. Added explicit `{}` overrides on each so they fall back to the "not
supported" error until someone verifies them live.
##########
superset/models/helpers.py:
##########
@@ -3026,15 +3026,30 @@ def adhoc_metric_to_sqla(
if expression_type == utils.AdhocMetricExpressionType.SIMPLE:
aggregate: Any = metric.get("aggregate")
- if (
- not isinstance(aggregate, str)
- or aggregate not in self.sqla_aggregations
- ):
- raise QueryObjectValidationError(_("Adhoc metric aggregate is
invalid"))
metric_column = metric.get("column") or {}
column_name = cast(str, metric_column.get("column_name"))
sqla_column = sa.column(column_name)
- sqla_metric = self.sqla_aggregations[aggregate](sqla_column)
+
+ if isinstance(aggregate, str) and aggregate in
self.sqla_aggregations:
+ sqla_metric = self.sqla_aggregations[aggregate](sqla_column)
+ elif isinstance(aggregate, str) and (
+ extended_func :=
self.db_engine_spec.get_extended_aggregation_func(
+ aggregate
+ )
+ ):
+ sqla_metric = extended_func(sqla_column)
Review Comment:
Same root cause as the other thread on `base.py`, disabled it on
Vertica/Netezza/HANA/Snowflake/CockroachDB/Greenplum/RisingWave/YugabyteDB/Hologres
(and the MySQL-family equivalents, Doris/StarRocks/OceanBase, which had the
same leak).
--
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]
