mikebridge commented on code in PR #44266:
URL: https://github.com/apache/superset/pull/44266#discussion_r4048116840
##########
superset/mcp_service/semantic_layer/schemas.py:
##########
@@ -105,17 +111,39 @@ class ListMetricsRequest(BaseModel):
description="Filter to metrics from a specific semantic view.",
)
include_compatible_dimensions: bool = Field(
- default=True,
+ default=False,
description=(
- "When True, each metric includes its list of compatible
dimensions. "
- "Set to False to reduce response size when dimensions aren't
needed."
+ "Embed compatible dimensions only when explicitly requested. "
+ "Use get_compatible_dimensions for the full per-metric list. "
+ "When True, set page_size to at most 8 unless scoped to a built-in
"
+ "dataset with dataset_id."
),
)
page: int = Field(default=1, ge=1, description="1-based page number.")
page_size: int = Field(
- default=50, ge=1, le=500, description="Number of metrics per page."
+ default=25, ge=1, le=500, description="Number of metrics per page."
)
+ @model_validator(mode="after")
+ def validate_embedded_dimensions_page_size(self) -> "ListMetricsRequest":
+ """Reject embedded pages that risk exceeding the MCP response guard."""
+ if (
+ self.include_compatible_dimensions
+ and self.dataset_id is None
Review Comment:
Good catch — removed the dataset_id exemption in
https://github.com/apache/superset/commit/3015741aa82b28aa498fb3d37bd95aef411857bd.
Built-in and external embedded requests both require page_size <= 8; no
columns are truncated and the response shape is unchanged. Non-embedded
requests retain the 500 ceiling. The schema and upgrade guidance reflect this;
the fixed cap remains independent of the configured token limit and is not a
universal payload-fit guarantee.
##########
tests/unit_tests/mcp_service/semantic_layer/tool/test_list_metrics.py:
##########
@@ -476,6 +628,38 @@ async def
test_list_metrics_page_size_over_max_rejected(mcp_server: FastMCP) ->
await client.call_tool("list_metrics", {"request": {"page_size":
501}})
[email protected]
[email protected]("page_size", [9, 25, 500])
+async def test_builtin_embedded_metrics_keep_normal_page_ceiling(
+ mcp_server: FastMCP, page_size: int
+) -> None:
+ """Built-in-only discovery is not subject to the external embedding cap."""
+ mock_ds: MagicMock = _make_dataset(42)
+ with _patched_dataset_lookup(mock_ds):
+ async with Client(mcp_server) as client:
+ result: Any = await client.call_tool(
+ "list_metrics",
+ {
+ "request": {
+ "dataset_id": 42,
+ "page_size": page_size,
+ "include_compatible_dimensions": True,
+ }
+ },
+ )
+ data: dict[str, Any] = json.loads(result.content[0].text)
+ assert data["success"] is True
+ assert data["page_size"] == page_size
+ assert data["metrics"][0]["compatible_dimensions"]
Review Comment:
Agreed — replaced the acceptance-only regression with oversized-page
rejection and a 20-metric x 30-column fixture in
https://github.com/apache/superset/commit/3015741aa82b28aa498fb3d37bd95aef411857bd.
It checks all three pages (8/8/4), all 20 distinct metrics, all 30 columns per
metric, and estimate_response_tokens < DEFAULT_TOKEN_LIMIT on every page. Four
regression cases failed before the fix; the two affected suites pass 135 tests
afterward.
--
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]
