aminghadersohi commented on code in PR #43770:
URL: https://github.com/apache/superset/pull/43770#discussion_r4156493234
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1715,6 +1781,668 @@ def map_histogram_config(config:
"HistogramChartConfig") -> Dict[str, Any]:
return form_data
+def _bullet_token_list(values: Sequence[str | int | float]) -> str:
+ """Serialize typed Bullet controls to the frontend's comma-separated
form."""
+ tokens: list[str] = []
+ for value in values:
+ if isinstance(value, float):
+ token = repr(value)
+ # ``100`` parses back to the same binary float as ``100.0`` and
+ # preserves the frontend's established compact integer spelling.
+ if token.endswith(".0") and not (
+ value == 0.0 and math.copysign(1.0, value) < 0
+ ):
+ token = token[:-2]
+ tokens.append(token)
+ else:
+ tokens.append(str(value))
+ return ",".join(tokens)
+
+
+def map_bullet_config(config: BulletChartConfig) -> Dict[str, Any]: # noqa:
C901
+ """Map typed Bullet config to ``Bullet/buildQuery`` and transformProps.
+
+ The frontend buildQuery replaces the generic query fields with exactly one
+ metric and the groupby hierarchy. Presentation controls stay in native
+ snake_case form_data; the chart plugin camelizes them for transformProps.
+ """
+ if config.dimensions is None and config.order_by:
+ # An update resolves its saved hierarchy before mapping. Without one,
+ # creation must validate sort targets against an empty hierarchy.
+ BulletChartConfig.model_validate(
+ {**config.model_dump(exclude_unset=True), "dimensions": []}
+ )
+ metric = create_metric_object(config.metric)
+ form_data: Dict[str, Any] = {
+ "viz_type": "bullet",
+ "metric": metric,
+ }
+
+ # Optional semantic/query fields are emitted only when explicitly supplied.
+ # This lets update_chart and update_chart_preview preserve native saved
state,
+ # while an explicit empty value still clears it through the generic merge
path.
+ if "dimensions" in config.model_fields_set:
+ form_data["groupby"] = [dimension.name for dimension in
config.dimensions or []]
+ if "row_limit" in config.model_fields_set:
+ form_data["row_limit"] = config.row_limit
+ if "time_range" in config.model_fields_set:
+ form_data["time_range"] = config.time_range
+
+ if config.order_by:
+ dimensions = config.dimensions or []
+ orderby: list[list[Any]] = []
+ for order in config.order_by:
+ role, index = resolve_bullet_order_target(
+ order.column, dimensions, config.metric
+ )
+ if role == "metric":
+ order_target: Any = metric
+ else:
+ if index is None: # Defensive: resolver pairs dimensions with
indexes.
+ raise ValueError("Bullet dimension order target has no
index")
+ order_target = dimensions[index].name
+ orderby.append([order_target, order.ascending])
+ form_data["orderby"] = orderby
+ elif "order_by" in config.model_fields_set:
+ form_data["orderby"] = []
+
+ presentation_fields: dict[str, tuple[str, Any]] = {
+ "ranges": ("ranges", _bullet_token_list(config.ranges)),
+ "range_labels": (
+ "range_labels",
+ _bullet_token_list(config.range_labels),
+ ),
+ "markers": ("markers", _bullet_token_list(config.markers)),
+ "marker_labels": (
+ "marker_labels",
+ _bullet_token_list(config.marker_labels),
+ ),
+ "marker_lines": (
+ "marker_lines",
+ _bullet_token_list(config.marker_lines),
+ ),
+ "marker_line_labels": (
+ "marker_line_labels",
+ _bullet_token_list(config.marker_line_labels),
+ ),
+ "y_axis_format": ("y_axis_format", config.y_axis_format),
+ "show_labels": ("show_labels", config.show_labels),
+ "show_legend": ("show_legend", config.show_legend),
+ }
+ for field_name, (form_key, value) in presentation_fields.items():
+ if field_name in config.model_fields_set:
+ form_data[form_key] = value
+
+ _add_adhoc_filters(form_data, config.filters)
+ if config.filters == [] and "filters" in config.model_fields_set:
+ form_data["adhoc_filters"] = []
+ if config.time_range and config.temporal_column:
+ _ensure_temporal_adhoc_filter(form_data, config.temporal_column)
+ for filter_ in form_data.get("adhoc_filters", []):
+ if (
+ isinstance(filter_, dict)
+ and filter_.get("operator") ==
FilterOperator.TEMPORAL_RANGE.value
+ and filter_.get("subject") == config.temporal_column
+ and filter_.get("comparator") == NO_TIME_RANGE
+ ):
+ filter_["comparator"] = config.time_range
+ return form_data
+
+
+def merge_bullet_form_data(
+ existing_form_data: Mapping[str, Any], new_form_data: Dict[str, Any]
+) -> None:
+ """Preserve omitted native Bullet controls across update tool paths.
+
+ Query roles and every UI control have an explicit typed representation.
+ Mappers emit optional fields only when the caller supplied them, so copying
+ the bounded native keys below preserves omitted state while explicit empty,
+ false, null, and zero-like values remain authoritative.
+ """
+ if (
+ existing_form_data.get("viz_type") != "bullet"
+ or new_form_data.get("viz_type") != "bullet"
+ ):
+ return
+ preserved_keys = {
+ "groupby",
+ "adhoc_filters",
+ "time_range",
+ "row_limit",
+ "orderby",
+ "ranges",
+ "range_labels",
+ "markers",
+ "marker_labels",
+ "marker_lines",
+ "marker_line_labels",
+ "y_axis_format",
+ "show_labels",
+ "show_legend",
+ MCP_DASHBOARD_TIME_FILTER_SUBJECT,
+ }
+
+ # Threshold and label arrays are one frontend control pair. If callers
+ # replace the values without replacing their labels, clear the stale labels
+ # instead of accidentally reassigning them by position.
+ dependent_controls = {
+ "ranges": "range_labels",
+ "markers": "marker_labels",
+ "marker_lines": "marker_line_labels",
+ }
+ for values_key, labels_key in dependent_controls.items():
+ if values_key in new_form_data and labels_key not in new_form_data:
+ new_form_data[labels_key] = ""
+
+ for key in preserved_keys:
+ if (
+ key == MCP_DASHBOARD_TIME_FILTER_SUBJECT
+ and "adhoc_filters" in new_form_data
+ ):
+ # The marker describes a mapper-generated temporal filter. Do not
+ # retain stale provenance when an explicit filter update removed
it.
+ continue
+ if key in existing_form_data and key not in new_form_data:
+ new_form_data[key] = existing_form_data[key]
+
+
+def _filter_identity(filter_: Any) -> tuple[Any, ...] | None:
+ """Return the native identity used when one filter replaces another."""
+ if not isinstance(filter_, Mapping):
+ return None
+ return (
+ filter_.get("clause"),
+ filter_.get("expressionType"),
+ filter_.get("subject"),
+ filter_.get("operator"),
+ )
+
+
+def _temporal_binding_filter(filters: list[Any], subject: Any) -> dict[str,
Any] | None:
+ """Find the unique filter owned by a recorded MCP temporal marker."""
+ if subject is None:
+ return None
+ if not isinstance(subject, str) or not subject:
+ raise ValueError(
+ "MCP temporal binding provenance subject must be a non-empty
string"
+ )
+ matches = [
+ filter_
+ for filter_ in filters
+ if isinstance(filter_, dict)
+ and filter_.get("subject") == subject
+ and filter_.get("operator") == FilterOperator.TEMPORAL_RANGE.value
+ ]
+ if len(matches) != 1:
+ raise ValueError(
+ "MCP temporal binding provenance must match exactly one "
+ f"TEMPORAL_RANGE filter for subject {subject!r}; found
{len(matches)}"
+ )
+ return matches[0]
+
+
+def _append_or_replace_filter(filters: list[Any], filter_: Any) -> None:
+ """Append a filter, replacing the same native role when identifiable."""
+ identity = _filter_identity(filter_)
+ if identity is None:
+ if filter_ not in filters:
+ filters.append(filter_)
+ return
+ filters[:] = [item for item in filters if _filter_identity(item) !=
identity]
+ filters.append(filter_)
+
+
+_NATIVE_TEMPORAL_ROLE_FIELDS: dict[str, frozenset[str]] = {
+ # Typed ``x`` is persisted as native x_axis/granularity_sqla for XY and
+ # Mixed Timeseries. Waterfall exposes the typed field as ``x_axis``.
+ "x_axis": frozenset({"x", "x_axis"}),
+ "granularity_sqla": frozenset({"x", "x_axis", "temporal_column"}),
+ # Chart plugins may designate a chart-specific query role as the implicit
+ # dashboard-time subject.
+ "start_time": frozenset({"start_time"}),
+}
+
+
+def _native_temporal_subject_changed(
+ existing_form_data: Mapping[str, Any],
+ new_form_data: Mapping[str, Any],
+ explicit_fields: set[str],
+) -> bool:
+ """Return whether an authoritative native temporal role was replaced.
+
+ Mapping a partial update can propose a dataset fallback binding even when
+ the caller only changed filters. That proposal is not authoritative. A
+ changed x/granularity/chart-specific role is authoritative only when its
+ corresponding typed field was actually supplied.
+ """
+ for native_key, typed_fields in _NATIVE_TEMPORAL_ROLE_FIELDS.items():
+ if explicit_fields.isdisjoint(typed_fields):
+ continue
+ existing_value = existing_form_data.get(native_key)
+ incoming_value = new_form_data.get(native_key)
+ if existing_value != incoming_value:
+ return True
+ return False
+
+
+def _native_temporal_binding(
+ form_data: Mapping[str, Any], filters: list[Any]
+) -> tuple[str | None, dict[str, Any] | None]:
+ """Resolve one binding for a trusted native temporal role, if present."""
+ for native_key in _NATIVE_TEMPORAL_ROLE_FIELDS:
+ subject = form_data.get(native_key)
+ if not isinstance(subject, str) or not subject:
+ continue
+ matches = [
+ filter_
+ for filter_ in filters
+ if isinstance(filter_, dict)
+ and filter_.get("subject") == subject
+ and filter_.get("operator") == FilterOperator.TEMPORAL_RANGE.value
+ ]
+ if len(matches) > 1:
+ raise ValueError(
+ "An authoritative native temporal subject must match at most
one "
+ f"TEMPORAL_RANGE filter for subject {subject!r}; found "
+ f"{len(matches)}"
+ )
+ if matches:
+ return subject, matches[0]
+ return None, None
+
+
+def merge_update_form_data( # noqa: C901
+ existing_form_data: Mapping[str, Any],
+ new_form_data: Dict[str, Any],
+ config: ChartConfig,
+) -> None:
+ """Apply the shared omission/provenance contract for chart updates.
+
+ Mapper-generated neutral temporal bindings are infrastructure, not evidence
+ that the caller supplied ``filters`` or changed a saved time-range binding.
+ This helper is used by immediate saves, preview-first saved updates, and
+ cached-preview updates so omission, clear, replacement, and temporal
+ overrides have identical behavior.
+ """
+ existing_filters = list(existing_form_data.get("adhoc_filters") or [])
+ incoming_filters = list(new_form_data.get("adhoc_filters") or [])
+ existing_subject =
existing_form_data.get(MCP_DASHBOARD_TIME_FILTER_SUBJECT)
+ incoming_subject = new_form_data.get(MCP_DASHBOARD_TIME_FILTER_SUBJECT)
+ existing_binding = _temporal_binding_filter(existing_filters,
existing_subject)
Review Comment:
Fixed in 475698ba5a831ad8af0d6d8f427449afe8abe81e by discarding temporal
provenance with incompatible inherited filters. Table and Bullet regressions
cover replacement datasets with and without a different time column.
##########
superset/mcp_service/chart/tool/update_chart_preview.py:
##########
@@ -243,62 +294,75 @@ 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_chart_form_data(
- previous_form_data,
- new_form_data,
- config,
- dataset_rebind=dataset_rebind,
- )
+ merge_plugin =
plugin_for_viz_type(new_form_data.get("viz_type"))
+ if merge_plugin is not None and merge_plugin.owns_update_merge:
+ # The plugin owns its temporal/rebind merge contract.
+ # Generic merges would restore deliberately removed state.
+ new_form_data = merge_chart_form_data(
+ previous_form_data,
+ new_form_data,
+ config,
+ dataset_rebind=dataset_rebind,
+ )
+ else:
+ new_form_data = merge_chart_form_data(
+ previous_form_data,
+ new_form_data,
+ config,
+ dataset_rebind=dataset_rebind,
+ )
+ merge_update_form_data(previous_form_data, new_form_data,
config)
+ merge_same_viz_form_data(previous_form_data,
new_form_data, config)
Review Comment:
Fixed in 475698ba5a831ad8af0d6d8f427449afe8abe81e by pruning incompatible
cached query roles before the preservation pass and binding the returned
datasource to the replacement dataset. Regression coverage checks grouping,
sorting, filters, and stale datasource identifiers.
--
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]
