codeant-ai-for-open-source[bot] commented on code in PR #38403:
URL: https://github.com/apache/superset/pull/38403#discussion_r3012340417
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -851,6 +852,111 @@ def validate_query_fields(self) ->
"HandlebarsChartConfig":
return self
+class BigNumberChartConfig(UnknownFieldCheckMixin):
+ model_config = ConfigDict(extra="ignore")
+
+ 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) -> Self:
+ """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.compare_lag and not self.show_trendline:
+ raise ValueError(
+ "compare_lag requires show_trendline=True. "
+ "Period comparison is only available for "
+ "trendline charts."
+ )
+ return self
+
+ @model_validator(mode="after")
+ def validate_metric_aggregate(self) -> Self:
+ """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."
Review Comment:
**Suggestion:** The Big Number chart schema currently rejects metrics that
reference saved dataset metrics (`saved_metric=True`) because the validator
strictly checks for a non-null `aggregate`, even though `ColumnRef` and
`map_big_number_config` fully support saved metrics; this causes valid
configurations using saved metrics to fail validation instead of being accepted
like other chart types. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP big_number charts cannot use saved dataset metrics.
- ⚠️ LLM clients must duplicate logic via ad-hoc aggregates.
- ⚠️ Behavior inconsistent with pie and pivot chart metrics.
- ⚠️ Advanced users hit confusing MISSING_BIG_NUMBER_AGGREGATE errors.
```
</details>
```suggestion
"""Ensure metric is a valid metric reference (aggregate or saved
metric)."""
if not self.metric.is_metric:
raise ValueError(
"Big Number metric must be either a saved dataset metric "
"or include an aggregate function (e.g., SUM, COUNT, AVG). "
"Set 'saved_metric': true to use a saved metric, or add "
"'aggregate' to the metric specification."
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. From an MCP client, call the `generate_chart` tool implemented in
`superset/mcp_service/chart/tool/generate_chart.py` (see lines 200–279) with
a request
payload containing a Big Number chart config.
2. Use a request of the form:
`{"dataset_id": 1, "config": {"chart_type": "big_number", "metric":
{"name":
"my_saved_metric", "saved_metric": True}}}`.
This follows the `ColumnRef` schema in
`superset/mcp_service/chart/schemas.py:470-505`,
where `saved_metric=True` means `name` refers to a saved dataset metric
and aggregate
is ignored.
3. The MCP tool converts this to a dict via `request.model_dump()` and
passes it into
`ValidationPipeline.validate_request_with_warnings()` in
`superset/mcp_service/chart/validation/pipeline.py:150-188`, which in turn
calls
`SchemaValidator.validate_request()` in
`superset/mcp_service/chart/validation/schema_validator.py:40-60`.
4. Inside `_pre_validate_big_number_config()` at
`schema_validator.py:110-135`, the code
inspects `config["metric"]` and fails because `metric.get("aggregate")` is
falsy,
returning `is_valid=False` with error_code `"MISSING_BIG_NUMBER_AGGREGATE"`,
so the
request is rejected before Pydantic BigNumberChartConfig is even
constructed, preventing
any Big Number chart that references a saved metric from being created via
MCP.
5. Independently, if another caller constructs `BigNumberChartConfig`
directly in Python
(as tests do for other cases in
`tests/unit_tests/mcp_service/chart/test_big_number_chart.py:41-152`), using
`BigNumberChartConfig(chart_type="big_number",
metric=ColumnRef(name="my_saved_metric",
saved_metric=True))`, the `ColumnRef` post-validator
`clear_aggregate_for_saved_metric()`
(`schemas.py:33-38`) clears `aggregate` to `None`, and
`BigNumberChartConfig.validate_metric_aggregate()` (`schemas.py:94-103`)
raises a
`ValueError("Big Number metric must have an aggregate function ...")`,
meaning the schema
itself also rejects saved metrics even though `create_metric_object()`
(`chart_utils.py:446-485`) and `map_big_number_config()`
(`chart_utils.py:663-700`) fully
support saved metrics.
```
</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:** 950:955
**Comment:**
*Logic Error: The Big Number chart schema currently rejects metrics
that reference saved dataset metrics (`saved_metric=True`) because the
validator strictly checks for a non-null `aggregate`, even though `ColumnRef`
and `map_big_number_config` fully support saved metrics; this causes valid
configurations using saved metrics to fail validation instead of being accepted
like other chart types.
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=baa9fa88ee79db099ffc48a55e377680db61a433f2426f60a4dd28f1850c6f57&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38403&comment_hash=baa9fa88ee79db099ffc48a55e377680db61a433f2426f60a4dd28f1850c6f57&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]
