codeant-ai-for-open-source[bot] commented on code in PR #38403:
URL: https://github.com/apache/superset/pull/38403#discussion_r2930721931
##########
superset/mcp_service/app.py:
##########
@@ -116,6 +116,8 @@ def get_default_instructions(branding: str = "Apache
Superset") -> str:
- chart_type="xy", kind="bar": Bar chart for category comparison
- chart_type="xy", kind="area": Area chart for volume visualization
- chart_type="xy", kind="scatter": Scatter plot for correlation analysis
+- chart_type="big_number": Big Number display (single metric, header only)
+- chart_type="big_number", show_trendline=True: Big Number with trendline
Review Comment:
**Suggestion:** The new instruction says `show_trendline=True` is sufficient
for a Big Number trendline, but the chart schema requires a temporal column
when trendline is enabled. This mismatch will cause generated chart requests to
fail validation (`MISSING_TEMPORAL_COLUMN`) when assistants follow the
instruction literally. Update the instruction to include the required
`temporal_column` (and optionally `time_grain`) so tool calls are valid. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Big-number trendline requests fail from default MCP instructions.
- ⚠️ `generate_chart` validation rejects missing temporal trendline configs.
- ⚠️ `generate_explore_link` calls can fail schema parsing early.
```
</details>
```suggestion
- chart_type="big_number", show_trendline=True, temporal_column="<date_col>"
(optional time_grain): Big Number with trendline
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start MCP server with default setup; `mcp = create_mcp_app()` in
`superset/mcp_service/app.py:355` uses `DEFAULT_INSTRUCTIONS` from
`app.py:217`, generated
by `get_default_instructions()` at `app.py:34`.
2. Read the advertised chart guidance in `superset/mcp_service/app.py:120`:
`chart_type="big_number", show_trendline=True` (no `temporal_column`), then
call
`generate_chart` or `generate_explore_link` accordingly.
3. The request is parsed by `@parse_request(GenerateChartRequest)` at
`superset/mcp_service/chart/tool/generate_chart.py:122` (or
`@parse_request(GenerateExploreLinkRequest)` at
`superset/mcp_service/explore/tool/generate_explore_link.py:43`), where
`config` is typed
as `ChartConfig` (`schemas.py:1022`, `schemas.py:1073`).
4. Validation enforces temporal requirement: `BigNumberChartConfig` checks
`if
self.show_trendline and not self.temporal_column` at
`superset/mcp_service/chart/schemas.py:744`; generate-chart pre-validation
also checks
this at `schema_validator.py:327` and returns
`error_code="MISSING_TEMPORAL_COLUMN"` at
`schema_validator.py:338` (covered by test
`tests/unit_tests/mcp_service/chart/test_big_number_chart.py:420-432`).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/app.py
**Line:** 120:120
**Comment:**
*Logic Error: The new instruction says `show_trendline=True` is
sufficient for a Big Number trendline, but the chart schema requires a temporal
column when trendline is enabled. This mismatch will cause generated chart
requests to fail validation (`MISSING_TEMPORAL_COLUMN`) when assistants follow
the instruction literally. Update the instruction to include the required
`temporal_column` (and optionally `time_grain`) so tool calls are valid.
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=bb4d70d987b92c56a2e47a92a24e355eb260ec77183c4296b025d4736da52260&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38403&comment_hash=bb4d70d987b92c56a2e47a92a24e355eb260ec77183c4296b025d4736da52260&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -615,6 +619,59 @@ def map_pie_config(config: PieChartConfig) -> Dict[str,
Any]:
"date_format": "smart_date",
}
+ if time_range := getattr(config, "time_range", None):
+ form_data["time_range"] = time_range
+
+ if config.filters:
+ form_data["adhoc_filters"] = [
+ {
+ "clause": "WHERE",
+ "expressionType": "SIMPLE",
+ "subject": filter_config.column,
+ "operator": map_filter_operator(filter_config.op),
+ "comparator": filter_config.value,
+ }
+ for filter_config in config.filters
+ if filter_config is not None
+ ]
+
+ return form_data
+
+
+def map_big_number_config(config: BigNumberChartConfig) -> Dict[str, Any]:
+ """Map big number chart config to Superset form_data."""
+ # Determine viz_type: big_number (with trendline) or big_number_total
+ if config.show_trendline and config.temporal_column:
+ viz_type = "big_number"
+ else:
+ viz_type = "big_number_total"
+
+ metric = create_metric_object(config.metric)
+ form_data: Dict[str, Any] = {
+ "viz_type": viz_type,
+ "metric": metric,
+ }
Review Comment:
**Suggestion:** The big-number trendline request flag is never written to
form_data. The ECharts big-number plugin uses `show_trend_line` to decide
whether to render the trendline, so omitting it can silently produce a
number-only chart even when trendline was requested. Map `show_trendline` to
`show_trend_line` in the generated form_data. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ⚠️ Big-number trendline requests may render without trendline.
- ❌ MCP `generate_chart` output can misrepresent requested visualization.
- ⚠️ `generate_explore_link` previews can contradict user intent.
- ⚠️ `update_chart` may silently remove expected trendline display.
```
</details>
```suggestion
form_data: Dict[str, Any] = {
"viz_type": viz_type,
"metric": metric,
"show_trend_line": config.show_trendline,
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger MCP chart creation through a real entrypoint: `generate_chart`
maps request
config via `map_config_to_form_data(...)` at
`superset/mcp_service/chart/tool/generate_chart.py:252`, and the same mapper
is also used
by `generate_explore_link.py:110`, `update_chart.py:132`, and
`update_chart_preview.py:72`.
2. Send a Big Number trendline config (`chart_type="big_number",
show_trendline=True,
temporal_column="ds"`); schema permits this path
(`superset/mcp_service/chart/schemas.py:703-747`) and
`map_big_number_config` selects
`viz_type="big_number"`
(`superset/mcp_service/chart/chart_utils.py:644-647`).
3. Inspect produced `form_data` in `map_big_number_config`
(`chart_utils.py:650-674`): it
sets `viz_type`, `metric`, `granularity_sqla`, etc., but does not set
`show_trend_line`.
4. Frontend big-number rendering gates trendline on that flag:
`transformProps` only
builds trendline when `showTrendLine` is truthy
(`superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:218`),
while `BigNumberViz` defaults `showTrendLine = false`
(`.../BigNumber/BigNumberViz.tsx:57`), so omitted flag can render a
number-only chart
despite trendline request.
```
</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/chart_utils.py
**Line:** 650:653
**Comment:**
*Logic Error: The big-number trendline request flag is never written to
form_data. The ECharts big-number plugin uses `show_trend_line` to decide
whether to render the trendline, so omitting it can silently produce a
number-only chart even when trendline was requested. Map `show_trendline` to
`show_trend_line` in the generated form_data.
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=a7246bdec9c092d8def3d95159a2d137e0356cf39f36db7f48ac0dab3eb0a53f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38403&comment_hash=a7246bdec9c092d8def3d95159a2d137e0356cf39f36db7f48ac0dab3eb0a53f&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -664,6 +664,103 @@ 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."
+ ),
+ max_length=255,
+ )
Review Comment:
**Suggestion:** `temporal_column` is accepted as an arbitrary string without
identifier validation, then passed directly into `granularity_sqla`. This
allows invalid or unsafe values to reach query-building paths and can cause
runtime query failures. Constrain it to a safe column-name pattern (like other
column references) to reject malformed input early. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Big-number trendline creation fails on malformed temporal columns.
- ⚠️ Errors surface late during compile check.
- ⚠️ generate_explore_link may return unusable broken chart URLs.
```
</details>
```suggestion
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\-\.]*$",
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call MCP `generate_chart` with `config.chart_type="big_number"`,
`show_trendline=true`,
and invalid `temporal_column` (entrypoint
`superset/mcp_service/chart/tool/generate_chart.py:121-125`).
2. Request passes schema because `BigNumberChartConfig.temporal_column` only
has
`max_length` and no identifier/pattern validation
(`superset/mcp_service/chart/schemas.py:687-694`).
3. Mapping writes this raw value into `form_data["granularity_sqla"]` in
`map_big_number_config`
(`superset/mcp_service/chart/chart_utils.py:641-667`).
4. Compile check executes query (`_compile_chart`) and returns
`compile_error` on bad
temporal column
(`superset/mcp_service/chart/tool/generate_chart.py:544-567`), so chart
generation fails late instead of early validation.
```
</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:** 687:694
**Comment:**
*Possible Bug: `temporal_column` is accepted as an arbitrary string
without identifier validation, then passed directly into `granularity_sqla`.
This allows invalid or unsafe values to reach query-building paths and can
cause runtime query failures. Constrain it to a safe column-name pattern (like
other column references) to reject malformed input early.
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=ee52a5c732d823eb2551bb0df7af8c39f376c61714b03496554019f78b3d77a8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38403&comment_hash=ee52a5c732d823eb2551bb0df7af8c39f376c61714b03496554019f78b3d77a8&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]
