sadpandajoe commented on code in PR #43770:
URL: https://github.com/apache/superset/pull/43770#discussion_r4224615351
##########
superset/mcp_service/chart/preview_utils.py:
##########
@@ -1398,6 +2413,112 @@ def fallback_vega_lite_preview(
return None
+def generate_xy_pivot_vega_lite_preview(
+ data: list[dict[str, Any]], form_data: dict[str, Any], *, mark: str
+) -> VegaLitePreview | None:
+ """Render flattened timeseries pivot columns without dropping grouped
series.
+
+ Folding escaped field paths resolves literal output keys without splitting
+ category values that contain escaped commas. The legend retains each
+ complete metric/category label.
+ Long-form results continue through the generic renderer.
+ """
+ from superset.mcp_service.chart.chart_helpers import _as_list
+ from superset.utils.pandas_postprocessing.utils import (
+ escape_separator,
+ FLAT_COLUMN_SEPARATOR,
+ )
+
+ if not data:
+ return None
+ dimensions = [
+ label
+ for column in _as_list(form_data.get("groupby"))
+ if (label := _form_column_label(column))
+ ]
+ if not dimensions or any(label in data[0] for label in dimensions):
+ return None
+ x_axis = _form_column_label(form_data.get("x_axis")) or "__timestamp"
+ if x_axis not in data[0]:
+ return None
+ metric_labels = [
+ label
+ for metric in _as_list(form_data.get("metrics"))
+ if (label := metric_result_label(metric))
+ ]
+ # Chart-data results unescape the flattened column names, while raw
+ # post-processing output keeps escaped separators; match either spelling.
+ prefixes = {
+ spelling
+ for label in metric_labels
+ for spelling in (label, escape_separator(label))
+ }
+ fields = [
+ field
+ for field in data[0]
+ if field != x_axis
+ and any(
+ field.startswith(prefix + FLAT_COLUMN_SEPARATOR)
+ or field.startswith(prefix + "__")
+ for prefix in prefixes
+ )
+ ]
+ if not fields and len(metric_labels) == 1 and
form_data.get("truncate_metric"):
+ # A single truncated metric drops its label from the pivoted column
+ # names, so every non-x-axis column is one category series.
+ fields = [field for field in data[0] if field != x_axis]
+ if not fields:
+ return None
+ sample = data[0][x_axis]
+ x_type = (
+ "temporal"
+ if isinstance(sample, str) and any(char in sample for char in "-/: ")
+ else "quantitative"
+ if isinstance(sample, (int, float))
+ else "nominal"
+ )
+ return VegaLitePreview(
+ specification={
+ "$schema": "https://vega.github.io/schema/vega-lite/v5.json";,
+ "data": {"values": data},
+ "transform": [
+ {
+ "fold": [
+ "".join(
+ "\\" + char if char in ".[]\\" else char for char
in field
+ )
+ for field in fields
+ ],
+ "as": ["__mcp_xy_series", "__mcp_xy_value"],
+ }
+ ],
+ "mark": mark,
+ "encoding": {
+ "x": {"field": x_axis, "type": x_type, "title": x_axis},
Review Comment:
The folded series fields are escaped for `.`, `[`, `]` and `\`, but the x
encoding here (and the x field in the tooltip below) uses the raw `x_axis`
name. For a chart whose x-axis column is `orders.date`, Vega-Lite reads that as
a nested path, finds no value, and the x axis renders empty while the preview
still reports success. Should the x references go through the same escaping?
##########
superset/mcp_service/chart/tool/update_chart_preview.py:
##########
@@ -208,11 +245,27 @@ def update_chart_preview( # noqa: C901
or (previous_form_data or {}).get("datasource_id")
or ""
).split("__", 1)[0]
- plugin = get_registry().get(config.chart_type,
include_disabled=True)
+ # The cached chart keeps its plugin's contract for the whole
+ # update, even when its chart type is disabled for new charts.
+ contract_scope.enter_context(
+ saved_chart_contract((previous_form_data or
{}).get("viz_type"))
+ )
+ plugin = get_registry().get(config.chart_type)
dataset_rebind = previous_datasource != str(dataset.id) and (
bool(previous_datasource)
or bool(plugin and plugin.unbound_form_data_is_rebind)
)
+ if (
+ dataset_rebind
+ and previous_form_data
+ and not (plugin is not None and plugin.strict_dataset_rebind)
+ ):
+ # Match saved-chart rebinds: retain only references that
resolve
+ # against the replacement dataset before resolving omitted
roles.
+ previous_form_data = _prune_inherited_query_state(
Review Comment:
This prune treats the rebind as same-dataset, but
`_inherited_state_invalid_keys` (`update_chart.py` ~378) only checks `groupby`
when it is a list, and Bullet explicitly supports a scalar saved `groupby`
(`bullet_groupby_list` mirrors the frontend `ensureIsArray`). For a saved
Bullet with `groupby: "OldRegion"` rebound to a dataset that has `Revenue` but
no `OldRegion` (`update_chart_preview` with `dataset_id` and only a new
metric), the stale hierarchy survives the prune, gets normalized to
`["OldRegion"]`, and the query fails on a missing column instead of dropping
the incompatible hierarchy as the docs describe. Should the scalar form be
validated like the list form?
##########
superset/mcp_service/chart/preview_utils.py:
##########
@@ -1398,6 +2413,112 @@ def fallback_vega_lite_preview(
return None
+def generate_xy_pivot_vega_lite_preview(
+ data: list[dict[str, Any]], form_data: dict[str, Any], *, mark: str
+) -> VegaLitePreview | None:
+ """Render flattened timeseries pivot columns without dropping grouped
series.
+
+ Folding escaped field paths resolves literal output keys without splitting
+ category values that contain escaped commas. The legend retains each
+ complete metric/category label.
+ Long-form results continue through the generic renderer.
+ """
+ from superset.mcp_service.chart.chart_helpers import _as_list
+ from superset.utils.pandas_postprocessing.utils import (
+ escape_separator,
+ FLAT_COLUMN_SEPARATOR,
+ )
+
+ if not data:
+ return None
+ dimensions = [
+ label
+ for column in _as_list(form_data.get("groupby"))
+ if (label := _form_column_label(column))
+ ]
+ if not dimensions or any(label in data[0] for label in dimensions):
+ return None
+ x_axis = _form_column_label(form_data.get("x_axis")) or "__timestamp"
+ if x_axis not in data[0]:
+ return None
+ metric_labels = [
+ label
+ for metric in _as_list(form_data.get("metrics"))
+ if (label := metric_result_label(metric))
+ ]
+ # Chart-data results unescape the flattened column names, while raw
+ # post-processing output keeps escaped separators; match either spelling.
+ prefixes = {
+ spelling
+ for label in metric_labels
+ for spelling in (label, escape_separator(label))
+ }
+ fields = [
Review Comment:
A saved grouped line chart with one metric and a time comparison (say
`metrics: ["revenue"]`, `groupby: ["region"]`, `time_compare: ["1 year ago"]`,
`comparison_type: "values"`) comes back from the new rename step with keys like
`revenue, East` and `1 year ago, East` (`chart_helpers.py` ~1372 renames the
shifted series to the bare offset when there is one metric). This prefix filter
only keeps fields that start with `revenue`, so the prior-year series is folded
out silently and the preview still reports success. The `truncate_metric`
fallback below only runs when no field matched, so it doesn't cover this mixed
shape either. Should field discovery also accept the offset-named series (or
fold every non-x-axis column when the shapes are mixed)?
--
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]
