bito-code-review[bot] commented on code in PR #38416:
URL: https://github.com/apache/superset/pull/38416#discussion_r2930869461
##########
superset/mcp_service/chart/tool/list_charts.py:
##########
@@ -71,7 +92,7 @@ async def list_charts(request: ListChartsRequest, ctx:
Context) -> ChartList:
modified time.
Sortable columns for order_column: id, slice_name, viz_type,
- datasource_name, description, changed_on, created_on
+ datasource_name, description, changed_on, created_on, popularity_score
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing sortable column in schema</b></div>
<div id="fix">
The function docstring lists 'datasource_name' as a sortable column, but
CHART_SORTABLE_COLUMNS imported from schema_discovery.py does not include it.
This will cause sorting requests for datasource_name to fail with an error, as
the ModelListCore validates against the sortable_columns list. Since
datasource_name is a valid Slice model column and was previously sortable, it
should be included.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- "viz_type",
- "description",
+ "viz_type",
+ "datasource_name",
+ "description",
```
</div>
</details>
</div>
<small><i>Code Review Run #3a8f29</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/dashboard/tool/list_dashboards.py:
##########
@@ -108,34 +124,53 @@ 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"]
+ # Ensure id is loaded when popularity_score was requested
+ # (scores are keyed by id)
+ if "popularity_score" in request.select_columns and "id" not in
dao_columns:
+ dao_columns = ["id"] + dao_columns
+
+ with event_logger.log_context(action="mcp.list_dashboards.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
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Serialization Filtering Bug</b></div>
<div id="fix">
When sorting by non-popularity columns but requesting popularity_score, the
field gets attached but filtered out during serialization since
columns_requested excludes it. Update columns_requested to match the full
request.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
attach_popularity_scores(result.dashboards, scores)
result.columns_requested = request.select_columns
````
</div>
</details>
</div>
<small><i>Code Review Run #3a8f29</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -137,6 +137,12 @@ class DatasetInfo(BaseModel):
is_favorite: bool | None = Field(
None, description="Whether this dataset is favorited by the current
user"
)
+ popularity_score: float | None = Field(
+ None,
+ description="Popularity score based on chart usage, certification, "
+ "and recency. Request via select_columns=['popularity_score'] or sort "
+ "via order_column='popularity_score'.",
+ )
model_config = ConfigDict(
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing popularity_score computation</b></div>
<div id="fix">
The popularity_score field has been added to DatasetInfo but remains
unpopulated in get_dataset_info, always returning None. While list_datasets
computes this conditionally via select_columns, get_dataset_info should always
populate it for single dataset retrievals to match the field's documented
behavior.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- from superset.mcp_service.utils.schema_utils import parse_request
+ from superset.mcp_service.utils.schema_utils import parse_request
+ from superset.mcp_service.common.popularity import
compute_dataset_popularity
@@ -109,1 +110,4 @@
- result = tool.run_tool(request.identifier)
+ result = tool.run_tool(request.identifier)
+
+ # Compute popularity score for the retrieved dataset
+ if isinstance(result, DatasetInfo) and result.id:
+ scores = compute_dataset_popularity([result.id])
+ result.popularity_score = scores.get(result.id)
```
</div>
</details>
</div>
<small><i>Code Review Run #3a8f29</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/common/schema_discovery.py:
##########
@@ -449,6 +465,13 @@ def get_columns_from_model(
"charts": ColumnMetadata(
name="charts", description="Charts in dashboard", type="list",
is_default=False
),
+ "popularity_score": ColumnMetadata(
+ name="popularity_score",
+ description="Popularity score based on views, favorites, chart count, "
+ "and recency",
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inaccurate Column Description</b></div>
<div id="fix">
The description for dashboard popularity_score omits certification and
published status bonuses that are part of the scoring formula in
compute_dashboard_popularity.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
description="Popularity score based on views, favorites, chart
count, certification, published status, "
"and recency",
````
</div>
</details>
</div>
<small><i>Code Review Run #3a8f29</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
