gabotorresruiz commented on code in PR #44951:
URL: https://github.com/apache/superset/pull/44951#discussion_r4187107845
##########
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:
Not a blocker, but worth a decision before this ships: `SqlaTable.columns`
declares no `order_by` (`superset/connectors/sqla/models.py:1577`), and I
captured the SQL the tool's eager options actually emit; the `table_columns`
subquery ends at `JOIN table_columns ON tables_1.id = table_columns.table_id`,
with no `ORDER BY`. So `all_columns[:max_columns]` keeps whatever row order the
engine happened to return. At the 100 default that only shows up past 100
columns, which is why it has never mattered. Once a caller sets `max_columns=5`
it gets an arbitrary 5 of N on every call, and the response reports
`total_column_count` and `columns_truncated` but not which 5, so an agent
hunting a filter column can miss the `is_dttm` one and two identical calls are
not guaranteed to agree.
```python
all_columns = sorted(
getattr(datasource, "columns", None) or [],
key=lambda column: getattr(column, "column_name", "") or "",
)
```
That does shift which 100 survive by default on very wide datasets, so an
`order_by` on the eager load option is the more conservative variant (it would
miss the semantic view path though). Either way, a shuffled input case in
`test_get_dashboard_datasets_max_columns` would pin whichever you pick; the
existing cases build their columns in the order they then assert, so they
cannot catch this. Or am I misreading how the relationship gets loaded here?
##########
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:
Observation rather than a blocker. I drove the tool over the real ASGI
transport with the production 50,000 byte guard against a dashboard of 10
datasets carrying 120 columns and 50 metrics each: the default cap gives
174,539 bytes and the "reduce `max_columns`" advice, and `max_columns=0` gives
87,559 bytes and lands on this branch, telling the caller there is nothing left
to reduce. The metrics are the floor on that shape, and they are capped at 50
with no caller-facing knob, so the new parameter cannot get such a caller under
the limit at all and this message is the only exit.
The description is upfront that the change bounds column details and not
total bytes, so this is really a scoping question: is a `max_metrics` sibling
(same shape, `MAX_DASHBOARD_DATASET_METRICS` as both ceiling and default) worth
folding in here, or deliberately left for a follow up?
--
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]
