sadpandajoe commented on code in PR #43771:
URL: https://github.com/apache/superset/pull/43771#discussion_r4206380247
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -2158,6 +3048,56 @@ def map_mixed_timeseries_config(
_add_adhoc_filters(form_data, config.filters)
+ fields_set = config.model_fields_set
+ if "adhoc_filters_b" in fields_set:
+ form_data["adhoc_filters_b"] = config.adhoc_filters_b
+ elif "filters_secondary" in fields_set:
+ form_data["adhoc_filters_b"] = [
Review Comment:
`filters_secondary` is mapped straight into `adhoc_filters_b` here, but
`MixedTimeseriesPlugin.extract_column_refs` only collects the columns from the
primary `filters`. A misspelled column such as `{"column": "regoin", "op": "=",
"value": "EMEA"}` in `filters_secondary` is never checked against the dataset,
so the chart compiles and Query B silently runs unfiltered across every region.
Should the secondary filter columns go through the same dataset validation and
normalization as `filters`?
##########
superset/mcp_service/chart/chart_helpers.py:
##########
@@ -450,12 +582,200 @@ def _resolve_deck_gl_metrics(
if value:
metrics.append(value)
elif isinstance(prf, str) and _is_metric_ref(prf):
- # Legacy deck_scatter: point_radius_fixed as a bare non-numeric metric
key
- logger.debug("Legacy point_radius_fixed string metric encountered:
%s", prf)
metrics.append(prf)
return metrics
+def _deck_query_adapter( # noqa: C901
+ form_data: dict[str, Any], query: dict[str, Any], viz_type: str
+) -> dict[str, Any]:
+ """Apply the native frontend builder for a single Deck.gl layer."""
+ base_columns = list(query.get("columns") or [])
+ base_metrics = list(query.get("metrics") or [])
+ filters = list(query.get("filters") or [])
+ tooltips = _deck_tooltip_columns(form_data.get("tooltip_contents"))
+
+ def add_null(column: str, *, value: Any = ...) -> None:
+ clause: dict[str, Any] = {"col": column, "op": "IS NOT NULL"}
+ if value is not ...:
+ clause["val"] = value
+ filters.append(clause)
+
+ if viz_type == "deck_geojson":
+ geometry = form_data.get("geojson")
+ if not isinstance(geometry, str) or not geometry:
+ raise ValueError("GeoJSON column is required for GeoJSON charts")
+ columns = _add_deck_columns(base_columns, [geometry] if geometry else
[])
+ cross_filter = form_data.get("cross_filter_column")
+ if cross_filter:
+ columns = _add_deck_columns(columns, [cross_filter])
+ columns = _add_deck_columns(columns, tooltips)
+ if form_data.get("filter_nulls", True) and isinstance(geometry, str):
+ add_null(geometry)
+ query.update(
+ columns=columns,
+ metrics=[],
+ groupby=[],
+ filters=filters,
+ is_timeseries=False,
+ )
+ return query
+
+ if viz_type == "deck_polygon":
+ line_column = form_data.get("line_column")
+ if not isinstance(line_column, str) or not line_column:
+ raise ValueError("Polygon column is required for Polygon charts")
+ columns = _add_deck_columns(base_columns, [line_column] if line_column
else [])
+ cross_filter = form_data.get("cross_filter_column")
+ if cross_filter:
+ columns = _add_deck_columns(columns, [cross_filter])
+ columns = _add_deck_columns(columns, tooltips)
+ metrics: list[Any] = []
+ if metric := form_data.get("metric"):
+ metrics.append(metric)
+ radius = form_data.get("point_radius_fixed")
+ if (
+ isinstance(radius, dict)
+ and radius.get("type") == "metric"
+ and radius.get("value") is not None
+ ):
+ metrics.append(radius["value"])
+ if form_data.get("filter_nulls", True) and isinstance(line_column,
str):
+ add_null(line_column)
+ if metric:
+ add_null(_deck_metric_label(metric))
+ query.update(
+ columns=columns,
+ metrics=metrics,
+ filters=filters,
+ is_timeseries=False,
+ )
+ return query
+
+ if viz_type == "deck_path":
+ line_column = form_data.get("line_column")
+ if not isinstance(line_column, str) or not line_column:
+ raise ValueError("Line column is required for Path charts")
+ columns = list(base_columns)
+ metrics = [metric for metric in base_metrics if
_is_deck_metric_value(metric)]
+ groupby = list(query.get("groupby") or [])
+ metric = form_data.get("metric")
+ if metrics or metric:
+ if metric and metric not in metrics:
+ metrics.append(metric)
+ if line_column and line_column not in groupby:
+ groupby.append(line_column)
+ elif line_column:
+ columns = _add_deck_columns(columns, [line_column])
+ if dimension := form_data.get("dimension"):
+ columns = _add_deck_columns(columns, [dimension])
+
+ line_width = form_data.get("line_width")
+ raw_width = (
+ line_width
+ if isinstance(line_width, str)
+ else line_width.get("value")
+ if isinstance(line_width, dict)
+ else None
+ )
+ width_metric = (
+ raw_width
+ if _is_deck_metric_value(line_width)
+ and raw_width is not None
+ and not isinstance(raw_width, (int, float))
+ else None
+ )
+ for extra_metric in (width_metric, form_data.get("breakpoint_metric")):
+ if extra_metric is None:
+ continue
+ labels = {_deck_metric_label(item) for item in metrics}
+ if _deck_metric_label(extra_metric) not in labels:
+ metrics.append(extra_metric)
+ if line_column and line_column not in groupby:
+ groupby.append(line_column)
+ columns = _add_deck_columns(columns, tooltips)
+ groupby = _add_deck_columns(groupby, tooltips)
+ if not any(
+ filter_.get("col") == line_column and filter_.get("op") == "IS NOT
NULL"
+ for filter_ in filters
+ if isinstance(filter_, dict)
+ ):
+ add_null(line_column)
+ query.update(
+ columns=columns,
+ metrics=metrics,
+ groupby=groupby,
Review Comment:
For a saved `deck_path` chart without a stored `query_context`,
`line_column` is placed in `groupby` while `columns` is also set.
`QueryObjectFactory.create()` only applies a deprecated `groupby` through
`kwargs.setdefault("columns", ...)`, so the existing `columns` wins and the
path column is dropped. With `line_column="path"` and `metric="count"`,
`get_chart_data` and `get_chart_sql` would return one dataset-wide aggregate
instead of per-path rows. Could the line column go into canonical `columns`
here (as the non-metric branch already does)?
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2627,6 +3810,37 @@ class XYChartConfig(BaseChartConfig):
le=10000,
)
+ series_limit_metric: ColumnRef | None = Field(
+ None,
+ description="Metric used to rank limited series; null clears it",
+ )
+ timeseries_limit_metric: ColumnRef | None = Field(
+ None,
+ description="Native timeseries ranking metric; null clears it",
+ )
+
+ @field_validator("series_limit_metric", "timeseries_limit_metric",
mode="before")
+ @classmethod
+ def coerce_series_ranking_metric(cls, value: Any) -> Any:
+ """Accept the same native metric references as the Y-axis."""
+ if isinstance(value, dict) and "expressionType" in value:
Review Comment:
This before-validator only coerces dict values that carry `expressionType`,
so a saved-metric string passes through unchanged and then fails `ColumnRef`
validation. For example, `{"viz_type": "echarts_timeseries_line", "x_axis":
"ds", "metrics": ["count"], "timeseries_limit_metric": "count"}` normalizes
`metrics` but is rejected on `timeseries_limit_metric`, even though
`create_metric_object` itself emits the plain string for a saved ranking
metric. Could string values be coerced the same way `metrics` entries are (e.g.
via `_coerce_native_metric`)?
##########
docs/docs/using-superset/using-ai-with-superset.mdx:
##########
@@ -461,6 +472,24 @@ also have `data: null`. For each statement, `executed_sql`
is `null` when it equ
- Restart your AI client if you recently changed the configuration
+### Sunburst chart data
+
+MCP chart tools support ordered Sunburst hierarchies with a primary metric and
+an optional secondary metric. When supplied, the secondary-to-primary ratio
+drives a sequential color scale (for example, profit divided by revenue shows
+margin); when omitted, arc colors are categorical. Use `chart_type: "sunburst"`
+in typed requests; the native chart uses `viz_type: "sunburst_v2"`.
+
+SQL NULL aggregate values are rendered as zero, matching the Sunburst frontend,
+including during compile checks, previews, and data exports. Missing result
+columns, numeric strings, booleans, and infinite metric values remain invalid.
Review Comment:
The claim that infinite metric values "remain invalid" doesn't match the
implementation: non-finite floats are normalized to `null` and then converted
to zero, so compile checks pass and previews and exports show `0`
(`test_compile_accepts_nullable_and_finite_metric_boundaries` covers this). An
analyst relying on the documented rejection could read an overflowing aggregate
as a genuine zero. Should this sentence describe the normalization instead?
##########
tests/unit_tests/mcp_service/chart/test_preview_utils.py:
##########
@@ -154,11 +172,41 @@ def
test_build_query_columns_empty_columns_key_keeps_groupby():
) == ["country"]
+def test_generate_preview_seeds_form_data_before_query_execution():
+ """Preview execution seeds the form data consumed by virtual-dataset
Jinja."""
+ with (
+ patch(
+ "superset.charts.data.form_data.set_query_context_form_data"
+ ) as mock_set_form_data,
+ patch(
+ "superset.commands.chart.data.get_data_command.ChartDataCommand"
+ ) as mock_cmd_cls,
+ patch(
+ "superset.common.query_context_factory.QueryContextFactory"
+ ) as mock_factory,
+ patch("superset.extensions.db") as mock_db,
+ ):
+ mock_db.session.get.return_value = MagicMock(id=12)
+ query_context = MagicMock()
+ mock_factory.return_value.create.return_value = query_context
+ mock_cmd_cls.return_value.run.return_value = chart_data_command_result(
+ rows=[], columns=["value"]
+ )
+
+ preview_utils.generate_preview_from_form_data(
+ form_data={"metrics": [{"label": "count"}]},
+ dataset_id=12,
+ preview_format="table",
+ )
+
+ mock_set_form_data.assert_called_once_with(query_context, 12, "table")
Review Comment:
This regression test only asserts that the mocked seeder was called, so it
would still pass if seeding moved after `command.run()`. An unsaved
virtual-dataset preview using `filter_values("region")` or
`url_param("tenant")` would then run with missing or stale inputs and the test
would stay green. Could the test use the real seeding and assert those macro
values are visible when the command is built, plus a successful preview result?
The other macro tests cover the helper and saved previews, not this unsaved
sequence.
--
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]
