codeant-ai-for-open-source[bot] commented on code in PR #38403:
URL: https://github.com/apache/superset/pull/38403#discussion_r2931137889
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -690,6 +690,105 @@ class MixedTimeseriesChartConfig(BaseModel):
filters: List[FilterConfig] | None = Field(None, description="Filters to
apply")
+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. Set show_trendline=True with a temporal_column "
+ "for a number with trendline, or leave show_trendline=False "
+ "for a standalone number."
+ ),
+ )
+ 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."
+ ),
+ min_length=1,
+ max_length=255,
+ pattern=r"^[a-zA-Z0-9_][a-zA-Z0-9_\s\-\.]*$",
+ )
+ 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."
+ ),
+ )
+ 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."
+ )
+ return self
Review Comment:
**Suggestion:** `compare_lag` is accepted even when `show_trendline` is
false, but downstream mapping only applies `compare_lag` for trendline charts,
so user input is silently ignored and produces incorrect chart behavior. Add
validation that `compare_lag` requires trendline mode (and a temporal column)
to prevent accepted-but-dropped config. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ generate_chart accepts compare_lag then ignores it silently.
- ⚠️ generate_explore_link previews omit requested period comparison.
```
</details>
```suggestion
@model_validator(mode="after")
def validate_trendline_fields(self) -> "BigNumberChartConfig":
"""Validate trendline-specific field dependencies."""
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.compare_lag is not None and not self.show_trendline:
raise ValueError(
"Big Number `compare_lag` requires show_trendline=True "
"and a valid 'temporal_column'."
)
return self
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call MCP tool `generate_chart` (entrypoint
`superset/mcp_service/chart/tool/generate_chart.py:122-126`) with
`config.chart_type=\"big_number\"`, `compare_lag=7`, and
`show_trendline=false`.
2. Request is parsed as `GenerateChartRequest`
(`superset/mcp_service/chart/schemas.py:1048-1051`), which instantiates
`BigNumberChartConfig`; current validator `validate_trendline_fields`
(`superset/mcp_service/chart/schemas.py:770-778`) only enforces temporal
column when
trendline is true, so this payload is accepted.
3. Generation path maps config via `map_config_to_form_data`
(`superset/mcp_service/chart/chart_utils.py:308-329`) into
`map_big_number_config`
(`.../chart_utils.py:649`).
4. In `map_big_number_config`, non-trendline config resolves to
`viz_type=\"big_number_total\"` (`.../chart_utils.py:652-655`), and
`compare_lag` is only
written inside `if viz_type == \"big_number\"`
(`.../chart_utils.py:670-683`), so
user-provided `compare_lag` is silently dropped.
5. The same silent drop occurs for preview flow `generate_explore_link`
(`superset/mcp_service/explore/tool/generate_explore_link.py:42-45`, mapping
call at
`:31-33`), so both creation and exploration can misrepresent requested
comparison
behavior.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/schemas.py
**Line:** 769:778
**Comment:**
*Logic Error: `compare_lag` is accepted even when `show_trendline` is
false, but downstream mapping only applies `compare_lag` for trendline charts,
so user input is silently ignored and produces incorrect chart behavior. Add
validation that `compare_lag` requires trendline mode (and a temporal column)
to prevent accepted-but-dropped config.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38403&comment_hash=a030865feaf89f4874abe418e08911374b59531935b4b82713b4782b513f85a9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38403&comment_hash=a030865feaf89f4874abe418e08911374b59531935b4b82713b4782b513f85a9&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]
