aminghadersohi commented on code in PR #38416:
URL: https://github.com/apache/superset/pull/38416#discussion_r2930716476
##########
superset/mcp_service/chart/tool/list_charts.py:
##########
@@ -106,34 +129,50 @@ def _serialize_chart(
"""Serialize chart object (field filtering handled by
model_serializer)."""
return serialize_chart_object(cast(ChartLike | None, obj))
- tool = ModelListCore(
- dao_class=ChartDAO,
- output_schema=ChartInfo,
- item_serializer=_serialize_chart,
- filter_type=ChartFilter,
- default_columns=DEFAULT_CHART_COLUMNS,
- search_columns=[
- "slice_name",
- "description",
- ],
- list_field_name="charts",
- output_list_schema=ChartList,
- all_columns=all_columns,
- sortable_columns=CHART_SORTABLE_COLUMNS,
- logger=logger,
- )
-
try:
- with event_logger.log_context(action="mcp.list_charts.query"):
- result = tool.run_tool(
- filters=request.filters,
- search=request.search,
- select_columns=request.select_columns,
- order_column=request.order_column,
- order_direction=request.order_direction,
- page=max(request.page - 1, 0),
- page_size=request.page_size,
+ # Two-pass approach when sorting by popularity_score
+ if request.order_column == "popularity_score":
+ with
event_logger.log_context(action="mcp.list_charts.popularity_sort"):
+ result = _list_charts_by_popularity(
+ request, ChartDAO, _serialize_chart, all_columns, ctx
+ )
+ else:
+ list_core = ModelListCore(
+ dao_class=ChartDAO,
+ output_schema=ChartInfo,
+ item_serializer=_serialize_chart,
+ filter_type=ChartFilter,
+ default_columns=DEFAULT_CHART_COLUMNS,
+ search_columns=CHART_SEARCH_COLUMNS,
+ list_field_name="charts",
+ output_list_schema=ChartList,
+ all_columns=all_columns,
+ sortable_columns=CHART_SORTABLE_COLUMNS,
+ logger=logger,
)
+
+ # Strip computed fields before passing to DAO query
+ dao_columns = request.select_columns
+ if dao_columns:
+ dao_columns = [c for c in dao_columns if c !=
"popularity_score"]
+
+ with event_logger.log_context(action="mcp.list_charts.query"):
+ result = list_core.run_tool(
+ filters=request.filters,
+ search=request.search,
+ select_columns=dao_columns,
+ order_column=request.order_column,
+ order_direction=request.order_direction,
+ page=max(request.page - 1, 0),
+ page_size=request.page_size,
+ )
+
+ # Attach popularity scores if requested in select_columns
+ if request.select_columns and "popularity_score" in
request.select_columns:
+ if chart_ids := [c.id for c in result.charts if c.id is not
None]:
+ scores = compute_chart_popularity(chart_ids)
+ attach_popularity_scores(result.charts, scores)
Review Comment:
Good catch — fixed in ecc8782. When `popularity_score` is in
`select_columns`, it's now re-added to `columns_to_filter` before serialization
so it's not silently dropped. Also ensures `id` is always loaded so scores can
be attached by ID.
##########
superset/mcp_service/dashboard/tool/list_dashboards.py:
##########
@@ -108,34 +124,49 @@ def _serialize_dashboard(
"""Serialize dashboard object (field filtering handled by
model_serializer)."""
return serialize_dashboard_object(obj)
- tool = ModelListCore(
- dao_class=DashboardDAO,
- output_schema=DashboardInfo,
- item_serializer=_serialize_dashboard,
- filter_type=DashboardFilter,
- default_columns=DEFAULT_DASHBOARD_COLUMNS,
- search_columns=[
- "dashboard_title",
- "slug",
- "uuid",
- ],
- list_field_name="dashboards",
- output_list_schema=DashboardList,
- all_columns=all_columns,
- sortable_columns=DASHBOARD_SORTABLE_COLUMNS,
- logger=logger,
- )
-
- with event_logger.log_context(action="mcp.list_dashboards.query"):
- result = tool.run_tool(
- filters=request.filters,
- search=request.search,
- select_columns=request.select_columns,
- order_column=request.order_column,
- order_direction=request.order_direction,
- page=max(request.page - 1, 0),
- page_size=request.page_size,
+ # Two-pass approach when sorting by popularity_score
+ if request.order_column == "popularity_score":
+ with
event_logger.log_context(action="mcp.list_dashboards.popularity_sort"):
+ result = _list_dashboards_by_popularity(
+ request, DashboardDAO, _serialize_dashboard, all_columns, ctx
+ )
+ else:
+ list_core = ModelListCore(
+ dao_class=DashboardDAO,
+ output_schema=DashboardInfo,
+ item_serializer=_serialize_dashboard,
+ filter_type=DashboardFilter,
+ default_columns=DEFAULT_DASHBOARD_COLUMNS,
+ search_columns=DASHBOARD_SEARCH_COLUMNS,
+ list_field_name="dashboards",
+ output_list_schema=DashboardList,
+ all_columns=all_columns,
+ sortable_columns=DASHBOARD_SORTABLE_COLUMNS,
+ logger=logger,
)
+
+ # Strip computed fields before passing to DAO query
+ dao_columns = request.select_columns
+ if dao_columns:
+ dao_columns = [c for c in dao_columns if c != "popularity_score"]
Review Comment:
Good catch — fixed in ecc8782. When `popularity_score` is in
`select_columns`, it's now re-added to `columns_to_filter` before serialization
so it's not silently dropped. Also ensures `id` is always loaded so scores can
be attached by ID.
##########
superset/mcp_service/dataset/tool/list_datasets.py:
##########
@@ -116,32 +129,50 @@ def _serialize_dataset(
"""Serialize dataset (filtering via model_serializer)."""
return serialize_dataset_object(obj)
- # Create tool with standard serialization
- tool = ModelListCore(
- dao_class=DatasetDAO,
- output_schema=DatasetInfo,
- item_serializer=_serialize_dataset,
- filter_type=DatasetFilter,
- default_columns=DEFAULT_DATASET_COLUMNS,
- search_columns=["schema", "sql", "table_name", "uuid"],
- list_field_name="datasets",
- output_list_schema=DatasetList,
- all_columns=all_columns,
- sortable_columns=DATASET_SORTABLE_COLUMNS,
- logger=logger,
- )
-
- with event_logger.log_context(action="mcp.list_datasets.query"):
- result = tool.run_tool(
- filters=request.filters,
- search=request.search,
- select_columns=request.select_columns,
- order_column=request.order_column,
- order_direction=request.order_direction,
- page=max(request.page - 1, 0),
- page_size=request.page_size,
+ # Two-pass approach when sorting by popularity_score
+ if request.order_column == "popularity_score":
+ with
event_logger.log_context(action="mcp.list_datasets.popularity_sort"):
+ result = _list_datasets_by_popularity(
+ request, DatasetDAO, _serialize_dataset, all_columns, ctx
+ )
+ else:
+ # Create tool with standard serialization
+ list_core = ModelListCore(
+ dao_class=DatasetDAO,
+ output_schema=DatasetInfo,
+ item_serializer=_serialize_dataset,
+ filter_type=DatasetFilter,
+ default_columns=DEFAULT_DATASET_COLUMNS,
+ search_columns=DATASET_SEARCH_COLUMNS,
+ list_field_name="datasets",
+ output_list_schema=DatasetList,
+ all_columns=all_columns,
+ sortable_columns=DATASET_SORTABLE_COLUMNS,
+ logger=logger,
)
+ # Strip computed fields before passing to DAO query
+ dao_columns = request.select_columns
+ if dao_columns:
+ dao_columns = [c for c in dao_columns if c !=
"popularity_score"]
+
+ with event_logger.log_context(action="mcp.list_datasets.query"):
+ result = list_core.run_tool(
+ filters=request.filters,
+ search=request.search,
+ select_columns=dao_columns,
+ order_column=request.order_column,
+ order_direction=request.order_direction,
+ page=max(request.page - 1, 0),
+ page_size=request.page_size,
+ )
+
+ # Attach popularity scores if requested in select_columns
+ if request.select_columns and "popularity_score" in
request.select_columns:
+ if ds_ids := [d.id for d in result.datasets if d.id is not
None]:
+ scores = compute_dataset_popularity(ds_ids)
+ attach_popularity_scores(result.datasets, scores)
Review Comment:
Good catch — fixed in ecc8782. When `popularity_score` is in
`select_columns`, it's now re-added to `columns_to_filter` before serialization
so it's not silently dropped. Also ensures `id` is always loaded so scores can
be attached by ID.
--
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]
