bito-code-review[bot] commented on code in PR #38403:
URL: https://github.com/apache/superset/pull/38403#discussion_r2888915260
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Missing validation logic</b></div>
<div id="fix">
The header_only field allows False values even when show_trendline is False,
but the field description implies this combination is invalid. This could
result in charts that don't render as expected. Add validation to ensure
header_only=False only when show_trendline=True.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
@model_validator(mode="after")
def validate_header_only(self) -> "BigNumberChartConfig":
if not self.header_only and not self.show_trendline:
raise ValueError(
"Big Number chart with header_only=False requires "
"show_trendline=True to display the trendline."
)
return self
````
</div>
</details>
</div>
<small><i>Code Review Run #cb929c</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
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Logic Inconsistency in Viz Type Resolution</b></div>
<div id="fix">
The _resolve_viz_type function's logic for big_number charts should match
map_big_number_config. Currently, it only checks show_trendline, but
map_big_number_config requires both show_trendline and temporal_column to
select 'big_number' over 'big_number_total'. This inconsistency could lead to
incorrect viz_type resolution when temporal_column is missing despite
show_trendline being True.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
elif chart_type == "big_number":
show_trendline = getattr(config, "show_trendline", False)
temporal_column = getattr(config, "temporal_column", None)
return "big_number" if show_trendline and temporal_column else
"big_number_total"
````
</div>
</details>
</div>
<small><i>Code Review Run #cb929c</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]
