aminghadersohi commented on code in PR #44603:
URL: https://github.com/apache/superset/pull/44603#discussion_r4170735005
##########
superset/mcp_service/chart/validation/dataset_validator.py:
##########
@@ -678,46 +714,127 @@ def _get_column_suggestions(
return suggestions
+ @staticmethod
+ def _get_metric_suggestions(
+ name: str, dataset_context: DatasetContext, max_suggestions: int = 3
+ ) -> List[str]:
+ """Fuzzy-match *name* against saved metric names only."""
+ by_lowered = {
+ metric["name"].lower(): metric["name"]
+ for metric in dataset_context.available_metrics
+ }
+ return [
+ by_lowered[match]
+ for match in difflib.get_close_matches(
+ name.lower(), list(by_lowered), n=max_suggestions, cutoff=0.6
+ )
+ ]
+
+ @staticmethod
+ def _rank_candidates(
+ names: List[str],
+ suggestions_map: Dict[str, List[ColumnSuggestion]],
+ ) -> List[str]:
+ """Interleave each missing column's candidates, best match first.
+
+ The ``Did you mean`` cap is shared across every missing column, so a
+ flat in-order concatenation lets the first name consume every slot.
+ Round-robin gives each missing column a turn regardless of input order.
+ """
+ ranked: List[str] = []
+ per_name = [
+ [suggestion.name for suggestion in suggestions_map.get(name, [])]
+ for name in names
+ ]
+ for tier in zip_longest(*per_name):
+ for candidate in tier:
+ if candidate is not None and candidate not in ranked:
+ ranked.append(candidate)
+ return ranked
+
+ @staticmethod
+ def _saved_metric_hints(metric_hints: Dict[str, List[str]]) -> List[str]:
+ """Guidance for metric-slot refs that near-miss a saved metric name.
+
+ Only the dataset's own metric names appear, never the caller's input.
+ """
+ names = list(
+ dict.fromkeys(name for matches in metric_hints.values() for name
in matches)
+ )[:MAX_DID_YOU_MEAN_CANDIDATES]
+ if not names:
+ return []
+ quoted = ", ".join(f"'{name}'" for name in names)
+ return [
+ f"Did you mean the saved metric {quoted}? Reference a saved metric
"
+ 'as {"name": "<metric>", "saved_metric": true} rather than setting
'
+ '"aggregate".'
+ ]
+
+ @staticmethod
+ def _bounded_error_context(
+ dataset_context: DatasetContext, candidates: List[str]
+ ) -> DatasetContext:
+ """Names-only dataset context for a column error.
+
+ Names are returned verbatim per the Tool Result Value Contract; only
+ the number of entries is bounded, and SQL expressions are dropped.
+ Fuzzy candidates come first so the suggested column is never cut from
+ the list by the count bound.
+ """
+ all_names = [col["name"] for col in dataset_context.available_columns]
+ ranked = [name for name in candidates if name in all_names]
+ ranked += [name for name in all_names if name not in ranked]
+ return DatasetContext(
+ id=dataset_context.id,
+ table_name=dataset_context.table_name,
+ schema=dataset_context.schema_name,
+ database_name=dataset_context.database_name,
+ available_columns=[
+ {"name": name} for name in ranked[:MAX_ERROR_SUGGESTIONS]
+ ],
+ available_metrics=[
Review Comment:
Done in 96e5067: hinted saved metrics now lead `available_metrics` (same
candidate-first ranking as columns), and a partial metric list adds `Showing 10
of N saved metrics; call get_dataset_info for the full list`. Added a test with
25+ metrics where the hinted name sits past the bound.
##########
superset/mcp_service/chart/validation/dataset_validator.py:
##########
@@ -377,8 +382,25 @@ def _validate_columns_exist( # noqa: C901
)
suggestions_map[col_ref.name] = suggestions
+ # A ref in a metric slot that near-misses a saved metric name would
+ # otherwise dead-end on "No matching columns found.", because metrics
+ # are excluded from the column candidates above. Only reached when the
+ # physical-column pass found nothing, so the more direct column fix
+ # still wins when it exists.
+ metric_hints: Dict[str, List[str]] = {}
+ for col_ref in invalid_columns:
+ if col_ref.name is None or col_ref.aggregate is None:
Review Comment:
The gate is deliberate, for the reason you give:
`_extract_column_references` flattens slots, so without `aggregate` a
metric-slot ref can't be told apart from a dimension, and dimensions must never
be pointed at a saved metric. Threading slot identity through is out of scope
here, so the docs section now says the hint needs an explicit `aggregate`
(96e5067).
##########
superset/mcp_service/utils/error_builder.py:
##########
@@ -128,14 +147,32 @@ class ChartErrorBuilder:
"Use the list_datasets tool to find available datasets",
],
},
+ # Free-form ``{suggestions}`` text, used where the caller composes its
+ # own closing line (e.g. the saved-metric validator).
"column_not_found": {
"message": "Column '{column}' not found in dataset",
"details": "The column '{column}' does not exist in the dataset
schema",
- "suggestions": [
- "Check column name spelling and case sensitivity",
- "Use get_dataset_info to see available columns",
- "Did you mean: {suggestions}?",
- ],
+ "suggestions": [*_COLUMN_GUIDANCE, "{suggestions}"],
Review Comment:
Done in 96e5067: `_validate_saved_metrics` now uses a dedicated
`saved_metric_not_found` template ("Saved metric '...' not found in dataset",
metric-specific guidance), and `test_nonexistent_saved_metric_fails` asserts
the message.
--
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]
