codeant-ai-for-open-source[bot] commented on code in PR #42895:
URL: https://github.com/apache/superset/pull/42895#discussion_r3740065827
##########
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:
**Suggestion:** The lookup uses normal class-attribute inheritance, so every
subclass of an engine spec that defines `_extended_aggregations` automatically
exposes those aggregates, even when that subclass has not verified support for
them. This defeats the documented opt-in behavior and can cause unsupported
databases to generate aggregate SQL instead of returning the intended “not
supported” error. Restrict the lookup to mappings explicitly declared by the
concrete engine spec, or require each supported engine to define its own
mapping. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unverified aggregates reach Netezza and HANA queries.
- ❌ Unsupported databases may fail at database execution time.
- ⚠️ Users lose the intended clear support error.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cdeb6c07a2fd4d80b9924ea3fd863dfe&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=cdeb6c07a2fd4d80b9924ea3fd863dfe&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/base.py
**Line:** 636:636
**Comment:**
*Api Mismatch: The lookup uses normal class-attribute inheritance, so
every subclass of an engine spec that defines `_extended_aggregations`
automatically exposes those aggregates, even when that subclass has not
verified support for them. This defeats the documented opt-in behavior and can
cause unsupported databases to generate aggregate SQL instead of returning the
intended “not supported” error. Restrict the lookup to mappings explicitly
declared by the concrete engine spec, or require each supported engine to
define its own mapping.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42895&comment_hash=1c549db85cecd8f92402f01bb19b8fc742a749003d2fff8fa464f009d34f3674&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42895&comment_hash=1c549db85cecd8f92402f01bb19b8fc742a749003d2fff8fa464f009d34f3674&reaction=dislike'>👎</a>
##########
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:
**Suggestion:** The new dispatch now enables every engine spec subclass that
inherits `PostgresBaseEngineSpec` to use the PostgreSQL extended aggregate
expressions. This includes engines such as Vertica, Netezza, and Snowflake,
whose dialect-specific support and semantics are not guaranteed to match
PostgreSQL; queries using these aggregates can therefore compile to unsupported
or incorrect SQL instead of being rejected. Restrict the backend opt-in to
verified engines or add explicit overrides that disable unsupported aggregates.
[api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Metrics can fail on inherited non-Postgres backends.
- ⚠️ Unverified aggregates may produce incorrect statistics.
- ⚠️ Unsupported-database errors are bypassed for several engines.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c9364ade54144b55b0f91281574a5b12&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c9364ade54144b55b0f91281574a5b12&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 3036:3040
**Comment:**
*Api Mismatch: The new dispatch now enables every engine spec subclass
that inherits `PostgresBaseEngineSpec` to use the PostgreSQL extended aggregate
expressions. This includes engines such as Vertica, Netezza, and Snowflake,
whose dialect-specific support and semantics are not guaranteed to match
PostgreSQL; queries using these aggregates can therefore compile to unsupported
or incorrect SQL instead of being rejected. Restrict the backend opt-in to
verified engines or add explicit overrides that disable unsupported aggregates.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42895&comment_hash=fa3de7f0153d732313368dacba65278d78f93ded10d1369fef869aabdefb4d7d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42895&comment_hash=fa3de7f0153d732313368dacba65278d78f93ded10d1369fef869aabdefb4d7d&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/validation/runtime/format_validator.py:
##########
@@ -217,7 +217,7 @@ def suggest_format(column: ColumnRef) -> str:
"""Suggest appropriate format based on column and aggregation."""
if column.aggregate in ["COUNT", "COUNT_DISTINCT"]:
return ",d" # Integer with thousands separator
- elif column.aggregate in ["AVG", "STDDEV", "VAR"]:
+ elif column.aggregate in ["AVG", "STDDEV_SAMP", "VAR_SAMP", "STDDEV",
"VAR"]:
Review Comment:
**Suggestion:** `MEDIAN` is a numeric statistical aggregate like
`STDDEV_SAMP` and `VAR_SAMP`, but it is omitted from this numeric-format
branch. `suggest_format` consequently returns an empty format for median
metrics while returning `,.2f` for the other newly supported statistical
aggregates, producing an inconsistent and less useful chart configuration.
Include `MEDIAN` in the branch. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ MCP median format suggestions remain empty.
- ⚠️ Median presentation differs from equivalent statistical metrics.
- ⚠️ Current helper has no active repository callers.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1e11e2b3cc714f7d8e12db6f2aae088c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1e11e2b3cc714f7d8e12db6f2aae088c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/validation/runtime/format_validator.py
**Line:** 220:220
**Comment:**
*Incorrect Condition Logic: `MEDIAN` is a numeric statistical aggregate
like `STDDEV_SAMP` and `VAR_SAMP`, but it is omitted from this numeric-format
branch. `suggest_format` consequently returns an empty format for median
metrics while returning `,.2f` for the other newly supported statistical
aggregates, producing an inconsistent and less useful chart configuration.
Include `MEDIAN` in the branch.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42895&comment_hash=095d17ce1840050c99a91fdd3d9929aea2430c02952274886abcb512aa633c0b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42895&comment_hash=095d17ce1840050c99a91fdd3d9929aea2430c02952274886abcb512aa633c0b&reaction=dislike'>👎</a>
##########
superset/utils/core.py:
##########
@@ -183,6 +183,15 @@ class AdhocMetricExpressionType(StrEnum):
SQL = "SQL"
+# Aggregates with no safe, universal cross-dialect spelling -- unlike
+# SUM/COUNT/AVG/MIN/MAX/COUNT_DISTINCT, whose SQL is generated the same way on
+# every engine. Support for these is opt-in per `BaseEngineSpec` (see
+# `get_extended_aggregation_func`); used to distinguish a genuinely invalid
+# aggregate name from one that is valid but unsupported on the current
database,
+# for a clearer user-facing error.
+EXTENDED_METRIC_AGGREGATES = frozenset({"MEDIAN", "STDDEV_SAMP", "VAR_SAMP"})
Review Comment:
**Suggestion:** The newly accepted aggregates are not recognized by
`get_metric_type_from_column`: its expression parser and `METRIC_MAP_TYPE`
contain no `STDDEV_SAMP` or `VAR_SAMP` entries. For an all-null result column
whose type cannot be obtained from datasource metadata, these aggregates
therefore infer as an empty type and fall back to `GenericDataType.STRING`
instead of numeric, which can cause numeric metrics to be treated as
categorical. Add the new aggregate names to the metric-expression parser and
numeric type map. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ All-null STDDEV_SAMP results become string-typed.
- ❌ All-null VAR_SAMP results become string-typed.
- ⚠️ Chart query payloads expose incorrect `coltypes`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a7899ae569f2449fb7400eabe527de2f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a7899ae569f2449fb7400eabe527de2f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/core.py
**Line:** 192:192
**Comment:**
*Type Error: The newly accepted aggregates are not recognized by
`get_metric_type_from_column`: its expression parser and `METRIC_MAP_TYPE`
contain no `STDDEV_SAMP` or `VAR_SAMP` entries. For an all-null result column
whose type cannot be obtained from datasource metadata, these aggregates
therefore infer as an empty type and fall back to `GenericDataType.STRING`
instead of numeric, which can cause numeric metrics to be treated as
categorical. Add the new aggregate names to the metric-expression parser and
numeric type map.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42895&comment_hash=dfc1461acfcb2b600eef59fbcf026c86cb0d085baeca576988600cf825158616&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42895&comment_hash=dfc1461acfcb2b600eef59fbcf026c86cb0d085baeca576988600cf825158616&reaction=dislike'>👎</a>
--
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]
