aminghadersohi commented on code in PR #43771:
URL: https://github.com/apache/superset/pull/43771#discussion_r3927150024
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1045,6 +1045,323 @@ def reject_sql_expression_on_dimensions(self) ->
"PieChartConfig":
return self
+class SunburstChartConfig(BaseChartConfig):
+ """Config for the ECharts Sunburst plugin (viz_type ``sunburst_v2``).
+
+ ``hierarchy`` follows the frontend ``columns`` control: the first entry is
+ the innermost ring and each later entry adds a child level. The primary
+ metric sizes arcs; an optional secondary metric colors arcs by the
+ secondary/primary ratio.
+ """
+
+ model_config = ConfigDict(extra="ignore", populate_by_name=True)
+
+ chart_type: Literal["sunburst"] = "sunburst"
+ viz_type: Literal["sunburst_v2"] = Field(
+ "sunburst_v2",
+ description="Exact Superset frontend visualization tag",
+ )
+ hierarchy: List[ColumnRef] = Field(
+ ...,
+ min_length=1,
+ description=(
+ "Hierarchy dimensions in ring order, from the innermost/root level
"
+ "to the outermost/leaf level"
+ ),
+ validation_alias=AliasChoices("hierarchy", "columns", "groupby"),
+ )
+ metric: ColumnRef = Field(
+ ...,
+ description=(
+ "Primary metric used to size arcs. Use aggregate for a SIMPLE "
+ "adhoc metric, saved_metric=True for a dataset metric, or "
+ "sql_expression plus label for a SQL metric."
+ ),
+ )
+ secondary_metric: ColumnRef | None = Field(
+ None,
+ description=(
+ "Optional metric used to color arcs by secondary/primary ratio. "
+ "When omitted, colors are categorical."
+ ),
+ validation_alias=AliasChoices("secondary_metric", "secondaryMetric"),
+ )
+ filters: List[FilterConfig] | None = Field(
+ None,
+ description=(
+ "Structured WHERE filters (column/op/value). An omitted list is "
+ "preserved on updates; an explicit [] clears saved filters."
+ ),
+ )
+ time_range: str | None = Field(
+ None,
+ min_length=1,
+ max_length=1000,
+ description=(
+ "Superset time range, for example 'Last year', "
+ "'2025-01-01 : 2025-12-31', or 'No filter'"
+ ),
+ )
+ time_grain: TimeGrain | None = Field(
+ None,
+ description="Optional bucket for temporal hierarchy columns",
+ validation_alias=AliasChoices("time_grain", "time_grain_sqla"),
+ )
+ sort_by_metric: bool = Field(
+ False,
+ description=(
+ "Order hierarchy rows by the primary metric descending before "
+ "applying row_limit, matching the frontend buildQuery transform"
+ ),
+ )
+ row_limit: int = Field(10000, description="Maximum hierarchy rows", ge=1,
le=50000)
+ color_scheme: str | None = Field(
+ None,
+ max_length=100,
+ description="Categorical scheme used when secondary_metric is omitted",
+ )
+ linear_color_scheme: str | None = Field(
+ None,
+ max_length=100,
+ description="Sequential scheme used when secondary_metric is present",
+ )
+ show_labels: bool = False
+ show_labels_threshold: float = Field(
+ 5,
+ ge=0,
+ le=100,
+ description="Minimum arc size in percentage points for showing a
label",
+ )
+ show_total: bool = False
+ show_null_values: bool = Field(
+ True,
+ description="Keep null-valued hierarchy nodes in the rendered tree",
+ )
+ label_type: Literal["key", "value", "key_value"] = "key"
+ number_format: str = Field("SMART_NUMBER", min_length=1, max_length=50)
+ date_format: str = Field("smart_date", min_length=1, max_length=50)
+ currency_format: CurrencyFormat | None = None
+
+ @staticmethod
+ def _looks_like_native_form_data(data: Any) -> bool:
+ """Identify saved Explore payloads without weakening typed typo
checks."""
+ if not isinstance(data, dict) or data.get("viz_type") != "sunburst_v2":
+ return False
+ metric = data.get("metric")
+ return (
+ any(
+ key in data
+ for key in (
+ "adhoc_filters",
+ "annotation_layers",
+ "datasource",
+ "extra_form_data",
+ "since",
+ "slice_id",
+ "standardizedFormData",
+ "until",
+ )
+ )
+ or isinstance(metric, str)
+ or (isinstance(metric, dict) and "expressionType" in metric)
+ )
+
+ @staticmethod
+ def _coerce_native_metric(value: Any) -> Any:
+ """Accept saved metric names and native SIMPLE/SQL metric objects."""
+ if isinstance(value, str):
+ return {"name": value, "saved_metric": True}
+ if not isinstance(value, dict) or "expressionType" not in value:
+ return value
+
+ expression_type = value.get("expressionType")
+ label = value.get("label") if value.get("hasCustomLabel", True) else
None
+ if expression_type == "SQL":
+ return {
+ "sql_expression": value.get("sqlExpression"),
+ "label": label,
+ }
Review Comment:
Addressed in the current head. `_coerce_native_metric` preserves a nonempty
native label and falls back to `sqlExpression` when `hasCustomLabel=false`;
`test_native_noncustom_sql_metric_uses_effective_frontend_label` verifies
validation and mapped form_data retain the effective label and provenance. The
full MCP suite passes at 5306ee42.
##########
superset/mcp_service/chart/tool/update_chart_preview.py:
##########
@@ -240,6 +245,11 @@ def update_chart_preview( # noqa: C901
if previous_form_data:
merge_table_column_config(previous_form_data, new_form_data)
merge_interactive_pivot_ui_config(previous_form_data,
new_form_data)
+ new_form_data = merge_form_data_for_update(
+ previous_form_data, new_form_data, config
+ )
Review Comment:
Addressed in the current head. Cross-viz updates start from the mapped
target and preserve only the bounded registry, so source query roles cannot
survive; same-viz updates strip the target role vocabulary before overlay.
`test_cross_viz_preview_and_compile_start_from_mapped_sunburst_roles` and
`test_cross_viz_preview_update_and_immediate_save_report_sunburst_state`
exercise divergent cached source roles through preview/compile/save.
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1044,6 +1045,109 @@ def map_pie_config(config: PieChartConfig) -> Dict[str,
Any]:
return form_data
+def map_sunburst_config(config: SunburstChartConfig) -> Dict[str, Any]:
+ """Map typed Sunburst config to the ECharts ``sunburst_v2`` form_data.
+
+ The frontend control panel stores hierarchy levels under ``columns`` and
+ metrics under singular ``metric`` / ``secondary_metric`` keys. Its
+ buildQuery adds primary-metric descending ordering when ``sort_by_metric``
+ is enabled; server-side query builders mirror that transform separately.
+ """
+ form_data: Dict[str, Any] = {
+ "viz_type": "sunburst_v2",
+ "columns": [dimension.name for dimension in config.hierarchy],
+ "metric": create_metric_object(config.metric),
+ "sort_by_metric": config.sort_by_metric,
+ "row_limit": config.row_limit,
+ "show_labels": config.show_labels,
+ "show_labels_threshold": config.show_labels_threshold,
+ "show_total": config.show_total,
+ "show_null_values": config.show_null_values,
+ "label_type": config.label_type,
+ "number_format": config.number_format,
+ "date_format": config.date_format,
+ }
+ if config.secondary_metric is not None:
+ form_data["secondary_metric"] =
create_metric_object(config.secondary_metric)
+ if config.color_scheme is not None:
+ form_data["color_scheme"] = config.color_scheme
+ if config.linear_color_scheme is not None:
+ form_data["linear_color_scheme"] = config.linear_color_scheme
+ if config.time_range is not None:
+ form_data["time_range"] = config.time_range
+ if config.time_grain is not None:
+ form_data["time_grain_sqla"] = config.time_grain
+ form_data["granularity_sqla"] = config.temporal_column
+
+ add_currency_format(form_data, config.currency_format)
+ _add_adhoc_filters(form_data, config.filters)
+ return form_data
+
+
+# Sunburst fields whose mapper defaults must not overwrite existing values when
+# a same-type update omitted the corresponding typed field. Required query
roles
+# (hierarchy and metric) are deliberately absent: a full replacement always
+# updates them.
+_SUNBURST_UPDATE_FIELD_KEYS: dict[str, str] = {
+ "secondary_metric": "secondary_metric",
+ "filters": "adhoc_filters",
+ "time_range": "time_range",
+ "time_grain": "time_grain_sqla",
+ "temporal_column": "granularity_sqla",
+ "sort_by_metric": "sort_by_metric",
+ "row_limit": "row_limit",
+ "color_scheme": "color_scheme",
+ "linear_color_scheme": "linear_color_scheme",
+ "show_labels": "show_labels",
+ "show_labels_threshold": "show_labels_threshold",
+ "show_total": "show_total",
+ "show_null_values": "show_null_values",
+ "label_type": "label_type",
+ "number_format": "number_format",
+ "date_format": "date_format",
+ "currency_format": "currency_format",
+}
+
+
+def merge_form_data_for_update(
+ existing_form_data: Dict[str, Any],
+ new_form_data: Dict[str, Any],
+ config: Any,
+) -> Dict[str, Any]:
+ """Merge a same-viz update without resetting omitted Sunburst UI state.
+
+ Native keys that MCP does not model are retained, matching the established
+ preview-update behavior. For a same-type Sunburst update, mapper defaults
+ are replaced by saved values when the caller omitted that field; explicit
+ values (including ``False``, ``0``, ``None`` and ``[]``) win.
+ """
+ merged = {**existing_form_data, **new_form_data}
+ if not isinstance(config, SunburstChartConfig) or existing_form_data.get(
+ "viz_type"
+ ) != new_form_data.get("viz_type"):
+ return merged
+
+ fields_set = config.model_fields_set
+ for field_name, form_key in _SUNBURST_UPDATE_FIELD_KEYS.items():
+ if field_name not in fields_set:
+ if form_key in existing_form_data:
+ merged[form_key] = existing_form_data[form_key]
+ else:
+ merged.pop(form_key, None)
+ continue
+
+ value = getattr(config, field_name)
+ if value is None or (field_name == "filters" and value == []):
+ merged.pop(form_key, None)
Review Comment:
Addressed in the current head. Explicit time-range/column/grain changes
invoke `_scrub_temporal_form_data`, removing temporal state and TEMPORAL_RANGE
filters from top-level and `extra_form_data` reconstruction sources before
merge. `test_explicit_temporal_clear_removes_preserved_temporal_filters`,
`test_each_temporal_clear_scrubs_every_reconstruction_source`, and the native
explicit-null test cover this behavior.
##########
superset/mcp_service/utils/response_utils.py:
##########
@@ -167,53 +171,208 @@ def build(self) -> Dict[str, str]:
STATS_ROW_CAP: int = 5000
+STATS_SAMPLE_VALUE_COUNT: int = 3
+STATS_TOTAL_WORK_CAP: int = 100_000
+
+_GENERIC_DATA_TYPE_NAMES: dict[int, str] = {
+ GenericDataType.NUMERIC: "numeric",
+ GenericDataType.STRING: "string",
+ GenericDataType.TEMPORAL: "temporal",
+ GenericDataType.BOOLEAN: "boolean",
+}
+_MAX_PROFILE_INTEGER_BITS = 4_096
+_MAX_PROFILE_STRING_LENGTH = 65_536
+
+
+@dataclass
+class _ColumnStatsBudget:
+ """One nested-node budget shared by all result columns."""
+
+ nodes: int = 0
+
+
+def data_column_stats_row_limit(row_count: int, column_count: int) -> int:
+ """Return a row sample whose aggregate top-level cell work is bounded."""
+ if row_count <= 0 or column_count <= 0:
+ return 0
+ return min(row_count, STATS_ROW_CAP, STATS_TOTAL_WORK_CAP // column_count)
+
+
+def _profile_value_identity( # noqa: C901
+ value: Any, budget: _ColumnStatsBudget
+) -> tuple[Any, ...] | None:
+ """Build a hook-free identity under the shared iterative node budget."""
+ tokens: list[Any] = []
+ stack: list[tuple[str, Any]] = [("value", value)]
+ active_containers: set[int] = set()
+ while stack:
+ action, item = stack.pop()
+ budget.nodes += 1
+ if budget.nodes > STATS_TOTAL_WORK_CAP:
+ return None
+ if action == "token":
+ tokens.append(item)
+ continue
+ if action == "leave":
+ active_containers.remove(id(item))
+ continue
+ if type(item) is list:
+ identity = id(item)
+ if identity in active_containers:
+ tokens.append(("cyclic_list",))
+ continue
+ active_containers.add(identity)
+ width = list.__len__(item)
+ tokens.append(("list", width))
+ stack.append(("leave", item))
+ stack.append(("token", "list_end"))
+ stack.extend(
+ ("value", list.__getitem__(item, index))
+ for index in range(width - 1, -1, -1)
+ )
+ continue
+ if type(item) is dict:
+ identity = id(item)
+ if identity in active_containers:
+ tokens.append(("cyclic_dict",))
+ continue
+ active_containers.add(identity)
+ entries = list(dict.items(item))
+ tokens.append(("dict", list.__len__(entries)))
+ stack.append(("leave", item))
+ stack.append(("token", "dict_end"))
+ for key, child in reversed(entries):
+ key_token = (
+ ("key", key)
+ if type(key) is str
+ and str.__len__(key) <= _MAX_PROFILE_STRING_LENGTH
+ else ("opaque_key", id(type(key)), id(key))
+ )
+ stack.append(("value", child))
+ stack.append(("token", key_token))
+ continue
+
+ value_type = type(item)
Review Comment:
Addressed and regression-tested at 5306ee42. Booleans use the distinct
`("boolean", value)` identity while integers retain numeric rational
identities; `test_boolean_and_integer_cardinality_remain_distinct` asserts
`[True, 1, False, 0]` has `unique_count == 4`.
--
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]
