gabotorresruiz commented on code in PR #45149:
URL: https://github.com/apache/superset/pull/45149#discussion_r4250991531
##########
superset/mcp_service/server.py:
##########
@@ -746,29 +755,66 @@ async def search_tools(
tool = Tool.from_function(fn=search_tools,
name=transform._search_tool_name)
return _fix_search_tool_query(tool)
- def _promote_exact_name(
+ def _promote_name_matches(
tools: Sequence[Tool],
query: str,
ranked: Sequence[Tool],
max_results: int,
+ *,
+ cover_query_words: bool = False,
) -> Sequence[Tool]:
- """Promote caller-visible exact names without duplicating ranked
matches."""
+ """Promote tools whose names the query spells out, within
``max_results``.
+
+ BM25 indexes a tool's name as ordinary text next to its description, so
+ a tool with a long definition can rank below the result limit even when
+ the query contains every word of its name. Two kinds of name match are
+ moved ahead of the upstream ranking:
+
+ 1. Exact names, which always come first.
+ 2. With ``cover_query_words`` (natural-language search only),
multi-word
+ names whose words appear in the query as a contiguous, in-order run,
+ for example ``get_dataset_info`` for "get dataset info execute sql",
+ ordered by name length and then by upstream rank. Words scattered
+ across the query do not count.
+
+ Only caller-visible candidates are inspected, never the full catalog,
+ and the result never exceeds ``max_results``.
+ """
normalized_query = " ".join(query.casefold().replace("_", " ").split())
exact = [
tool
for tool in tools
if " ".join(tool.name.casefold().replace("_", " ").split())
== normalized_query
]
- if not exact:
- return ranked
- # Only inspect the caller-filtered candidates, never the full catalog.
- # The upstream top-N contains enough non-exact results to fill the
- # remaining slots, without changing upstream ordering or shared limits.
exact_names = {tool.name for tool in exact}
+ covered: list[Tool] = []
+ if cover_query_words:
+ query_words = _name_words(query)
+ ranked_position = {tool.name: index for index, tool in
enumerate(ranked)}
+ covered = sorted(
+ (
+ tool
+ for tool in tools
+ if tool.name not in exact_names
+ and len(words := _name_words(tool.name)) > 1
+ and any(
+ query_words[start : start + len(words)] == words
+ for start in range(len(query_words) - len(words) + 1)
+ )
+ ),
+ key=lambda tool: (
+ -len(_name_words(tool.name)),
+ ranked_position.get(tool.name, len(ranked_position)),
Review Comment:
Not a blocker, just a question about the tie break.
`ranked_position.get(tool.name, len(ranked_position))` hands the same sentinel
to every covered tool that BM25 did not rank, so when more names are covered
than fit in `max_results` the one that gets cut is decided by catalog
registration order rather than by anything in the query. With the registered
catalog, `generate chart get chart info get dataset info get dashboard info
list dashboards execute sql` drops `execute_sql`; hand `_search` the same
catalog list reversed and the same query drops `generate_chart` instead. Would
breaking those ties by where the name run starts in the query be more
defensible, so the names mentioned first survive the cut?
```python
key=lambda match: (
-len(match.words),
match.run_start,
ranked_position.get(match.tool.name, len(ranked_position)),
),
```
Collecting `(tool, words, run_start)` in the filter would also save
recomputing `_name_words(tool.name)` inside the key.
##########
superset/mcp_service/server.py:
##########
@@ -746,29 +755,66 @@ async def search_tools(
tool = Tool.from_function(fn=search_tools,
name=transform._search_tool_name)
return _fix_search_tool_query(tool)
- def _promote_exact_name(
+ def _promote_name_matches(
tools: Sequence[Tool],
query: str,
ranked: Sequence[Tool],
max_results: int,
+ *,
+ cover_query_words: bool = False,
) -> Sequence[Tool]:
- """Promote caller-visible exact names without duplicating ranked
matches."""
+ """Promote tools whose names the query spells out, within
``max_results``.
+
+ BM25 indexes a tool's name as ordinary text next to its description, so
+ a tool with a long definition can rank below the result limit even when
+ the query contains every word of its name. Two kinds of name match are
+ moved ahead of the upstream ranking:
+
+ 1. Exact names, which always come first.
+ 2. With ``cover_query_words`` (natural-language search only),
multi-word
+ names whose words appear in the query as a contiguous, in-order run,
+ for example ``get_dataset_info`` for "get dataset info execute sql",
+ ordered by name length and then by upstream rank. Words scattered
+ across the query do not count.
+
+ Only caller-visible candidates are inspected, never the full catalog,
+ and the result never exceeds ``max_results``.
+ """
normalized_query = " ".join(query.casefold().replace("_", " ").split())
exact = [
tool
for tool in tools
if " ".join(tool.name.casefold().replace("_", " ").split())
== normalized_query
]
- if not exact:
- return ranked
- # Only inspect the caller-filtered candidates, never the full catalog.
- # The upstream top-N contains enough non-exact results to fill the
- # remaining slots, without changing upstream ordering or shared limits.
exact_names = {tool.name for tool in exact}
+ covered: list[Tool] = []
+ if cover_query_words:
+ query_words = _name_words(query)
+ ranked_position = {tool.name: index for index, tool in
enumerate(ranked)}
+ covered = sorted(
+ (
+ tool
+ for tool in tools
+ if tool.name not in exact_names
+ and len(words := _name_words(tool.name)) > 1
+ and any(
+ query_words[start : start + len(words)] == words
Review Comment:
Small one, and possibly intended: a shorter name's words can themselves be a
contiguous run inside a longer name's words, so spelling out
`create_dataset_metric` promotes `create_dataset` as well and one intent takes
two of the five slots. The catalog has four such pairs
(`create_dataset_metric`/`create_dataset`,
`delete_dataset_metric`/`delete_dataset`,
`update_dataset_metric`/`update_dataset`,
`update_chart_preview`/`update_chart`). On `create dataset metric for revenue`
that second promotion is what pushes `list_metrics` out of the results. Worth
skipping a covered name when another covered name already contains it, or is
the extra one useful context?
--
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]
