aminghadersohi commented on code in PR #38403:
URL: https://github.com/apache/superset/pull/38403#discussion_r2894946513
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -485,6 +485,113 @@ def validate_value_type_matches_operator(self) ->
FilterConfig:
# Actual chart types
+class BigNumberChartConfig(BaseModel):
+ model_config = ConfigDict(extra="forbid")
+
+ chart_type: Literal["big_number"] = Field(
+ ...,
+ description=(
+ "Chart type discriminator - MUST be 'big_number'. "
+ "Creates Big Number charts that display a single prominent "
+ "metric value. Use 'header_only' for a standalone number, "
+ "or include a temporal column with 'show_trendline' for a "
+ "number with trendline."
+ ),
+ )
+ metric: ColumnRef = Field(
+ ...,
+ description=(
+ "The metric to display as a big number. "
+ "Must include an aggregate function (e.g., SUM, COUNT)."
+ ),
+ )
+ temporal_column: str | None = Field(
+ None,
+ description=(
+ "Temporal column for the trendline x-axis. "
+ "Required when show_trendline is True."
+ ),
+ max_length=255,
+ )
+ time_grain: TimeGrain | None = Field(
+ None,
+ description=(
+ "Time granularity for trendline data. "
+ "Common values: PT1H (hour), P1D (day), P1W (week), "
+ "P1M (month), P1Y (year)."
+ ),
+ )
+ show_trendline: bool = Field(
+ False,
+ description=(
+ "Show a trendline below the big number. "
+ "Requires 'temporal_column' to be set."
+ ),
+ )
+ header_only: bool = Field(
+ True,
+ description=(
+ "If True, renders as big_number_total (number only). "
+ "If False and show_trendline is True, renders as "
+ "big_number (number + trendline)."
+ ),
+ )
+ subheader: str | None = Field(
+ None,
+ description="Subtitle text displayed below the big number",
+ max_length=500,
+ )
+ y_axis_format: str | None = Field(
+ None,
+ description=(
+ "Number format string for the metric value "
+ "(e.g., '$,.2f' for currency, ',.0f' for integers, "
+ "'.2%' for percentages)"
+ ),
+ max_length=50,
+ )
+ start_y_axis_at_zero: bool = Field(
+ True,
+ description="Anchor trendline y-axis at zero",
+ )
+ compare_lag: int | None = Field(
+ None,
+ description=(
+ "Number of time periods to compare against. "
+ "Displays a percentage change vs the prior period."
+ ),
+ ge=1,
+ )
+ filters: list[FilterConfig] | None = Field(
+ None,
+ description="Filters to apply",
+ )
+
+ @model_validator(mode="after")
+ def validate_trendline_fields(self) -> "BigNumberChartConfig":
+ """Validate trendline requires temporal column."""
+ if self.show_trendline and not self.temporal_column:
+ raise ValueError(
+ "Big Number chart with show_trendline=True requires "
+ "'temporal_column'. Specify a date/time column for "
+ "the trendline x-axis."
+ )
+ if self.show_trendline:
+ self.header_only = False
+ return self
+
+ @model_validator(mode="after")
+ def validate_metric_aggregate(self) -> "BigNumberChartConfig":
+ """Ensure metric has an aggregate function."""
+ if not self.metric.aggregate:
+ raise ValueError(
+ "Big Number metric must have an aggregate function "
+ "(e.g., SUM, COUNT, AVG). Add 'aggregate' to the "
+ "metric specification."
+ )
+ return self
+
+
Review Comment:
The `header_only` field is valid even when `show_trendline` is False — it
controls whether to show just the header number without a subtitle, which is
independent of the trendline setting. The field description could be clearer,
but the behavior is correct.
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -585,40 +638,55 @@ def map_filter_operator(op: str) -> str:
return operator_map.get(op, op)
-def generate_chart_name(config: TableChartConfig | XYChartConfig) -> str:
+def generate_chart_name(
+ config: TableChartConfig | XYChartConfig | BigNumberChartConfig,
+) -> str:
"""Generate a chart name based on the configuration."""
if isinstance(config, TableChartConfig):
- return f"Table Chart - {', '.join(col.name for col in config.columns)}"
+ cols = ", ".join(col.name for col in config.columns)
+ return f"Table Chart - {cols}"
elif isinstance(config, XYChartConfig):
chart_type = config.kind.capitalize()
x_col = config.x.name
y_cols = ", ".join(col.name for col in config.y)
return f"{chart_type} Chart - {x_col} vs {y_cols}"
+ elif isinstance(config, BigNumberChartConfig):
+ metric_label = (
+ config.metric.label or
f"{config.metric.aggregate}({config.metric.name})"
+ )
+ if config.show_trendline:
+ return f"Big Number - {metric_label} (with trendline)"
+ return f"Big Number - {metric_label}"
else:
return "Chart"
+def _resolve_viz_type(config: Any) -> str:
+ """Resolve the Superset viz_type from a chart config object."""
+ chart_type = getattr(config, "chart_type", "unknown")
+ if chart_type == "xy":
+ kind = getattr(config, "kind", "line")
+ viz_type_map = {
+ "line": "echarts_timeseries_line",
+ "bar": "echarts_timeseries_bar",
+ "area": "echarts_area",
+ "scatter": "echarts_timeseries_scatter",
+ }
+ return viz_type_map.get(kind, "echarts_timeseries_line")
+ elif chart_type == "table":
+ return getattr(config, "viz_type", "table")
+ elif chart_type == "big_number":
+ show_trendline = getattr(config, "show_trendline", False)
+ return "big_number" if show_trendline else "big_number_total"
Review Comment:
Good observation. The `_resolve_viz_type` function intentionally only checks
`show_trendline` because that's the primary differentiator between `big_number`
(with trendline) and `big_number_total` (without). The `header_only` flag is a
display option that works with either viz type.
--
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]
