gabotorresruiz commented on code in PR #44603:
URL: https://github.com/apache/superset/pull/44603#discussion_r4170675030
##########
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:
Just a small NIT, take it or leave it: `_validate_saved_metrics` still rides
this template, so a bad saved metric comes back as `Column 'no_such_metric' not
found in dataset` with `Check column name spelling and case sensitivity` above
the metric list. Now that there are four dedicated column templates, a sibling
entry that says metric would read better for the one error type that is
definitely not about a column.
##########
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:
Question rather than a finding: is the `aggregate is not None` gate
deliberate?
A `y` entry with no `aggregate` is a legal metric slot on this branch. I
verified that `y: [{"name": "revenue"}]` returns `success: true` against a
dataset where `revenue` is a plain column, so the slot does not have to carry
an aggregate. With `y: [{"name": "sum_boy"}]` against a dataset owning
`sum_boys`, the ref still dead-ends on `No matching columns found.`, which is
the dead end this hint exists to remove; add the `aggregate` and the hint
appears.
I can see why the gate is here: `_extract_column_references` flattens slots,
and dropping it would hand the hint to a dimension ref too, which
`test_dimension_near_miss_never_points_at_a_saved_metric` rightly forbids. If
threading slot identity through is too much for this PR, would it be worth
saying in the new docs section that the hint needs an explicit `aggregate`? Or
am I missing a reason the no-aggregate shape cannot reach here?
##########
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:
Not a blocker, and it is the same point from Rafael's review about the
column list, just on its sibling.
`available_columns` gets candidate-first ranking plus the `Showing 10 of N
columns` notice, but `available_metrics` gets neither. I ran a dataset with 25
saved metrics where the near-missed one sits at index 20: the error says `Did
you mean the saved metric 'sum_boys'?` while
`dataset_context.available_metrics` comes back as `metric_00` through
`metric_09`, with nothing saying the metric list was cut. So the error names a
metric that its own metric list appears to deny, which is the trust problem the
column ranking was added to avoid.
`_build_column_error` already has `metric_hints` in hand, so passing those
names into `_bounded_error_context` next to `candidates` and ranking them here
the same way `ranked` is built above would close half of it, and a `Showing 10
of N saved metrics` line next to the column one closes the rest. A test shaped
like `test_truncated_context_says_how_many_columns_exist` but with 25 metrics
and the hinted name beyond the bound would lock it in.
--
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]
