rusackas commented on code in PR #46048:
URL: https://github.com/apache/superset/pull/46048#discussion_r4489074705
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -920,6 +925,229 @@ class DeleteDatasetMetricResponse(BaseModel):
error: str | None = Field(None, description="Error message, or null on
success")
+UPDATABLE_COLUMN_FIELDS: frozenset[str] = frozenset(
+ {
+ "column_name",
+ "expression",
+ "verbose_name",
+ "description",
+ "type",
+ "is_dttm",
+ "groupby",
+ "filterable",
+ "python_date_format",
+ }
+)
+
+# Properties tied to a physical column's identity in the source table. Changing
+# them on a physical column breaks it rather than editing it.
+PHYSICAL_COLUMN_LOCKED_FIELDS: frozenset[str] = frozenset({"column_name",
"expression"})
+
+
+class DatasetColumnProperties(OmittedMeansUnchanged):
+ """Dataset identifier and writable column properties."""
+
+ model_config = ConfigDict(populate_by_name=True)
+
+ dataset_id: int | str = Field(
+ ...,
+ description="Dataset identifier — numeric ID or UUID string. "
+ "Use list_datasets to find valid IDs.",
+ )
+ column_name: str | None = Field(
+ None,
+ max_length=255,
+ description="New column name, unique within the dataset. Only pass
this "
+ "to rename a calculated column; physical columns cannot be renamed.",
+ )
+ expression: str | None = Field(
+ None,
+ description="SQL expression evaluated per row, e.g. "
+ "\"CASE WHEN amount > 100 THEN 'large' ELSE 'small' END\". "
+ "Only calculated columns have an expression.",
+ )
+ verbose_name: str | None = Field(
+ None, max_length=1024, description="Human-friendly display label."
+ )
+ description: str | None = Field(None, description="Column description.")
+ type: str | None = Field(
+ None,
+ description="SQL type of the expression's result (e.g. 'VARCHAR', "
+ "'DOUBLE', 'TIMESTAMP'). Helps Superset pick filters and formatting.",
+ )
+ is_dttm: bool | None = Field(
+ None, description="Whether the column is temporal (usable as a time
axis)."
+ )
+ groupby: bool | None = Field(
+ None, description="Whether the column can be used as a dimension."
+ )
+ filterable: bool | None = Field(
+ None, description="Whether the column can be filtered on."
+ )
+ python_date_format: str | None = Field(
+ None,
+ min_length=1,
+ max_length=255,
+ description="Python strftime format of a temporal string column "
+ "(e.g. '%Y-%m-%d'), or 'epoch_s' / 'epoch_ms' for epoch values.",
+ )
+
+ @field_validator("column_name", mode="before")
+ @classmethod
+ def reject_blank_column_name(cls, value: object) -> object:
+ """Reject blank names before the length cap without trimming valid
names."""
+ if isinstance(value, (bytes, bytearray)):
+ try:
+ value = value.decode()
+ except UnicodeDecodeError:
+ return value # Let Pydantic report its string_unicode error.
+ if isinstance(value, str) and not value.strip():
+ raise PydanticCustomError("empty_text", "column_name cannot be
empty")
+ return value
+
+ @field_validator("python_date_format")
+ @classmethod
+ def python_date_format_must_be_valid(cls, value: str | None) -> str | None:
+ from superset.daos.dataset import DatasetDAO
+
+ if value is not None and not
DatasetDAO.validate_python_date_format(value):
+ raise ValueError(
+ "Invalid python_date_format: must be a strftime format that "
+ "produces an ISO 8601 timestamp, or 'epoch_s' / 'epoch_ms'"
+ )
+ return value
+
+ def updates(self) -> Dict[str, Any]:
+ """Return only the column properties explicitly provided by the caller.
+
+ ``exclude_unset`` distinguishes "not provided" (leave the stored value
+ alone) from an explicit ``null`` (clear the stored value).
+ """
+ return self.model_dump(
+ exclude_unset=True,
+ include=set(UPDATABLE_COLUMN_FIELDS),
+ )
+
+ @model_validator(mode="after")
+ def validate_updates(self) -> "DatasetColumnProperties":
+ """Require at least one updatable property and reject empty values."""
+ provided = self.model_fields_set & UPDATABLE_COLUMN_FIELDS
+ if not provided:
+ raise ValueError(
+ "At least one column property must be provided to update. "
+ f"Updatable properties: {sorted(UPDATABLE_COLUMN_FIELDS)}."
+ )
+ if "column_name" in provided and self.column_name is None:
+ with field_validation_error("column_name"):
+ raise PydanticCustomError(
+ "empty_text", "column_name cannot be empty or null"
+ )
+ if "expression" in provided and not (self.expression or "").strip():
+ raise ValueError(
+ "expression cannot be empty or null; use delete_dataset_column
"
+ "to remove a calculated column"
+ )
+ return self
+
+
+class UpdateDatasetColumnRequest(DatasetColumnProperties):
+ """Request schema for update_dataset_column."""
+
+ column: int | str = Field(
+ ...,
+ description="Column to update — numeric column ID, column UUID, or "
+ "column_name (e.g. 'order_size'). Numeric strings are treated as IDs. "
+ "Use get_dataset_info to discover a dataset's columns.",
+ )
+
+
+class CreateDatasetColumnRequest(DatasetColumnProperties):
+ """Request schema for create_dataset_column."""
+
+ column_name: str = Field(
+ ..., max_length=255, description="Column name, unique within the
dataset."
+ )
+ expression: str = Field(
+ ...,
+ description="SQL expression evaluated per row, e.g. "
+ "\"CASE WHEN amount > 100 THEN 'large' ELSE 'small' END\".",
+ )
+
+
+class DeleteDatasetColumnRequest(BaseModel):
+ """Request schema for delete_dataset_column."""
+
+ dataset_id: int | str = Field(
+ ..., description="Dataset identifier — numeric ID or UUID string."
+ )
+ column: int | str = Field(
+ ...,
+ description="Calculated column ID, UUID, or column_name. Numeric
strings "
+ "are IDs.",
+ )
Review Comment:
Good catch. Added a before-validator that rejects booleans for `dataset_id`
and `column` (update and delete requests), with a test.
##########
superset/mcp_service/dataset/tool/update_dataset_column.py:
##########
@@ -0,0 +1,294 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Update a column on a dataset (FastMCP tool)."""
+
+import difflib
+from typing import Any
+
+from fastmcp import Context
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset import security_manager
+from superset.extensions import event_logger
+from superset.mcp_service.dataset.schemas import (
+ _calculated_expression,
+ DatasetColumnDetail,
+ PHYSICAL_COLUMN_LOCKED_FIELDS,
+ UpdateDatasetColumnRequest,
+ UpdateDatasetColumnResponse,
+)
+
+
+def _is_calculated(column: Any) -> bool:
+ """A calculated column is one defined by a non-blank SQL expression."""
+ return _calculated_expression(column) is not None
+
+
+def _find_column(columns: list[Any], identifier: int | str) -> Any | None:
+ """Find a dataset column by ID, UUID, or column_name."""
+ if isinstance(identifier, str):
+ try:
+ identifier = int(identifier)
+ except ValueError:
+ pass
+
+ if isinstance(identifier, int):
+ return next((c for c in columns if c.id == identifier), None)
Review Comment:
Fixed together with the delete request: `column` now rejects booleans before
union coercion.
--
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]
