gabotorresruiz commented on code in PR #45153:
URL: https://github.com/apache/superset/pull/45153#discussion_r4264839398
##########
superset/mcp_service/utils/query_utils.py:
##########
@@ -15,12 +15,24 @@
# specific language governing permissions and limitations
# under the License.
-"""Backwards-compatible re-export of the shared name validator.
+"""Shared name validation for MCP tools.
-The implementation now lives in ``superset.common.tabular_query`` so the REST
+The name-validator implementation lives in ``superset.common.tabular_query``
so the REST
endpoint and the MCP tools cannot drift apart.
"""
from superset.common.tabular_query import validate_names
-__all__ = ["validate_names"]
+__all__: list[str] = ["validate_names", "validate_selection_names"]
+
+
+def validate_selection_names(
+ selected_metrics: list[str],
+ selected_dimensions: list[str],
+ valid_metrics: set[str],
+ valid_dimensions: set[str],
+) -> list[str]:
+ """Report unknown selected members using the shared validation messages."""
+ return validate_names(selected_metrics, valid_metrics, "metric") +
validate_names(
+ selected_dimensions, valid_dimensions, "dimension"
+ )
Review Comment:
Not a blocker, more a parity question: should the external path hand back
the same recovery hint the SQL path already does? The SQL branch of this same
tool passes `list_valid_on_miss=True` and `full_list_hint="call list_metrics
for the full list"` (`get_compatible_metrics.py:161`), and
`validate_query_names` does the same for `get_table` on both sources, so an
unknown metric with no close match reads `Unknown metric: 'zzzzzzzz'. Valid
metrics: bookings, revenue` there but just `Unknown metric: 'zzzzzzzz'` here.
Since the caller is an agent, that list is usually what lets it retry without
another round trip. Leaving the dimension half bare keeps it matching the SQL
path, which is also bare.
```suggestion
def validate_selection_names(
selected_metrics: list[str],
selected_dimensions: list[str],
valid_metrics: set[str],
valid_dimensions: set[str],
) -> list[str]:
"""Report unknown selected members using the shared validation
messages."""
return validate_names(
selected_metrics,
valid_metrics,
"metric",
list_valid_on_miss=True,
full_list_hint="call list_metrics for the full list",
) + validate_names(selected_dimensions, valid_dimensions, "dimension")
```
--
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]
