bito-code-review[bot] commented on code in PR #43567:
URL: https://github.com/apache/superset/pull/43567#discussion_r4130417645
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1673,6 +1673,60 @@ def record_implicit_metric_aggregate(self) ->
"BubbleChartConfig":
return self
+class FunnelChartConfig(BaseChartConfig):
+ """Config for funnel charts (viz_type ``funnel``).
+
+ Matches the frontend Funnel buildQuery contract: a single ``groupby``
+ dimension whose values become the funnel stages and one ``metric`` sizing
+ each stage. When ``sort_by_metric`` is set the query orders stages by the
+ metric descending (largest stage first).
+ """
+
+ model_config = ConfigDict(extra="ignore", populate_by_name=True)
+
+ chart_type: Literal["funnel"] = "funnel"
+ dimension: ColumnRef = Field(
+ ...,
+ description="Category column whose values become the funnel stages",
+ validation_alias=AliasChoices("dimension", "groupby"),
+ )
+ metric: ColumnRef = Field(
+ ...,
+ description="Value metric sizing each stage (use aggregate e.g. SUM, "
+ "COUNT for ad-hoc, or set saved_metric=True for a saved dataset
metric)",
+ )
+ sort_by_metric: bool = Field(
+ True,
+ description="Order stages by the metric descending (frontend default)",
+ )
+ row_limit: int = Field(10, description="Max funnel stages", ge=1, le=10000)
+ filters: List[FilterConfig] | None = Field(
+ None,
+ description="Structured filters (column/op/value). "
+ "Do NOT use adhoc_filters or raw SQL expressions.",
+ )
+ color_scheme: str | None = Field(
+ None,
+ description=(
+ "Superset color scheme ID (e.g. 'supersetColors', 'lyftColors', "
+ "'googleCategory10c', 'd3Category10'). Defaults to
'supersetColors'."
+ ),
+ max_length=100,
+ )
+
+ @model_validator(mode="after")
+ def reject_sql_expression_on_dimensions(self) -> "FunnelChartConfig":
+ """aggregate, saved_metric and sql_expression are metric-only markers;
+ reject them on the dimension."""
+ _reject_sql_expression_on_dimension(self.dimension, "dimension")
+ if self.dimension and self.dimension.is_metric:
+ raise ValueError(
+ "dimension must be a plain column, not a metric; drop "
+ "'aggregate'/'saved_metric' (metrics belong in the 'metric'
field)"
+ )
+ return self
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Metric aggregate not validated</b></div>
<div id="fix">
`metric` accepts a bare column (`{"name": ...}`, no aggregate), but
`create_metric_object` silently defaults it to SUM and
`DatasetValidator._validate_aggregations` skips refs without `aggregate` — so
SUM(text_column) reaches the database as a raw DB error. `GaugeChartConfig`
rejects non-metric refs; `BubbleChartConfig.record_implicit_metric_aggregate`
records the implicit SUM for exactly this reason. Add the gauge-style guard
here.
</div>
</div>
<small><i>Code Review Run #ffe711</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
