fix(mcp): preserve table chart state during updates

superset/mcp_service/chart/compile.py

@@ -242,11 +242,9 @@ def _validate_adhoc_filter_columns(
     represented on the new config — those would otherwise bypass validation
     and surface only when Explore tries to run the query.
     """
-    adhoc_filters = form_data.get("adhoc_filters") or []
+    adhoc_filters = _active_adhoc_filters(form_data.get("adhoc_filters") or [])
     invalid: List[str] = []
     for f in adhoc_filters:
-        if not isinstance(f, dict):
-            continue
         # SIMPLE filters expose the column via "subject"; SQL-expression
         # filters carry a free-form ``sqlExpression`` we can't safely parse,
         # so skip those.
@@ -282,13 +280,35 @@ def _validate_adhoc_filter_columns(
         details=(
             "Adhoc filter columns must exist on the dataset. "
             "If these filters were preserved from a previous chart preview, "
-            "remove them or pass an explicit ``filters`` list on the new config."
+            "pass an explicit 'filters' list on the new config; use "
+            "'filters': [] to clear them."
         ),
         suggestions=suggestions,
         error_code="CHART_VALIDATION_FAILED",
     )
 
 
+def _is_inert_adhoc_filter(filter_: dict[str, Any]) -> bool:
+    """Whether a saved filter is Superset's non-filtering placeholder."""
+    operator = filter_.get("operator", filter_.get("op"))
+    comparator = filter_.get("comparator", filter_.get("val"))
+    return (
+        isinstance(operator, str)
+        and operator.casefold() == "temporal_range"
+        and isinstance(comparator, str)
+        and comparator.casefold() == "no filter"
+    )
+
+
+def _active_adhoc_filters(filters: list[Any]) -> list[dict[str, Any]]:
+    """Return structurally valid filters that can produce a predicate."""
+    return [
+        filter_
+        for filter_ in filters
+        if isinstance(filter_, dict) and not _is_inert_adhoc_filter(filter_)
+    ]
+
+
 def _classify_as_database_error(exc: BaseException, dataset_id: int) -> bool:
     """Use the dataset's DB engine spec to classify the error.
 

superset/mcp_service/chart/schemas.py

@@ -2351,6 +2351,14 @@ class UpdateChartRequest(ChartRequestNormalizerMixin, QueryCacheControl):
         None,
         description="Chart configuration. Optional; omit to only update chart_name.",
     )
+    add_columns: List[ColumnRef] | None = Field(
+        None,
+        description=(
+            "Table columns or metrics to append while preserving every existing "
+            "column and metric. Use this instead of config.columns when adding "
+            "columns to an existing table chart."
+        ),
+    )
     chart_name: str | None = Field(
         None,
         description="Auto-generates if omitted",
@@ -2383,6 +2391,19 @@ class UpdateChartRequest(ChartRequestNormalizerMixin, QueryCacheControl):
         ),
     )
 
+    @model_validator(mode="after")
+    def validate_column_patch(self) -> "UpdateChartRequest":
+        """Keep full-config replacement and additive table updates unambiguous."""
+        if self.config is not None and self.add_columns is not None:
+            raise ValueError(
+                "Use either 'config' for a full visualization replacement or "
+                "'add_columns' to append table columns while preserving the existing "
+                "configuration, not both."
+            )
+        if self.add_columns == []:
+            raise ValueError("'add_columns' must contain at least one column")
+        return self
+
     @field_validator("chart_name")
     @classmethod
     def sanitize_chart_name(cls, v: str | None) -> str | None:

superset/mcp_service/chart/tool/update_chart.py

@@ -43,8 +43,10 @@
 from superset.mcp_service.chart.compile import validate_and_compile
 from superset.mcp_service.chart.schemas import (
     AccessibilityMetadata,
+    ColumnRef,
     GenerateChartResponse,
     PerformanceMetadata,
+    TableChartConfig,
     UpdateChartRequest,
     wrap_sql_adhoc_metrics,
 )
@@ -77,11 +79,14 @@ def _validation_error_response(message: str, details: str) -> GenerateChartRespo
 
 def _missing_config_or_name_error() -> GenerateChartResponse:
     return _validation_error_response(
-        message="Either 'config', 'chart_name', or 'dataset_id' must be provided.",
+        message=(
+            "Either 'config', 'add_columns', 'chart_name', or 'dataset_id' must be "
+            "provided."
+        ),
         details=(
-            "Either 'config', 'chart_name', or 'dataset_id' must be provided. "
-            "Use config for visualization changes, chart_name for renaming, "
-            "dataset_id to rebind the chart to a different dataset."
+            "Use config for full visualization changes, add_columns to append table "
+            "columns without replacing existing columns, chart_name for renaming, "
+            "or dataset_id to rebind the chart to a different dataset."
         ),
     )
 
@@ -95,6 +100,98 @@ def _wrapped_form_data_for_response(
     return payload
 
 
+def _entry_key(entry: Any) -> str:
+    """Stable identity for a form_data column or metric entry.
+
+    Metric lists mix saved-metric names with adhoc metric dicts, so entries
+    are not reliably hashable. Serializing gives every shape a comparable
+    key without dropping the unhashable ones.
+    """
+    return json.dumps(entry, sort_keys=True, default=str)
+
+
+def _extend_without_duplicates(existing: list[Any], additions: Any) -> list[Any]:
+    """Append entries that are not already present, preserving order.
+
+    Column and metric order drives the rendered table layout, so appending
+    keeps first-occurrence position rather than rebuilding from a set.
+    """
+    merged = list(existing)
+    seen = {_entry_key(entry) for entry in merged}
+    for entry in additions:
+        key = _entry_key(entry)
+        if key in seen:
+            continue
+        seen.add(key)
+        merged.append(entry)
+    return merged
+
+
+def _append_table_columns(
+    existing_form_data: dict[str, Any],
+    columns: list[ColumnRef],
+) -> dict[str, Any] | GenerateChartResponse:
+    """Append table columns/metrics without replacing the saved column lists."""
+    if existing_form_data.get("viz_type") not in {"table", "ag-grid-table"}:
+        return _validation_error_response(
+            message="'add_columns' is only supported for table charts.",
+            details=(
+                "Use 'config' to replace the full configuration of a non-table chart."
+            ),
+        )
+
+    merged = dict(existing_form_data)
+    metric_columns = [column for column in columns if column.is_metric]
+    dimension_columns = [column for column in columns if not column.is_metric]
+    query_mode = existing_form_data.get("query_mode")
+    if query_mode == "raw":
+        if metric_columns:
+            return _validation_error_response(
+                message="Cannot add metrics to a table in raw query mode.",
+                details=(
+                    "Raw tables accept only unaggregated columns in 'add_columns'. "
+                    "Use 'config' with query_mode='aggregate' and the complete table "
+                    "configuration to convert the chart before adding metrics."
+                ),
+            )
+        merged["all_columns"] = _extend_without_duplicates(
+            list(existing_form_data.get("all_columns") or []),
+            (column.name for column in columns if column.name is not None),
+        )
+        return merged
+
+    # map_config_to_form_data() infers query_mode from the columns handed to
+    # it, so a dimension-only patch compiles to a raw table whose groupby is
+    # empty. Route each kind by is_metric instead, to keep an aggregate chart
+    # aggregate no matter which mix of columns is appended.
+    metric_patch = (
+        map_config_to_form_data(TableChartConfig(columns=metric_columns))
+        if metric_columns
+        else {}
+    )
+    merged["groupby"] = _extend_without_duplicates(
+        list(existing_form_data.get("groupby") or []),
+        (column.name for column in dimension_columns if column.name is not None),
+    )
+    merged["metrics"] = _extend_without_duplicates(
+        list(existing_form_data.get("metrics") or []),
+        metric_patch.get("metrics") or [],
+    )
+    return merged
+
+
+def _merge_replacement_config(
+    existing_form_data: dict[str, Any],
+    new_form_data: dict[str, Any],
+    parsed_config: Any,
+) -> dict[str, Any]:
+    """Merge a replacement config, honoring an explicit empty filter list."""
+    merged = {**existing_form_data, **new_form_data}
+    if getattr(parsed_config, "filters", None) == []:
+        merged.pop("adhoc_filters", None)
+    return merged
+
+
 def _build_update_payload(
     request: UpdateChartRequest,
     chart: Any,
@@ -136,6 +233,26 @@ def _build_update_payload(
             payload["datasource_type"] = "table"
         return payload
 
+    if request.add_columns is not None:
+        try:
+            existing_form_data = json.loads(chart.params) if chart.params else {}
+        except (ValueError, TypeError):
+            existing_form_data = {}
+        patched = _append_table_columns(existing_form_data, request.add_columns)
+        if isinstance(patched, GenerateChartResponse):
+            return patched
+        chart_name = request.chart_name or chart.slice_name
+        additive_payload: dict[str, Any] = {
+            "slice_name": chart_name,
+            "viz_type": patched["viz_type"],
+            "params": json.dumps(patched),
+            "query_context": None,
+        }
+        if request.dataset_id is not None:
+            additive_payload["datasource_id"] = request.dataset_id
+            additive_payload["datasource_type"] = "table"
+        return additive_payload
+
     # Dataset-only update: rebind chart to a different dataset without changing viz
     if request.dataset_id is not None:
         payload = {
@@ -185,7 +302,16 @@ def _build_preview_form_data(
             parsed_config, dataset_id=effective_dataset_id
         )
         new_form_data.pop("_mcp_warnings", None)
-        merged = {**existing_form_data, **new_form_data}
+        # An explicit filters list, including [], replaces saved filters. An
+        # omitted filters field preserves them through the shallow merge.
+        merged = _merge_replacement_config(
+            existing_form_data, new_form_data, parsed_config
+        )
+    elif request.add_columns is not None:
+        patched = _append_table_columns(existing_form_data, request.add_columns)
+        if isinstance(patched, GenerateChartResponse):
+            return patched
+        merged = patched
     else:
         if not request.chart_name and request.dataset_id is None:
             return _missing_config_or_name_error()
@@ -352,6 +478,7 @@ async def update_chart(  # noqa: C901
     - LLM clients MUST display the returned explore URL to users.
     - Use numeric ID or UUID string to identify the chart (NOT chart name).
     - config is optional — omit it to rename a chart without changing its visualization
+    - To append table columns without restating the existing list, use add_columns
 
     Example usage (preview, default):
     ```json
@@ -383,6 +510,24 @@ async def update_chart(  # noqa: C901
     }
     ```
 
+    Add a table column while preserving existing columns and metrics:
+    ```json
+    {
+        "identifier": 123,
+        "add_columns": [{"name": "go_live_date", "aggregate": "MIN"}]
+    }
+    ```
+
+    add_columns combines with chart_name to append and rename in one call.
+    Columns already on the chart are ignored, so repeating a column is safe:
+    ```json
+    {
+        "identifier": 123,
+        "chart_name": "Go-Live Tracker",
+        "add_columns": [{"name": "region"}]
+    }
+    ```
+
     Example usage with a custom SQL metric (ratios, conditional aggregations,
     unit conversions). Pass 'sql_expression' instead of 'name'+'aggregate'.
     A 'label' is required:
@@ -470,6 +615,9 @@ async def update_chart(  # noqa: C901
 
         # config is already a typed ChartConfig | None (validated by Pydantic)
         parsed_config = request.config
+        validation_config = parsed_config
+        if request.add_columns is not None:
+            validation_config = TableChartConfig(columns=request.add_columns)
 
         # Normalize column case to match dataset canonical names
         # (mirrors generate_chart pipeline layer 4)
@@ -481,16 +629,22 @@ async def update_chart(  # noqa: C901
             if request.dataset_id is not None
             else getattr(chart, "datasource_id", None)
         )
-        if parsed_config is not None and effective_norm_dataset_id is not None:
+        if validation_config is not None and effective_norm_dataset_id is not None:
             from superset.mcp_service.chart.validation.dataset_validator import (
                 DatasetValidator,
                 NORMALIZATION_EXCEPTIONS,
             )
 
             try:
-                parsed_config = DatasetValidator.normalize_column_names(
-                    parsed_config, effective_norm_dataset_id
+                validation_config = DatasetValidator.normalize_column_names(
+                    validation_config, effective_norm_dataset_id
                 )
+                if parsed_config is not None:
+                    parsed_config = validation_config
+                else:
+                    request = request.model_copy(
+                        update={"add_columns": validation_config.columns}
+                    )
             except NORMALIZATION_EXCEPTIONS as e:
                 logger.warning(
                     "Column normalization failed for chart %s: %s", chart.id, e
@@ -511,10 +665,10 @@ async def update_chart(  # noqa: C901
             # SQL errors so we don't commit a chart that can't be queried.
             # Renames (no parsed_config and no dataset_id) skip validation since
             # form_data is untouched and no rebind is requested.
-            if parsed_config is not None and new_form_data is not None:
+            if validation_config is not None and new_form_data is not None:
                 with event_logger.log_context(action="mcp.update_chart.validation"):
                     validation_error = _validate_update_against_dataset(
-                        parsed_config,
+                        validation_config,
                         new_form_data,
                         chart,
                         dataset_id=request.dataset_id,
@@ -549,10 +703,10 @@ async def update_chart(  # noqa: C901
                 return preview_or_error
 
             # Validate before caching the form_data — same rationale as above.
-            if parsed_config is not None:
+            if validation_config is not None:
                 with event_logger.log_context(action="mcp.update_chart.validation"):
                     validation_error = _validate_update_against_dataset(
-                        parsed_config,
+                        validation_config,
                         preview_or_error,
                         chart,
                         dataset_id=request.dataset_id,