gabotorresruiz commented on code in PR #45547:
URL: https://github.com/apache/superset/pull/45547#discussion_r4340573173
##########
superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:
##########
@@ -799,10 +801,11 @@ async def apply_dashboard_filters(
) -> ApplyDashboardFiltersResponse:
"""
Apply values to a dashboard's existing native filters for this user.
+ If a value is rejected, propose the listed value the user likely meant
+ and ask before applying it.
Use this to answer "show me the dashboard filtered to X". It returns a
- shareable ``/dashboard/p/<key>/`` permalink that opens the dashboard
- with the requested values already applied. The dashboard's saved
+ shareable ``/dashboard/p/<key>/`` permalink. The dashboard's saved
Review Comment:
This is the one spot where the change gives something up rather than adding
it: the full docstring loses "that opens the dashboard with the requested
values already applied", not just the compact cut. I think that is the budget
and not intent. This entry measures 3,672 against its `TOOL_BUDGETS` number of
3,700, so 28 bytes are left, and putting the clause back in a later paragraph
(which the 300 char compact cut never reaches) costs 77. The rule in the
comment above `TOOL_BUDGETS` would put this tool at 3,800. Worth bumping it and
keeping the clause?
##########
docs/admin_docs/configuration/mcp-server.mdx:
##########
@@ -190,7 +190,12 @@ selection the control cannot render.
Select values are checked against the target column's distinct values under the
caller's dataset permissions and row-level security. Exact values are
preserved;
a unique match ignoring case and whitespace uses the stored column value.
-Missing values fail without creating a permalink, with bounded suggestions.
+Missing values fail without creating a permalink. Unless an active dashboard
+scope suppresses them, the error lists bounded candidate values (closest
+matches, or the column's first values), and the tool description directs
Review Comment:
Two small things on this parenthetical, neither blocking.
Both lists can now appear together, which is exactly what the `any` to `all`
flip buys: with `["Emeaa", "APAC"]` the error carries `Closest matches for
'Emeaa': 'EMEA'.` and the `Available values` list in the same message. So "or"
undersells the change.
And "the column's first values" is not quite what gets listed.
`_unknown_values_error` does `sorted(candidates,
key=_value_label)[:_MAX_VALUE_SUGGESTIONS]`, and `values_for_column` issues a
`SELECT DISTINCT ... LIMIT` with no `ORDER BY`, so the ten shown are the
label-order first of an unordered page. On a 12 value column whose rows come
back `Zeta, Yankee, ..., Alpha, Bravo`, the error lists `Alpha` through `Xray`
and drops `Yankee` and `Zeta`, which are the column's actual first values.
```suggestion
matches, and up to ten values in label order), and the tool description
directs
```
##########
tests/unit_tests/mcp_service/native_tool_inventory.json:
##########
@@ -1267,12 +1267,12 @@
},
"totals": {
"catalog_bytes": {
- "structured": 524459,
- "text_only": 214456
+ "structured": 524939,
+ "text_only": 214936
Review Comment:
These four totals are the only thing that conflicts with master, which moved
them too. Worth regenerating with `SUPERSET_MCP_UPDATE_TOOL_INVENTORY=1` after
the merge rather than picking a side by hand: `size_budget` in
`test_native_tool_surface.py` allows `recorded + max(200, 2%)`, which is
roughly 10.5 KB at this scale, so keeping master's numbers would still pass CI
while recording the wrong catalog size.
--
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]
