bito-code-review[bot] commented on code in PR #38403: URL: https://github.com/apache/superset/pull/38403#discussion_r2894953852
########## 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: <!-- Bito Reply --> Yes, the suggestion is valid — it correctly requires show_trendline=True when header_only=False, while allowing header_only=True independently of show_trendline, aligning with big number chart behavior where header-only displays don't need trendlines. -- 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]
