aminghadersohi commented on code in PR #44951:
URL: https://github.com/apache/superset/pull/44951#discussion_r4187313757
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -2775,7 +2788,7 @@ def _serialize_dashboard_dataset(
type=getattr(column, "type", None),
is_dttm=getattr(column, "is_dttm", None),
)
- for column in all_columns[:MAX_DASHBOARD_DATASET_COLUMNS]
+ for column in all_columns[:max_columns]
Review Comment:
You read it right: neither relationship declares an `order_by`, so the slice
kept whatever order the engine returned. Fixed in
https://github.com/apache/superset/commit/1a7d700dcda85e536b4b13d5af65190e5d08461d.
`_serialize_dashboard_dataset` now sorts columns by `column_name` and metrics
by `metric_name` before the caps apply. I chose the in-serializer sort over an
`order_by` on the eager load because it also covers the semantic view path, and
the subset kept before was arbitrary anyway, so sorting changes nothing a
caller could have relied on.
`test_get_dashboard_datasets_caps_are_deterministic` feeds shuffled columns and
metrics to both a table and a semantic view and asserts the sorted prefix at
`max_columns` 2 and 100, with `max_metrics=2`.
`test_get_dashboard_datasets_multiple_datasets` was updated to the sorted order.
##########
superset/mcp_service/utils/response_size_utils.py:
##########
@@ -450,6 +459,22 @@ def _dashboard_layout_suggestions(
)
+def _dashboard_datasets_suggestions(query_params: Dict[str, Any]) -> List[str]:
+ """Suggest the column cap, or a fallback when columns are already
omitted."""
+ if query_params.get("max_columns") == 0:
+ return [
+ "Column details are already omitted. This tool cannot reduce "
Review Comment:
Agreed, that is a real gap and not just a scoping note: on that shape the
metrics were a floor the caller could not get under, so the "nothing left to
reduce" branch was the only way out. I added the `max_metrics` sibling here in
https://github.com/apache/superset/commit/1a7d700dcda85e536b4b13d5af65190e5d08461d.
It has the same shape as `max_columns`: 0 to 50, with
`MAX_DASHBOARD_DATASET_METRICS` as both the ceiling and the default, and the
total counts and `metrics_truncated` are kept. The size advice now names
whichever cap is still above 0. It falls back to the get_dashboard_info /
get_dataset_info route only when both caps are 0.
`test_metric_heavy_dashboard_fits_with_all_details_omitted` covers 10 datasets
× 120 columns × 50 metrics: with `max_columns=0` alone it is still over the
budget, and with both caps at 0 it fits. The response-size tests cover each
advice branch. A dashboard with very many datasets can still exceed the limit
with both caps at 0. The docs say so, and
bounding the dataset count is left out on purpose.
--
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]
