aminghadersohi commented on code in PR #39922:
URL: https://github.com/apache/superset/pull/39922#discussion_r3493565830
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1665,24 +1707,28 @@ def reject_sql_expression_on_dimensions(self) ->
"XYChartConfig":
@model_validator(mode="after")
def validate_unique_column_labels(self) -> "XYChartConfig":
"""Ensure all column labels are unique across x, y, and group_by."""
- labels_seen: dict[str, str] = {}
+ # Key is (saved_metric, label) so a saved metric and a regular column
+ # with the same input name are not flagged as duplicates — saved
metrics
+ # resolve to their actual casing from the dataset during normalization.
+ labels_seen: dict[tuple[bool, str], str] = {}
duplicates: list[str] = []
# Add x-axis label if present (x may be None, resolved later).
# The dimension validator rejects sql_expression on x, so name is set.
if self.x is not None:
x_label = self.x.label or self.x.name or ""
- labels_seen[x_label] = "x"
+ labels_seen[(self.x.saved_metric, x_label)] = "x"
# Check Y-axis labels
for i, col in enumerate(self.y):
label = _metric_display_label(col)
- if label in labels_seen:
+ key = (col.saved_metric, label)
+ if key in labels_seen:
duplicates.append(
- f"y[{i}]: '{label}' (conflicts with {labels_seen[label]})"
+ f"y[{i}]: '{label}' (conflicts with {labels_seen[key]})"
)
else:
- labels_seen[label] = f"y[{i}]"
+ labels_seen[key] = f"y[{i}]"
Review Comment:
Same rationale as the `validate_unique_column_labels` note I left on the
table validator: the `(saved_metric, label)` key is intentional. A saved metric
named `Revenue` and an ad-hoc `SUM(revenue)` that the user also labels
`Revenue` are different backend objects that may produce different canonical
names after normalization — saved metrics get their casing from the dataset via
`_get_canonical_metric_name`. Keying by bare `label` would falsely reject valid
payloads where both sides share the same string but diverge post-normalization.
If two entries genuinely produce the same display label after normalization,
the chart query layer surfaces a duplicate-label error at query time.
##########
superset/mcp_service/chart/plugins/table.py:
##########
@@ -0,0 +1,135 @@
+# 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.
+
+"""Table chart type plugin."""
+
+from __future__ import annotations
+
+from typing import Any
+
+from superset.mcp_service.chart.chart_utils import (
+ _summarize_filters,
+ _table_chart_what,
+ map_table_config,
+)
+from superset.mcp_service.chart.plugin import BaseChartPlugin
+from superset.mcp_service.chart.schemas import ColumnRef, TableChartConfig
+from superset.mcp_service.chart.validation.dataset_validator import
DatasetValidator
+from superset.mcp_service.common.error_schemas import ChartGenerationError
+
+
+class TableChartPlugin(BaseChartPlugin):
+ """Plugin for table chart type."""
+
+ chart_type = "table"
+ display_name = "Table"
+ native_viz_types = {
+ "table": "Table",
+ "ag-grid-table": "Interactive Table",
+ }
+
+ def pre_validate(
+ self,
+ config: dict[str, Any],
+ ) -> ChartGenerationError | None:
+ columns = (
+ config.get("columns") or config.get("all_columns") or
config.get("groupby")
+ )
+ if not columns:
+ return ChartGenerationError(
+ error_type="missing_columns",
+ message="Table chart missing required field: columns",
+ details=(
+ "Table charts require a 'columns' array to specify which "
+ "columns to display"
+ ),
+ suggestions=[
+ "Add 'columns' field with array of column specifications",
+ "Example: 'columns': [{'name': 'product'}, {'name':
'sales', "
+ "'aggregate': 'SUM'}]",
+ "Each column can have optional 'aggregate' for metrics",
+ ],
+ error_code="MISSING_COLUMNS",
+ )
+
+ if not isinstance(columns, list):
+ return ChartGenerationError(
+ error_type="invalid_columns_format",
+ message="Columns must be a list",
+ details="The 'columns' field must be an array of column
specifications",
+ suggestions=[
+ "Ensure columns is an array: 'columns': [...]",
+ "Each column should be an object with 'name' field",
+ ],
+ error_code="INVALID_COLUMNS_FORMAT",
+ )
+
+ return None
+
+ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
+ if not isinstance(config, TableChartConfig):
+ return []
+ refs: list[ColumnRef] = list(config.columns)
+ if config.filters:
+ for f in config.filters:
+ refs.append(ColumnRef(name=f.column))
+ return refs
Review Comment:
Valid enhancement — `sort_by` entries (`str | SortByConfig`) are not
currently included in `extract_column_refs`, so the dataset validator does not
check whether sort targets exist against the dataset. This PR's scope is the
plugin registry architecture refactor, not expanding column validation
coverage; `sort_by` validation was also absent before this PR. Adding it would
require handling both bare-string entries and `SortByConfig` objects in
`extract_column_refs`, which is a clean follow-up. Noted as a future
improvement.
--
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]
