rusackas commented on code in PR #42895:
URL: https://github.com/apache/superset/pull/42895#discussion_r3762408961
##########
superset/db_engine_specs/duckdb.py:
##########
@@ -198,6 +200,16 @@ class DuckDBEngineSpec(DuckDBParametersMixin,
BaseEngineSpec):
sqlalchemy_uri_placeholder = "duckdb:////path/to/duck.db"
supports_multivalues_insert = True
+ # Verified against a live duckdb instance (in-process, no server needed),
+ # including under GROUPING SETS: the grand total correctly reflects every
+ # row, not an aggregate-of-aggregates. Values match postgres/mysql exactly
Review Comment:
Fair catch, MySQL has no native `MEDIAN` so that line overclaimed. Reworded
to only claim the mysql match for `STDDEV_SAMP`/`VAR_SAMP`, `MEDIAN` is
verified against postgres.
##########
superset/mcp_service/chart/prompts/create_chart_guided.py:
##########
@@ -137,7 +137,9 @@ async def create_chart_guided_prompt(
- If the chart type doesn't suit the data, try a different kind
## Available Aggregations
-SUM, COUNT, AVG, MIN, MAX, COUNT_DISTINCT, STDDEV, VAR, MEDIAN
+SUM, COUNT, AVG, MIN, MAX, COUNT_DISTINCT, STDDEV_SAMP, VAR_SAMP, MEDIAN
+(support for STDDEV_SAMP/VAR_SAMP/MEDIAN depends on the connected database;
+an unsupported choice returns a clear error naming the aggregate and database)
Review Comment:
You're right, the actual error only names the aggregate, not the database.
Dropped that claim from the prompt.
##########
superset-frontend/src/explore/constants.ts:
##########
@@ -23,8 +23,11 @@ export const AGGREGATES = {
COUNT: 'COUNT',
COUNT_DISTINCT: 'COUNT_DISTINCT',
MAX: 'MAX',
+ MEDIAN: 'MEDIAN',
MIN: 'MIN',
+ STDDEV_SAMP: 'STDDEV_SAMP',
SUM: 'SUM',
+ VAR_SAMP: 'VAR_SAMP',
};
export const AGGREGATES_OPTIONS = Object.values(AGGREGATES);
Review Comment:
This mirrors how every other aggregate in this picker already works,
AVG/COUNT_DISTINCT/etc aren't filtered per engine here either, the backend
rejects at query time. Filtering the picker by connected engine is a bigger
change than this PR's scope.
##########
superset/db_engine_specs/postgres.py:
##########
@@ -192,6 +194,18 @@ 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.
+ _extended_aggregations: dict[str, Callable[[ColumnElement],
ColumnElement]] = {
+ "MEDIAN": lambda col: sa.func.percentile_cont(0.5).within_group(col),
+ "STDDEV_SAMP": sa.func.stddev_samp,
+ "VAR_SAMP": sa.func.var_samp,
+ }
Review Comment:
Same as the MySQL/MariaDB case above, TimescaleDB and Aurora Postgres are
Postgres forks by design, same category as Redshift, which also inherits
without overriding. Not adding a {} override for a hypothetical incompatibility
nobody's hit.
##########
superset/utils/core.py:
##########
@@ -1797,7 +1808,9 @@ def get_metric_type_from_column(column: Any, datasource:
Explorable) -> str:
expression: str = metric.expression
match = re.match(
- r"(SUM|AVG|COUNT|COUNT_DISTINCT|MIN|MAX|FIRST|LAST)\((.*)\)",
expression
+ r"(SUM|AVG|COUNT|COUNT_DISTINCT|MIN|MAX|FIRST|LAST"
+ r"|MEDIAN|STDDEV_SAMP|VAR_SAMP)\((.*)\)",
Review Comment:
This regex was already uppercase-only with no whitespace tolerance for
SUM/AVG/etc before this PR, I just extended the same pattern to the three new
aggregates. Not special-casing case-insensitivity for only these three without
touching the existing ones.
--
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]
