rebenitez1802 commented on code in PR #45504:
URL: https://github.com/apache/superset/pull/45504#discussion_r4337135548
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -292,10 +293,10 @@ async def duplicate_dashboard(
"""
Duplicate an existing dashboard under a new title.
Review Comment:
🟡 Medium — The compact tool listing drops the new default
With tool search on (the default, 300-character budget),
`_truncate_description` keeps only this first paragraph. The compact
`duplicate_dashboard` description goes from 257 chars, which included the
default, to just "Duplicate an existing dashboard under a new title."
(`compact_description_chars: 50` in `native_tool_inventory.json`). Agents in
compact or summary mode never learn that charts are now deep-copied by default,
and that's the main behavior change here. Moving it into the first paragraph
fixes it: I ran the PR's `_truncate_description` on this version and the
compact description is 112 chars and includes the default. The duplicate,
registration and config tests still pass. You'll need to regenerate the
inventory (`SUPERSET_MCP_UPDATE_TOOL_INVENTORY=1 pytest
tests/unit_tests/mcp_service/test_native_tool_surface.py`), and it would be
worth asserting on the truncated output in
`test_duplicate_description_explains_isolated_default`.
```suggestion
Duplicate a dashboard under a new title; charts are deep-copied by
default (duplicate_slices=false shares them).
```
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -440,6 +440,24 @@ def _big_number_headline(
)
+def _with_query_post_processing(response: ChartData, query_context: Any) ->
ChartData:
+ """Carry each executed query's operations to the composed dashboard
summary.
+
+ Use the context passed to the command, not the saved chart's viz type or
+ form data: saved, rebuilt and mixed queries can run different pipelines.
+ Private metadata survives response preflight without enlarging the wire
payload.
+ """
+ response._query_post_processing = [
+ [
+ str(step["operation"])
+ for step in getattr(query, "post_processing", None) or []
+ if isinstance(step, dict) and step.get("operation")
Review Comment:
🟡 Medium — `pivot` is always treated as row-collapsing, so most timeseries
charts now report `truncated=true`
The timeseries builder emits `pivot` for every chart with an x-axis and
metrics, even with no groupby (`common/form_data_query_context.py:1129`,
`"columns": []`). A pivot without `columns` keeps one row per index value, but
`_may_collapse_rows` only sees the operation name. So a 12-point monthly line
chart under a 100-row limit, which used to report `total_rows=12,
truncated=false`, now reports `total_rows=null, truncated=true` plus the
"verify source aggregates" note. That holds for nearly every line, bar and area
chart, so `truncated` stops being useful. The docs (`mcp-server.mdx`) already
say this applies to "timeseries pivots with groupby dimensions", so this
suggestion makes the code match. Charts with a groupby pivot (your
`test_timeseries_groupby_pivot_does_not_claim_complete_input`) and Big Number
trendlines (resample/rolling) stay on the conservative path. I ran
`test_get_dashboard_data.py` and `test_get_chart_data.py` with this change: 377
passed.
```suggestion
if isinstance(step, dict)
and step.get("operation")
# A pivot without columns keeps one row per index value.
and not (
step["operation"] == "pivot"
and not (step.get("options") or {}).get("columns")
)
```
A more precise follow-up: `sql_rowcount` is measured on the raw dataframe
before post-processing (`models/helpers.py`) and returned in the results
payload. Carrying it with the executed `row_limit` would give an exact
`truncated = sql_rowcount >= row_limit` for every chart, including histograms
and box plots, with the operation-name heuristic as the fallback.
--
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]
