feat(metrics): add MEDIAN/STDDEV_SAMP/VAR_SAMP as system-wide aggregates - #42895
Conversation
github-actions
Bot
added
preset-io
doc
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
rusackas
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42895 +/- ##
=======================================
Coverage 66.84% 66.84%
=======================================
Files 2876 2876
Lines 164066 164129 +63
Branches 37863 37872 +9
=======================================
+ Hits 109666 109713 +47
- Misses 52226 52240 +14
- Partials 2174 2176 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dosubot
Bot
added
change:backend
8305f5e to
f02ef22
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Code Review Agent Run #ecf60aActionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #2c7e2aActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #a559baActionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
dbf96b2 to
b616214
Compare
Code Review Agent Run #7cb282Actionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #341b33Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #8955f7Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…regates [DRAFT/SIP] Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tes [DRAFT/SIP] Implements the proposal in docs/sip/median-stddev-variance-aggregates.md: MEDIAN, STDDEV_SAMP, and VAR_SAMP are now selectable anywhere a metric aggregate is chosen, not just Pivot Table. Support is opt-in per engine via a new BaseEngineSpec.get_extended_aggregation_func hook (mirroring supports_grouping_sets/_time_grain_expressions), verified against live Postgres/MySQL/DuckDB instances before being enabled; unsupported engines raise a clear error instead of emitting unverified SQL. No pivot-table- specific code needed: these aggregates fall outside ADDITIVE_AGGREGATES automatically, so totals/subtotals already route through the correct DB-rollup path SIP-216 (#41184) introduced. Also fixes a live bug this surfaced: the MCP chart-creation service advertised STDDEV/VAR as valid aggregates that never matched any real Superset aggregate name, so MCP-built charts using them always errored at query time. Now normalized to STDDEV_SAMP/VAR_SAMP. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… decided Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
MEDIAN/STDDEV_SAMP/VAR_SAMP were only verified against live Postgres,
MySQL, DuckDB, and Redshift instances, but `_extended_aggregations`
lived on `PostgresBaseEngineSpec`/`PostgresEngineSpec`/`MySQLEngineSpec`,
so every other subclass (Vertica, Netezza, HANA, Snowflake, CockroachDB,
Greenplum, RisingWave, YugabyteDB, Hologres, Doris, StarRocks, OceanBase)
silently inherited unverified aggregate SQL instead of the intended
"not supported on this database" error. Each now explicitly overrides
`_extended_aggregations` to `{}` until someone verifies it live.
Also fixes two smaller gaps the same review surfaced: MEDIAN was missing
from the MCP chart format validator's numeric-format branch, and
`get_metric_type_from_column`/`METRIC_MAP_TYPE` didn't recognize
STDDEV_SAMP/VAR_SAMP, so an all-null result column for either aggregate
fell back to STRING instead of NUMERIC.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ones Postgres/MySQL-compatible engine specs (MariaDB, Aurora MySQL/Postgres, TimescaleDB) also inherit the new MEDIAN/STDDEV_SAMP/VAR_SAMP support via their parent spec class, same as Redshift; the entry previously only named the four directly-verified engines. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
MariaDB, Aurora MySQL/PostgreSQL (and their Data API variants), and TimescaleDB subclass MySQLEngineSpec/PostgresBaseEngineSpec without resetting _extended_aggregations, unlike the unverified appliance/ distributed-engine specs (CockroachDB, Greenplum, HANA). These are wire- and SQL-compatible forks running the same query engine, so keep them inheriting -- matching the existing Redshift precedent -- and add the same explicit test coverage plus a doc comment so the intent is no longer just implicit. Also add human-readable dropdown labels for STDDEV_SAMP/VAR_SAMP in the metric aggregate picker, instead of showing the raw enum value. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- parametrize the Postgres extended-aggregate test over MEDIAN, STDDEV_SAMP, and VAR_SAMP instead of asserting only MEDIAN - add a live in-process DuckDB execution test for the three aggregates, alongside the existing compile-only test whose docstring overclaimed live verification - make get_metric_type_from_column's aggregate regex case- and whitespace-insensitive so saved expressions like stddev_samp(x) or STDDEV_SAMP (x) are still recognized Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…trip/Redshift - ChartDataAdhocMetricSchema's aggregate enum now includes EXTENDED_METRIC_AGGREGATES, so Swagger/generated clients accept MEDIAN/STDDEV_SAMP/VAR_SAMP requests the compiler already handles. - Custom SQL tab now prefills MEDIAN as the portable PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY col) instead of the raw MEDIAN(col) that PostgreSQL would reject; the display label keeps the concise MEDIAN(col) form. - sqlaAutoGeneratedMetricRegex recognizes MEDIAN/STDDEV_SAMP/VAR_SAMP so they round-trip between Simple and Custom SQL like the original six. - Redshift now emits its native MEDIAN(x) function instead of inheriting Postgres's PERCENTILE_CONT WITHIN GROUP spelling, sidestepping Redshift's restriction on multiple sort-based aggregates with different ORDER BY expressions in one query. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3c640b8 to
1dbf3f1
Compare
sha174n
left a comment
sha174n
left a comment
There was a problem hiding this comment.
LGTM from a query-construction standpoint. The aggregate names from metric config are resolved purely as lookup keys into the per-engine function map (the emitted SQL comes from hardcoded sa.func callables), and the opt-in-per-engine model means an aggregate that isn't verified for an engine surfaces an explicit QueryObjectValidationError rather than emitting an expression the engine might compute differently. MEDIAN handling looks correct per engine. One non-blocking follow-up: the Custom SQL prefill for MEDIAN emits the Postgres PERCENTILE_CONT ... WITHIN GROUP spelling, which won't round-trip cleanly on engines expecting native MEDIAN(col) - fine as a later refinement.
sha174n
left a comment
sha174n
left a comment
There was a problem hiding this comment.
LGTM from a query-construction standpoint. Aggregate names from metric config resolve purely as lookup keys into the per-engine function map (emitted SQL comes from hardcoded sa.func callables), and the opt-in-per-engine model means an unverified aggregate surfaces an explicit QueryObjectValidationError rather than a silently-different expression. MEDIAN handling looks correct per engine. Non-blocking: the Custom SQL prefill for MEDIAN emits the Postgres PERCENTILE_CONT ... WITHIN GROUP spelling, which won't round-trip cleanly on engines expecting native MEDIAN(col).
sha174n
left a comment
sha174n
left a comment
There was a problem hiding this comment.
LGTM - aggregate names resolve as lookup keys into per-engine hardcoded sa.func callables (no injection), and unverified aggregates raise QueryObjectValidationError rather than emitting a silently-different expression. MEDIAN correct per engine.
sha174n
left a comment
sha174n
left a comment
There was a problem hiding this comment.
LGTM - aggregate names resolve as lookup keys into per-engine hardcoded sa.func callables (no injection), and an unverified aggregate raises QueryObjectValidationError rather than emitting a silently-different expression. MEDIAN handling correct per engine.
|
One non-blocking follow-up: the Custom SQL prefill for MEDIAN emits the Postgres PERCENTILE_CONT(0.5) WITHIN GROUP spelling (AdhocMetric.ts), so a user who switches a Simple MEDIAN metric to Custom SQL and saves gets an expression that DuckDB/Redshift (which use native MEDIAN(col)) may reject. Fine as a later refinement since Custom SQL is a raw-expression escape hatch, just flagging the cross-engine round-trip wart. |
|
@sha174n checked this against duckdb directly and PERCENTILE_CONT(0.5) WITHIN GROUP runs fine there, alone or combined with a native |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #adf52b
Actionable Suggestions - 1
-
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.ts - 1
- Wrong SQL for non-Postgres engines · Line 162-168
Additional Suggestions - 1
-
superset/db_engine_specs/snowflake.py - 1
-
Missing test for _extended_aggregations override · Line 91-94The `SnowflakeEngineSpec._extended_aggregations` override is not covered by any test. The test file has 43 references to `SnowflakeEngineSpec` yet 0 references to `_extended_aggregations` or `get_extended_aggregation_func`. A test should assert that `get_extended_aggregation_func('MEDIAN')` returns `None` to guard against accidental re-enablement as the class evolves.
-
Review Details
-
Files reviewed - 45 · Commit Range:
67af985..1dbf3f1- superset-frontend/packages/superset-ui-core/src/query/types/Metric.ts
- superset-frontend/plugins/plugin-chart-pivot-table/test/plugin/utilities.test.ts
- superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.test.ts
- superset-frontend/src/explore/components/controls/MetricControl/AdhocMetric.ts
- superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.tsx
- superset-frontend/src/explore/constants.ts
- superset/charts/schemas.py
- superset/connectors/sqla/models.py
- superset/db_engine_specs/base.py
- superset/db_engine_specs/cockroachdb.py
- superset/db_engine_specs/doris.py
- superset/db_engine_specs/duckdb.py
- superset/db_engine_specs/greenplum.py
- superset/db_engine_specs/hana.py
- superset/db_engine_specs/hologres.py
- superset/db_engine_specs/mysql.py
- superset/db_engine_specs/netezza.py
- superset/db_engine_specs/oceanbase.py
- superset/db_engine_specs/postgres.py
- superset/db_engine_specs/redshift.py
- superset/db_engine_specs/risingwave.py
- superset/db_engine_specs/snowflake.py
- superset/db_engine_specs/starrocks.py
- superset/db_engine_specs/vertica.py
- superset/db_engine_specs/yugabytedb.py
- superset/mcp_service/chart/chart_utils.py
- superset/mcp_service/chart/prompts/create_chart_guided.py
- superset/mcp_service/chart/schemas.py
- superset/mcp_service/chart/validation/dataset_validator.py
- superset/mcp_service/chart/validation/runtime/format_validator.py
- superset/models/helpers.py
- superset/utils/core.py
- tests/unit_tests/charts/test_schemas.py
- tests/unit_tests/db_engine_specs/test_aurora.py
- tests/unit_tests/db_engine_specs/test_base.py
- tests/unit_tests/db_engine_specs/test_duckdb.py
- tests/unit_tests/db_engine_specs/test_extended_aggregations_unverified.py
- tests/unit_tests/db_engine_specs/test_mariadb.py
- tests/unit_tests/db_engine_specs/test_mysql.py
- tests/unit_tests/db_engine_specs/test_postgres.py
- tests/unit_tests/db_engine_specs/test_redshift.py
- tests/unit_tests/db_engine_specs/test_timescaledb.py
- tests/unit_tests/mcp_service/chart/test_chart_utils.py
- tests/unit_tests/models/helpers_test.py
- tests/unit_tests/utils/map_type_tests.py
-
Files skipped - 2
- UPDATING.md - Reason: Filter setting
- docs/sip/median-stddev-variance-aggregates.md - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| if ( | ||
| params.transformCountDistinct && | ||
| aggregate === AGGREGATES.MEDIAN && | ||
| /^\(.*\)$/.test(column) | ||
| ) { | ||
| return `PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY ${column.slice(1, -1)})`; | ||
| } |
There was a problem hiding this comment.
The MEDIAN→PERCENTILE_CONT transformation at lines 162-168 applies unconditionally to ALL database engines, including DuckDB, Snowflake, BigQuery, Redshift, Oracle, T-SQL, Databricks, and Spark — all of which have a native MEDIAN(x) function per SIP documentation and engine specs. This generates incorrect SQL for those engines. The backend's _extended_aggregations map (postgres.py line 211, duckdb.py line 214) already handles engine-specific MEDIAN translation correctly at query execution time; the frontend rewrite bypasses this and overrides it with a Postgres-only form.
Code Review Run #adf52b
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
SUMMARY
This now has a working implementation alongside
docs/sip/median-stddev-variance-aggregates.md, so reviewers (and PMC members this gets shared with) have a concrete design to react to rather than just a description of one. Still opened as a draft: the point is to pressure-test the framing and scope before this goes up for a formal SIP vote, not to pre-empt that discussion.Prompted by a report that Pivot Table lost several aggregation options somewhere along the way. Short version: most of what looked missing (Count Unique Values, Count/Average/Max/Min, "as fraction of" display) already has an equivalent today, see the doc for specifics.
MEDIAN,STDDEV_SAMP(Sample Standard Deviation), andVAR_SAMP(Sample Variance) are genuinely gone, system-wide, not just Pivot Table, since #41184 (SIP-216) deliberately removed the old pivot-only "Aggregation function" control as the mechanism behind our long-standing wrong-totals bug. Not purely hypothetical either: the MCP chart-creation service already advertisedSTDDEV/VARas valid aggregates that never matched any real Superset aggregate name, so MCP-built charts using them always errored at query time — fixed here too, normalized to the real names.What's implemented:
MEDIAN/STDDEV_SAMP/VAR_SAMPadded to the system-wideAggregatetype and the metric-picker dropdown (every chart type, not a Pivot-Table-specific control).BaseEngineSpec.get_extended_aggregation_funcextensibility point (mirrorssupports_grouping_sets/_time_grain_expressions, both from feat(table/pivot-table): correct non-additive totals/subtotals via DB rollup [SIP-216] #41184) — engines opt in only once verified, unsupported engines get a clear "not supported on this database" error instead of unverified SQL.VARIANCE()is population variance, not sample).ADDITIVE_AGGREGATESautomatically, so totals/subtotals already route through the correct DB-rollup path SIP-216 introduced — same mechanism that already makesAVG/COUNT_DISTINCTcorrect today.STDDEV/VARshorthand normalized toSTDDEV_SAMP/VAR_SAMP.Explicitly out of scope, per the doc:
First/Last(no unambiguous SQL meaning as a plain aggregate without an explicit ordering) andList Unique Values(dialect + unbounded-payload questions) — flagged as candidate follow-up SIPs.TESTING INSTRUCTIONS
Backend:
pytest tests/unit_tests/db_engine_specs/test_base.py tests/unit_tests/db_engine_specs/test_postgres.py tests/unit_tests/db_engine_specs/test_mysql.py tests/unit_tests/db_engine_specs/test_duckdb.py tests/unit_tests/db_engine_specs/test_redshift.py tests/unit_tests/models/helpers_test.py tests/unit_tests/mcp_service/Frontend:
npx jest plugins/plugin-chart-pivot-table/test/plugin/utilities.test.ts(confirms the new aggregates are automatically treated as non-additive, with no pivot-table code changes).Manually: on a Postgres-backed dataset, create a chart with a
MEDIAN/STDDEV_SAMP/VAR_SAMPmetric; on a SQLite-backed one, confirm the same aggregate now returns a clear "not supported on this database" error rather than a generic one.ADDITIONAL INFORMATION