This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch feat/pivot-agg-engine-support in repository https://gitbox.apache.org/repos/asf/superset.git
commit d736289fd800f0b6747c994d223c4042a0500268 Author: Evan Rusackas <[email protected]> AuthorDate: Thu Sep 24 17:39:40 2026 -0700 feat(db_engine_specs): enable MEDIAN/STDDEV_SAMP/VAR_SAMP for Databricks and Snowflake #42895 restored these as first-class metric aggregates but only enabled them for Postgres, MySQL, DuckDB, and Redshift. A customer on Databricks (priority) and Snowflake needs them for pivot table aggregation-method parity work (see the new docs/sip/pivot-table-aggregation-parity.md tracking doc for the full context). Both vendors document native median()/stddev_samp()/var_samp() functions with correct sample (not population) semantics and no dialect-specific syntax needed, same as DuckDB/Redshift's pattern -- confirmed against each vendor's own SQL function reference, not yet against a live instance. Databricks: added to DatabricksBaseEngineSpec (covers the Native, Python Connector, and ODBC connector variants) and separately to DatabricksHiveEngineSpec (the Interactive Cluster connector, which inherits from HiveEngineSpec/PrestoEngineSpec instead and would otherwise silently fall back to the unimplemented default). Snowflake: MEDIAN overridden to use the native, simpler form instead of inheriting PostgresBaseEngineSpec's percentile_cont/WITHIN GROUP workaround (which Snowflake supports but doesn't need); STDDEV_SAMP/ VAR_SAMP reused directly from the Postgres dict since the spelling is identical. Also drops SnowflakeEngineSpec from the negative "must reject unverified" test list now that it opts in. Co-Authored-By: Evan Rusackas <[email protected]> Co-Authored-By: Claude Sonnet 5 <[email protected]> --- UPDATING.md | 19 ++-- docs/sip/pivot-table-aggregation-parity.md | 112 +++++++++++++++++++++ superset/db_engine_specs/databricks.py | 23 +++++ superset/db_engine_specs/snowflake.py | 18 +++- .../unit_tests/db_engine_specs/test_databricks.py | 48 +++++++++ .../test_extended_aggregations_unverified.py | 8 +- tests/unit_tests/db_engine_specs/test_snowflake.py | 25 +++++ 7 files changed, 238 insertions(+), 15 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index ba734724776..8093a9ef185 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -718,15 +718,16 @@ that don't touch SQLAlchemy directly. `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 *spec class*, -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. Engine specs that subclass one of those (e.g. MariaDB, Aurora -MySQL/Postgres, TimescaleDB) inherit the same support, on the same -not-yet-independently-verified basis. Picking one of these aggregates on a -database that has not opted in returns a clear "not supported on this -database" error rather than a failed query. See +Table's controls. Support is opt-in per database engine *spec class*: +Postgres, MySQL (`STDDEV_SAMP`/`VAR_SAMP` only, no `MEDIAN`), DuckDB, and +Redshift (inherits Postgres's support) were verified against a live +instance; Databricks and Snowflake (both `MEDIAN`/`STDDEV_SAMP`/`VAR_SAMP`) +were confirmed against each vendor's own documented SQL function reference, +not yet against a live instance. Engine specs that subclass one of these +(e.g. MariaDB, Aurora MySQL/Postgres, TimescaleDB) inherit the same support, +on the same verification basis as their parent. Picking one of these +aggregates on a database that has not opted in returns a clear "not +supported on this database" error rather than a failed query. See `docs/sip/median-stddev-variance-aggregates.md` for the full design rationale, including why this is safe to add without reintroducing the totals/subtotals correctness bug fixed by #41184 (SIP-216). diff --git a/docs/sip/pivot-table-aggregation-parity.md b/docs/sip/pivot-table-aggregation-parity.md new file mode 100644 index 00000000000..aac9de3a612 --- /dev/null +++ b/docs/sip/pivot-table-aggregation-parity.md @@ -0,0 +1,112 @@ +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +--> + +# Pivot Table aggregation-method parity (post-SIP-216 follow-up) + +## [WORKING DOC — tracking notes for an in-progress effort, not a formal SIP] + +This is a running plan/notes doc, not a polished proposal. It exists so we +don't lose track of what's been established across several research passes. +Update it as decisions land; don't let it drift from what's actually true. + +## Background + +- SIP-216 (#41184) fixed a real correctness bug: pre-#41184, pivot table + totals/subtotals were computed by re-aggregating already-aggregated leaf + cell values **client-side**, which is wrong for non-additive metrics. The + fix moved to one DB query with `GROUPING SETS`, computing cells and + totals from the same real aggregate. +- #41184's own migration stance: "No database (metadata) migration + required" — old `aggregateFunction` values were left orphaned in + `params`/`query_context`, silently ignored. +- #42761 restored the one piece that *was* safely auto-mappable: the 3 + "Sum as Fraction of..." values → the new `showValuesAs` control. This is + a pure display-layer transform on already-correct numbers, never a + metric/aggregate rewrite. `Count as Fraction of...` was deliberately + excluded (count-vs-value semantics differ). +- #42895 restored `MEDIAN`/`STDDEV_SAMP`/`VAR_SAMP` as first-class metric + SQL aggregates (`docs/sip/median-stddev-variance-aggregates.md`), usable + by any chart type via the standard metric popover. Its own SIP doc + explicitly says restoring old `aggregateFunction` values as a metric + rewrite is **not** a safe mechanical migration the way #42761 was, and + proposes a "flagged for review" (admin-surfaced) migration instead of an + automatic one. Engine support at merge time: Postgres, MySQL (no native + MEDIAN), DuckDB, Redshift only. +- A live customer need triggered this: they're on Databricks (priority) + and Snowflake. +- Filed apache/superset#44625 to track a separate, unverified suspicion: + `superset/charts/client_processing.py`'s `pivot_table_v2()` (used for + reports/alerts/exports, not the live view) may still have the pre-SIP-216 + re-aggregation bug in non-percent display mode. Shelved for later, + tracked separately from this doc. + +## Old (6.0) "Aggregation function" full list, and current disposition + +| Old value | Disposition | Why | +|---|---|---| +| Sum/Avg/Count/Min/Max as fraction of Total/Rows/Columns | Handled (#42761) | n/a | +| Count as Fraction of Total/Rows/Columns | Orphaned, left as-is | Count-vs-value semantics differ from `showValuesAs`; #42761 excluded these on purpose | +| First / Last / List Unique Values | Drop, no equivalent | Confirmed by Evan; no code action needed | +| Median / Sample Standard Deviation / Sample Variance / Count Unique Values | **Under discussion** | See "Open question" below — QA ticket assumed these were mechanically auto-mappable like #42761; #42895's own SIP doc says they are not, for a semantic reason independent of engine support | +| Sum / Average / Count / Min / Max (plain, non-fraction) | Believed inert, no action needed | Metric's own aggregate already reproduces old intent when it matches; needs final confirmation | + +## Open question Evan raised (2026-09-24): was 6.0 actually wrong, or did SIP-216 paint us into a corner? + +Investigating now (see task tracker / next update to this doc). Question: +in 6.0, did the query already pre-aggregate to one row per pivot cell +before `aggregateFunction` (e.g. Median) was applied — making it a +genuine re-aggregation-of-aggregates bug reaching leaf cells too, not just +totals — or did the old architecture send finer-grained rows so a real +Median over real matching rows was computed correctly at the leaf-cell +level, with the bug confined to subtotals/totals? This determines whether +"restore Median" has *any* faithful modern equivalent, or whether 6.0's +own leaf-cell numbers can't be trusted as the target to restore. + +**Answer: pending — do not act on the QA ticket's "mechanically mappable" framing until this lands.** + +## Engine support for MEDIAN / STDDEV_SAMP / VAR_SAMP + +| Engine | MEDIAN | STDDEV_SAMP | VAR_SAMP | Status | +|---|---|---|---|---| +| Postgres | `percentile_cont(0.5).within_group(col)` | native | native | Done (#42895) | +| MySQL | not implemented | native | native | Done (#42895) | +| DuckDB | native | native | native | Done (#42895) | +| Redshift | native (`sa.func.median`) | inherited from Postgres | inherited from Postgres | Done (#42895) | +| Databricks | native (`median`) | native (`stddev_samp`) | native (`var_samp`) | **Done, this branch** — `DatabricksBaseEngineSpec` (covers Native/PythonConnector/ODBC) and `DatabricksHiveEngineSpec` (Interactive Cluster), confirmed via Databricks SQL function reference docs, not yet a live instance | +| Snowflake | native (`median`, overridden — Postgres's inherited `percentile_cont` form works but is needlessly complex) | inherited from Postgres | inherited from Postgres | **Done, this branch** — confirmed via Snowflake SQL function reference docs, not yet a live instance | +| BigQuery, Trino/Presto, Hive (non-Databricks), MSSQL, Oracle, SQLite, ClickHouse, CockroachDB, and others | TBD | TBD | TBD | Survey pending (fork investigation in flight) | + +Tests: `tests/unit_tests/db_engine_specs/test_databricks.py` (new +`test_extended_aggregation_func_compiles_expected_sql`, +`test_databricks_hive_spec_shares_extended_aggregations`), +`tests/unit_tests/db_engine_specs/test_snowflake.py` (already had +`test_extended_aggregation_func_median_uses_native_snowflake_syntax`), +`tests/unit_tests/db_engine_specs/test_extended_aggregations_unverified.py` +updated to drop `SnowflakeEngineSpec` from the "must reject" negative-test +list now that it opts in (Databricks was never in that list — it isn't a +Postgres/MySQL-dialect-family spec, so it was never expected to inherit the +dict silently in the first place). + +## Plan (phases) + +1. **This branch (`feat/pivot-agg-engine-support`)**: add `_extended_aggregations` overrides for Databricks and Snowflake (assuming Snowflake support confirms), following the exact Postgres/DuckDB/Redshift pattern in `superset/db_engine_specs/*.py`. Tests for each. +2. Survey remaining engines for real dialect support; wire up whichever are confirmed, same pattern, likely as follow-up PRs per engine or a small batch. +3. Resolve the open question above, then decide the actual migration shape for orphaned Median/StdDev/Variance/CountUnique `aggregateFunction` values — likely the "flagged for review" approach #42895's SIP proposed (exact surfacing mechanism — Tag vs. report vs. in-product banner — still Evan's call, not yet decided). +4. Revisit apache/superset#44625 (reports/exports re-aggregation bug) once shelved time is up. +5. Out of scope for now: Table / Table v2 parity (confirmed not affected the same way — no pivot-style global aggregate control there) and AG Grid Interactive Pivot (separate viz, tracked as sc-118999 internally). diff --git a/superset/db_engine_specs/databricks.py b/superset/db_engine_specs/databricks.py index a099f6acc18..cc41cd6fa5b 100644 --- a/superset/db_engine_specs/databricks.py +++ b/superset/db_engine_specs/databricks.py @@ -22,6 +22,7 @@ from datetime import datetime from re import Pattern from typing import Any, Callable, cast, TYPE_CHECKING, TypedDict, Union +import sqlalchemy as sa from apispec import APISpec from apispec.ext.marshmallow import MarshmallowPlugin from flask import g @@ -32,6 +33,7 @@ from sqlalchemy import text, types from sqlalchemy.engine.default import DefaultDialect from sqlalchemy.engine.reflection import Inspector from sqlalchemy.engine.url import URL +from sqlalchemy.sql.elements import ColumnElement from superset.constants import TimeGrain from superset.databases.utils import make_url_safe @@ -246,6 +248,17 @@ class DatabricksBaseEngineSpec(BaseEngineSpec): identifier_quote_start: str = "`" identifier_quote_end: str = "`" + # Databricks SQL documents native MEDIAN/STDDEV_SAMP/VAR_SAMP aggregate + # functions (docs.databricks.com/aws/en/sql/language-manual/functions/ + # {median,stddev_samp,var_samp}), confirmed against that reference, not a + # live Databricks instance. All three are plain function calls, same + # spelling as Postgres/DuckDB, so no dialect-specific rewriting is needed. + _extended_aggregations: dict[str, Callable[[ColumnElement], ColumnElement]] = { + "MEDIAN": sa.func.median, + "STDDEV_SAMP": sa.func.stddev_samp, + "VAR_SAMP": sa.func.var_samp, + } + @classmethod def convert_dttm( cls, target_type: str, dttm: datetime, db_extra: dict[str, Any] | None = None @@ -986,6 +999,16 @@ class DatabricksHiveEngineSpec(HiveEngineSpec): _time_grain_expressions = time_grain_expressions + # Interactive Clusters run Spark SQL, same as the primary Databricks + # connector above; same native MEDIAN/STDDEV_SAMP/VAR_SAMP functions + # apply here rather than the inherited (unimplemented) HiveEngineSpec/ + # PrestoEngineSpec default. + _extended_aggregations: dict[str, Callable[[ColumnElement], ColumnElement]] = { + "MEDIAN": sa.func.median, + "STDDEV_SAMP": sa.func.stddev_samp, + "VAR_SAMP": sa.func.var_samp, + } + # TODO: remove once we've upgraded to SQLAlchemy>=2.0 and databricks-sql-python>=3.x monkeypatch_dialect() diff --git a/superset/db_engine_specs/snowflake.py b/superset/db_engine_specs/snowflake.py index c1bcd86dca3..a3e71b912a2 100644 --- a/superset/db_engine_specs/snowflake.py +++ b/superset/db_engine_specs/snowflake.py @@ -23,6 +23,7 @@ from re import Pattern from typing import Any, Callable, cast, Optional, TYPE_CHECKING, TypedDict from urllib import parse +import sqlalchemy as sa from apispec import APISpec from apispec.ext.marshmallow import MarshmallowPlugin from cryptography.hazmat.backends import default_backend @@ -142,10 +143,19 @@ class SnowflakeEngineSpec(PostgresBaseEngineSpec): force_column_alias_quotes = True max_column_name_length = 256 - # `PostgresBaseEngineSpec._extended_aggregations` (MEDIAN/STDDEV_SAMP/VAR_SAMP) - # is verified against real Postgres behavior, not Snowflake's; disable it here - # until someone confirms the same expressions against a live Snowflake instance. - _extended_aggregations: dict[str, Callable[[ColumnElement], ColumnElement]] = {} + # Snowflake documents native MEDIAN/STDDEV_SAMP/VAR_SAMP aggregate functions + # (docs.snowflake.com/en/sql-reference/functions/{median,stddev_samp,var_samp}), + # confirmed against that reference, not a live Snowflake instance. STDDEV_SAMP/ + # VAR_SAMP are plain function calls, same spelling as inherited from + # `PostgresBaseEngineSpec`, so those are reused directly. MEDIAN is overridden: + # Snowflake's own `MEDIAN(x)` is a plain function call, unlike Postgres's + # `percentile_cont(0.5) WITHIN GROUP (ORDER BY x)` (Postgres has no native + # MEDIAN), so there is no reason to compile the more complex inherited form. + _extended_aggregations: dict[str, Callable[[ColumnElement], ColumnElement]] = { + "MEDIAN": sa.func.median, + "STDDEV_SAMP": PostgresBaseEngineSpec._extended_aggregations["STDDEV_SAMP"], + "VAR_SAMP": PostgresBaseEngineSpec._extended_aggregations["VAR_SAMP"], + } # Snowflake doesn't support IS true/false syntax, use = true/false instead use_equality_for_boolean_filters = True diff --git a/tests/unit_tests/db_engine_specs/test_databricks.py b/tests/unit_tests/db_engine_specs/test_databricks.py index fe147df0928..905cc279d66 100644 --- a/tests/unit_tests/db_engine_specs/test_databricks.py +++ b/tests/unit_tests/db_engine_specs/test_databricks.py @@ -30,6 +30,7 @@ from sqlalchemy.engine.url import make_url from superset.db_engine_specs.base import OAuth2State from superset.db_engine_specs.databricks import ( + DatabricksBaseEngineSpec, DatabricksNativeEngineSpec, DatabricksPythonConnectorEngineSpec, ) @@ -1198,3 +1199,50 @@ def test_get_engine_spec_unrecognized_driver_prefers_python_connector() -> None: get_engine_spec("databricks", "databricks-sql-python") is DatabricksPythonConnectorEngineSpec ) + + [email protected]( + "spec_cls", + [DatabricksNativeEngineSpec, DatabricksPythonConnectorEngineSpec], +) [email protected]( + ("aggregate", "expected_sql"), + [ + ("MEDIAN", "median(sales)"), + ("STDDEV_SAMP", "stddev_samp(sales)"), + ("VAR_SAMP", "var_samp(sales)"), + ], +) +def test_extended_aggregation_func_compiles_expected_sql( + spec_cls: type[DatabricksBaseEngineSpec], aggregate: str, expected_sql: str +) -> None: + """ + Verified against the Databricks SQL function reference + (docs.databricks.com/aws/en/sql/language-manual/functions/{median, + stddev_samp,var_samp}), not a live instance: all three are plain, + dialect-agnostic function calls (unlike Postgres's MEDIAN, which needs + percentile_cont/WITHIN GROUP), so there is no dialect-specific SQL + construct to compile here. + """ + from sqlalchemy.sql import column + + func = spec_cls.get_extended_aggregation_func(aggregate) + assert func is not None + + compiled = str( + func(column("sales")).compile(compile_kwargs={"literal_binds": True}) + ) + assert compiled == expected_sql + + +def test_databricks_hive_spec_shares_extended_aggregations() -> None: + """ + Interactive Clusters (``DatabricksHiveEngineSpec``) run Spark SQL too, so + the same native aggregates apply -- confirming this spec does NOT fall + back to the inherited, unimplemented ``HiveEngineSpec``/``PrestoEngineSpec`` + default (an empty dict, i.e. "not supported on this database"). + """ + from superset.db_engine_specs.databricks import DatabricksHiveEngineSpec + + for aggregate in ("MEDIAN", "STDDEV_SAMP", "VAR_SAMP"): + assert DatabricksHiveEngineSpec.get_extended_aggregation_func(aggregate) diff --git a/tests/unit_tests/db_engine_specs/test_extended_aggregations_unverified.py b/tests/unit_tests/db_engine_specs/test_extended_aggregations_unverified.py index a5fc530a322..30f305e7854 100644 --- a/tests/unit_tests/db_engine_specs/test_extended_aggregations_unverified.py +++ b/tests/unit_tests/db_engine_specs/test_extended_aggregations_unverified.py @@ -23,6 +23,12 @@ materially different query engine (a proprietary appliance, a distributed SQL engine, or an OLAP engine) must not silently inherit that dict: each verified engine spec opts in explicitly, everything else stays unsupported until someone verifies it against a live instance. + +`SnowflakeEngineSpec` is deliberately absent from this list: it now opts in +explicitly (see `tests/unit_tests/db_engine_specs/test_snowflake.py`'s +`test_extended_aggregation_func_median_uses_native_snowflake_syntax`), its +own documented SQL function reference confirmed for MEDIAN/STDDEV_SAMP/ +VAR_SAMP, though not yet against a live Snowflake instance. """ from typing import Type @@ -38,7 +44,6 @@ from superset.db_engine_specs.hologres import HologresEngineSpec from superset.db_engine_specs.netezza import NetezzaEngineSpec from superset.db_engine_specs.oceanbase import OceanBaseEngineSpec from superset.db_engine_specs.risingwave import RisingWaveDbEngineSpec -from superset.db_engine_specs.snowflake import SnowflakeEngineSpec from superset.db_engine_specs.starrocks import StarRocksEngineSpec from superset.db_engine_specs.vertica import VerticaEngineSpec from superset.db_engine_specs.yugabytedb import YugabyteDBEngineSpec @@ -50,7 +55,6 @@ from superset.db_engine_specs.yugabytedb import YugabyteDBEngineSpec VerticaEngineSpec, NetezzaEngineSpec, HanaEngineSpec, - SnowflakeEngineSpec, CockroachDbEngineSpec, GreenplumEngineSpec, RisingWaveDbEngineSpec, diff --git a/tests/unit_tests/db_engine_specs/test_snowflake.py b/tests/unit_tests/db_engine_specs/test_snowflake.py index b7a33b7110d..f6165abed1b 100644 --- a/tests/unit_tests/db_engine_specs/test_snowflake.py +++ b/tests/unit_tests/db_engine_specs/test_snowflake.py @@ -807,3 +807,28 @@ def test_snowflake_oauth2_exception_catches_refresh_token_error() -> None: "OAuth2TokenRefreshError must be caught by " "SnowflakeEngineSpec.oauth2_exception" ) + + +def test_extended_aggregation_func_median_uses_native_snowflake_syntax() -> None: + """ + Snowflake documents a native `MEDIAN(x)` function, so MEDIAN is + overridden rather than inherited from `PostgresBaseEngineSpec` (which + compiles to `percentile_cont(0.5) WITHIN GROUP (ORDER BY col)`, since + Postgres has no native MEDIAN). STDDEV_SAMP/VAR_SAMP are plain function + calls with the same spelling on both engines, so those are inherited. + """ + from sqlalchemy import column + + from superset.db_engine_specs.snowflake import SnowflakeEngineSpec + + col = column("sales") + + median_func = SnowflakeEngineSpec.get_extended_aggregation_func("MEDIAN") + assert median_func is not None + assert ( + str(median_func(col).compile(compile_kwargs={"literal_binds": True})) + == "median(sales)" + ) + + for aggregate in ("STDDEV_SAMP", "VAR_SAMP"): + assert SnowflakeEngineSpec.get_extended_aggregation_func(aggregate) is not None
