aminghadersohi commented on code in PR #43770:
URL: https://github.com/apache/superset/pull/43770#discussion_r4183996396
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2713,6 +2820,464 @@ def validate_unique_column_labels(self) ->
"XYChartConfig":
return self
+class BulletChartConfig(BaseChartConfig):
+ """Config for bullet charts (viz_type ``bullet``)."""
+
+ # Semantic field names are exposed to MCP clients; validation aliases and
the
+ # native adapter accept saved Explore ``form_data`` without weakening the
+ # unknown-field checks that catch misspelled controls.
+ model_config = ConfigDict(extra="ignore", populate_by_name=True)
+
+ chart_type: Literal["bullet"] = "bullet"
+ metric: ColumnRef = Field(
+ ...,
+ description="Numeric measure shown by each bullet bar",
+ )
+ dimensions: List[ColumnRef] | None = Field(
+ None,
+ validation_alias=AliasChoices("dimensions", "groupby"),
+ description=(
+ "Category hierarchy; one bullet row per unique combination. Omit
to "
+ "keep the saved hierarchy on update; [] clears it."
+ ),
+ max_length=20,
+ )
+ filters: List[FilterConfig] | None = Field(None, max_length=100)
+ time_range: str | None = Field(
+ None,
+ min_length=1,
+ max_length=1000,
+ description=(
+ "Superset time range, e.g. 'Last 30 days' or '2025-01-01 :
2025-12-31'"
+ ),
+ )
+ row_limit: int = Field(
+ 10000,
+ ge=1,
+ le=50000,
+ description="Maximum bullet rows",
+ )
+ order_by: List[SortByConfig] = Field(
+ default_factory=list,
+ validation_alias=AliasChoices("order_by", "orderby", "order_by_cols"),
+ max_length=20,
+ description="Row order by a dimension name or the metric output
label/name",
+ )
+
+ # Presentation fields map one-for-one onto Bullet/transformProps.ts
controls.
+ ranges: List[float] = Field(
+ default_factory=list,
+ max_length=100,
+ description="Qualitative range thresholds shaded behind the measure",
+ )
+ range_labels: List[str] = Field(
+ default_factory=list,
+ validation_alias=AliasChoices("range_labels", "rangeLabels"),
+ max_length=100,
+ )
+ markers: List[float] = Field(
+ default_factory=list,
+ max_length=100,
+ description="Target values drawn as point markers",
+ )
+ marker_labels: List[str] = Field(
+ default_factory=list,
+ validation_alias=AliasChoices("marker_labels", "markerLabels"),
+ max_length=100,
+ )
+ marker_lines: List[float] = Field(
+ default_factory=list,
+ validation_alias=AliasChoices("marker_lines", "markerLines"),
+ max_length=100,
+ description="Reference values drawn as vertical lines",
+ )
+ marker_line_labels: List[str] = Field(
+ default_factory=list,
+ validation_alias=AliasChoices("marker_line_labels",
"markerLineLabels"),
+ max_length=100,
+ )
+ y_axis_format: str = Field(
+ "SMART_NUMBER",
+ validation_alias=AliasChoices("y_axis_format", "yAxisFormat"),
+ max_length=100,
+ )
+ show_labels: bool = Field(
+ False,
+ validation_alias=AliasChoices("show_labels", "showLabels"),
+ )
+ show_legend: bool = Field(
+ False,
+ validation_alias=AliasChoices("show_legend", "showLegend"),
+ )
+
+ @staticmethod
+ def _adapt_native_metric(value: Any) -> Any:
+ """Translate QueryFormMetric shapes into the shared ColumnRef
contract."""
+ if isinstance(value, str):
+ return {"name": value, "saved_metric": True}
+ if not isinstance(value, dict):
+ return value
+ if "expressionType" not in value:
+ # QueryObject's documented legacy saved-metric representation is a
+ # label-only object. Keep this adapter deliberately narrow: objects
+ # carrying ad-hoc fields must declare expressionType explicitly,
and
+ # semantic ColumnRef objects continue through normal validation.
+ if set(value) == {"label"}:
+ label = value["label"]
+ if not isinstance(label, str) or not label or len(label) > 255:
+ raise ValueError(
+ "legacy saved metric label must be a non-empty string
of "
+ "at most 255 characters"
+ )
+ return {"name": label, "saved_metric": True}
+ return value
+ expression_type = value.get("expressionType")
+ if expression_type == "SQL":
+ return {
+ "sql_expression": value.get("sqlExpression"),
+ "label": value.get("label") or value.get("sqlExpression"),
+ }
+ if expression_type != "SIMPLE":
+ raise ValueError("metric.expressionType must be 'SIMPLE' or 'SQL'")
+ column = value.get("column")
+ if isinstance(column, dict):
+ name = column.get("column_name")
+ else:
+ name = column
+ return {
+ "name": name,
+ "aggregate": value.get("aggregate"),
+ "label": value.get("label"),
+ }
+
+ @staticmethod
+ def _canonical_dimension_alias(value: Any, field_name: str) -> list[str]:
+ """Canonicalize semantic/native dimension aliases for conflict
checks."""
+ if not isinstance(value, list):
+ raise ValueError(f"{field_name} must be an array")
+ canonical: list[str] = []
+ for index, item in enumerate(value):
+ name: str | None
+ if isinstance(item, str):
+ name = item
+ elif isinstance(item, ColumnRef):
+ name = item.name
+ elif isinstance(item, dict):
+ name = next(
+ (
+ item[key]
+ for key in ("name", "column_name", "column")
+ if isinstance(item.get(key), str)
+ ),
+ None,
+ )
+ else:
+ name = None
+ if not name:
+ raise ValueError(
+ f"{field_name}[{index}] must identify a physical column"
+ )
+ canonical.append(name)
+ return canonical
+
+ @staticmethod
+ def _adapt_native_order_by(value: Any) -> Any: # noqa: C901
+ if value is None:
+ return []
+ if not isinstance(value, list):
+ raise ValueError("order_by must be an array")
+ result: list[Any] = []
+ for index, entry in enumerate(value):
+ if isinstance(entry, str):
+ if len(entry) > 2000:
+ raise ValueError(f"order_by[{index}] is too long")
+ try:
+ entry = json.loads(entry)
+ except json.JSONDecodeError:
+ # A bare output/column name is the ergonomic typed form.
+ result.append({"column": entry, "ascending": False})
+ continue
+ if isinstance(entry, dict):
+ result.append(entry)
+ continue
+ if not isinstance(entry, (list, tuple)) or len(entry) != 2:
+ raise ValueError(
+ f"order_by[{index}] must be [column, ascending_boolean]"
+ )
+ target, ascending = entry
+ if isinstance(target, dict):
+ target = target.get("label") or target.get("metric_name")
+ if not isinstance(target, str) or not target:
+ raise ValueError(f"order_by[{index}] needs a column or metric
label")
+ if not isinstance(ascending, bool):
+ raise ValueError(f"order_by[{index}] ascending value must be
boolean")
+ result.append({"column": target, "ascending": ascending})
+ return result
+
+ @staticmethod
+ def _adapt_native_filters(data: dict[str, Any]) -> None: # noqa: C901
+ if "adhoc_filters" not in data:
+ return
+ if "filters" in data:
+ raise ValueError("Use either filters or native adhoc_filters, not
both")
+ raw_filters = data.pop("adhoc_filters")
+ if not isinstance(raw_filters, list):
+ raise ValueError("adhoc_filters must be an array")
+ filters: list[dict[str, Any]] = []
+ temporal_pairs: list[tuple[str, str]] = []
+ inert_subjects: set[str] = set()
+ for index, raw_filter in enumerate(raw_filters):
+ if not isinstance(raw_filter, dict):
+ raise ValueError(f"adhoc_filters[{index}] must be an object")
+ if raw_filter.get("expressionType") != "SIMPLE":
+ raise ValueError(
+ f"adhoc_filters[{index}] must use expressionType='SIMPLE'"
+ )
+ if raw_filter.get("clause") not in (None, "WHERE"):
+ raise ValueError(f"adhoc_filters[{index}] must use
clause='WHERE'")
+ subject = raw_filter.get("subject")
+ operator = raw_filter.get("operator")
+ comparator = raw_filter.get("comparator")
+ if operator == "TEMPORAL_RANGE":
+ if not isinstance(subject, str) or not subject:
+ raise ValueError(
+ f"adhoc_filters[{index}] temporal filter needs subject"
+ )
+ if not isinstance(comparator, str) or not comparator:
+ raise ValueError(
+ f"adhoc_filters[{index}] temporal filter needs a range"
+ )
+ if comparator.casefold() == "no filter":
+ inert_subjects.add(subject)
+ else:
+ temporal_pairs.append((subject, comparator))
+ continue
+ if not isinstance(operator, str):
+ raise ValueError(f"adhoc_filters[{index}] needs an operator")
+ operator_map = {
+ "==": "=",
+ "EQUALS": "=",
+ "NOT_EQUALS": "!=",
+ "LESS_THAN": "<",
+ "LESS_THAN_OR_EQUAL": "<=",
+ "GREATER_THAN": ">",
+ "GREATER_THAN_OR_EQUAL": ">=",
+ "NOT_IN": "NOT IN",
+ "IS_NULL": "IS NULL",
+ "IS_NOT_NULL": "IS NOT NULL",
+ }
+ operator = operator_map.get(operator, operator)
+ filters.append({"column": subject, "op": operator, "value":
comparator})
+ if len(temporal_pairs) > 1:
+ raise ValueError(
+ "Multiple active native temporal filters cannot be represented
by "
+ "a single temporal_column/time_range pair"
+ )
+ if temporal_pairs:
+ subject, comparator = temporal_pairs[0]
+ if data.get("temporal_column") not in (None, subject) or data.get(
+ "time_range"
+ ) not in (None, "No filter", comparator):
+ raise ValueError(
+ "Native temporal filter conflicts with
temporal_column/time_range"
+ )
+ data["temporal_column"] = subject
+ data["time_range"] = comparator
+ elif len(inert_subjects) == 1:
+ data.setdefault("temporal_column", next(iter(inert_subjects)))
+ data["filters"] = filters
+
+ @model_validator(mode="before")
+ @classmethod
+ def adapt_native_form_data(cls, raw: Any) -> Any: # noqa: C901
+ """Accept recognized saved Bullet form_data and reject ambiguous
state."""
+ if not isinstance(raw, dict):
+ return raw
+ data = dict(raw)
+ # Null means no hierarchy was supplied, not a conflicting alias or a
+ # request to clear saved dimensions. Only an explicit [] clears them.
+ for key in ("dimensions", "groupby"):
+ if data.get(key) is None:
+ data.pop(key, None)
+ if "dimensions" in data and "groupby" in data:
+ dimensions = cls._canonical_dimension_alias(
+ data["dimensions"], "dimensions"
+ )
+ groupby = cls._canonical_dimension_alias(data["groupby"],
"groupby")
+ if dimensions != groupby:
+ raise ValueError(
+ "Conflicting Bullet dimension aliases: 'dimensions' and "
+ "native 'groupby' must identify the same physical columns
in "
+ "the same order; provide only one or make them equivalent"
+ )
+ # Avoid relying on AliasChoices precedence or JSON key order.
+ data.pop("groupby")
+ if data.get("viz_type") == "bullet":
+ from superset.mcp_service.chart.preview_utils import (
+ _bullet_numeric_control_tokens,
+ )
+
+ data.setdefault("chart_type", "bullet")
+ data.pop("viz_type", None)
+ # Saved Explore controls are lenient; newly authored typed lists
+ # still undergo the strict List[float] validation below.
+ for control in ("ranges", "markers", "marker_lines"):
+ if control in data:
+ data[control] = _bullet_numeric_control_tokens(
+ data[control], control
+ )
+ for key in (
+ "annotation_layers",
+ "dashboards",
+ "datasource",
+ "datasource_id",
+ "datasource_type",
+ "extra_form_data",
+ "slice_id",
+ "slice_name",
+ ):
+ data.pop(key, None)
+
+ if (marker_key := "_mcp_dashboard_time_filter_subject") in data:
+ marker = data.pop(marker_key)
+ if not isinstance(marker, str) or not marker:
+ raise ValueError(f"{marker_key} must be a physical column
name")
+ raw_filters = data.get("adhoc_filters")
+ if not isinstance(raw_filters, list):
+ raise ValueError(
+ f"{marker_key} requires an adhoc_filters array containing
its "
+ "generated binding"
+ )
+ provenance_matches = [
+ filter_
+ for filter_ in raw_filters
+ if isinstance(filter_, dict)
+ and filter_.get("subject") == marker
+ and filter_.get("operator") == "TEMPORAL_RANGE"
+ ]
+ if len(provenance_matches) != 1:
+ raise ValueError(
+ f"{marker_key} must match exactly one TEMPORAL_RANGE
filter "
+ f"for subject {marker!r}; found {len(provenance_matches)}"
+ )
+ data.setdefault("temporal_column", marker)
+
+ if "metric" in data:
+ data["metric"] = cls._adapt_native_metric(data["metric"])
+ for key in ("groupby", "dimensions"):
+ if key in data:
+ if not isinstance(data[key], list):
+ raise ValueError(f"{key} must be an array")
+ data[key] = [
Review Comment:
Fixed in 2831272d9b3c33d67f25671bc0dc22e84f657bfd; the order_by case is now
covered too.
--
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]
