codeant-ai-for-open-source[bot] commented on code in PR #40775:
URL: https://github.com/apache/superset/pull/40775#discussion_r3373700660
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1417,6 +1417,32 @@ class BigNumberChartConfig(UnknownFieldCheckMixin):
),
ge=1,
)
+ aggregation: (
+ Literal["LAST_VALUE", "sum", "mean", "min", "max", "median", "raw"] |
None
+ ) = Field(
+ None,
+ description=(
+ "How the single big-number value is computed from the trendline "
+ "data points. Only applies when show_trendline=True. "
+ "Options: "
+ "'sum' = Total (Sum) — add all data points; use for all-time
totals. "
+ "'LAST_VALUE' = most recent data point "
+ "(frontend default when this field is absent). "
+ "'mean' = Average (Mean). "
+ "'min' = Minimum. "
+ "'max' = Maximum. "
+ "'median' = Median. "
+ "'raw' = Overall value — single aggregate across the full period;
best for "
+ "non-additive metrics like ratios, averages, or distinct counts. "
+ "DIAGNOSIS: if a Big Number with Trendline shows an unexpectedly
low value "
+ "(e.g. yesterday's revenue instead of all-time total), "
+ "inspect form_data['aggregation'] "
+ "— when absent or 'LAST_VALUE' the chart shows only the last data
point. "
+ "Fix by setting aggregation='sum'. "
+ "IMPORTANT: when updating aggregation, always include "
+ "show_trendline=True and temporal_column to preserve the
trendline."
+ ),
+ )
Review Comment:
**Suggestion:** The new `aggregation` field is accepted even when
`show_trendline` is false, but `map_big_number_config` only writes
`aggregation` for trendline charts. This creates a silent no-op (and in update
flows can unintentionally keep/revert behavior without warning). Add
cross-field validation so setting `aggregation` requires `show_trendline=True`
(and a temporal column), returning a validation error instead of silently
ignoring the field. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Big number explore link ignores requested aggregation configuration.
- ⚠️ LLM callers see successful responses with unintended aggregation.
- ⚠️ No validation error when aggregation used without trendline.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the MCP Explore tool `generate_explore_link` defined in
`superset/mcp_service/explore/tool/generate_explore_link.py` (lines 47-58,
163-119) with a
`GenerateExploreLinkRequest` whose `config` has `chart_type: "big_number"`,
a valid
`metric`, `show_trendline: false` (or omitted so it defaults to False), and
`aggregation:
"sum"`.
2. Pydantic parses the request into a `GenerateExploreLinkRequest` whose
`config` is a
`BigNumberChartConfig` (model defined in
`superset/mcp_service/chart/schemas.py`, lines
1328-1527); there is a `model_validator` enforcing `compare_lag` requires
`show_trendline`
(lines 124-139) but no validator that forbids `aggregation` when
`show_trendline` is
False, so the instance is accepted with `aggregation="sum"` and
`show_trendline=False`.
3. Inside `generate_explore_link`, the code maps the typed config to form
data via
`map_config_to_form_data` in `superset/mcp_service/chart/chart_utils.py`
(lines 314-345);
since `config` is a `BigNumberChartConfig`, it falls into the `elif
isinstance(config,
BigNumberChartConfig)` branch and calls `map_big_number_config(config)`
regardless of the
`aggregation` value.
4. In `map_big_number_config` (`superset/mcp_service/chart/chart_utils.py`,
lines 821-47),
`viz_type` is set to `"big_number_total"` because `config.show_trendline` is
False, so the
trendline-specific block (`if viz_type == "big_number": ...`) that writes
`form_data["aggregation"] = config.aggregation` (lines 25-43) is never
executed; the
returned `form_data` omits `aggregation`, and subsequent validation and URL
generation
complete successfully, so the caller sees a successful Explore link even
though their
`aggregation="sum"` setting was silently ignored.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=29e3f9fcbac34e0a8959660cbf8ee732&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=29e3f9fcbac34e0a8959660cbf8ee732&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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:** 1420:1445
**Comment:**
*Incomplete Implementation: The new `aggregation` field is accepted
even when `show_trendline` is false, but `map_big_number_config` only writes
`aggregation` for trendline charts. This creates a silent no-op (and in update
flows can unintentionally keep/revert behavior without warning). Add
cross-field validation so setting `aggregation` requires `show_trendline=True`
(and a temporal column), returning a validation error instead of silently
ignoring the field.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40775&comment_hash=a5e07f13fa00f7cba31b93d6734aed14306fcb96af6a012151445070f05375cb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40775&comment_hash=a5e07f13fa00f7cba31b93d6734aed14306fcb96af6a012151445070f05375cb&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]
