aminghadersohi commented on code in PR #44266:
URL: https://github.com/apache/superset/pull/44266#discussion_r4045200909
##########
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:
Built-in dimensions cost the same per item as external; only the count
differs. dataset_id-scoped + embedded at the default page_size=25, with your
own fixture shape (20 metrics x 30 groupby columns): 32,196 tokens vs the
25,000 limit, and list_metrics can't truncate, so it's a hard ToolError.
##########
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:
_make_dataset is 2 metrics x 2 columns, so this pins acceptance only, not
size. The external sibling asserts estimate_response_tokens <
DEFAULT_TOKEN_LIMIT; the page_size=500 built-in case has no equivalent bound.
--
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]
