codeant-ai-for-open-source[bot] commented on code in PR #41860: URL: https://github.com/apache/superset/pull/41860#discussion_r3575758151
########## superset/mcp_service/chart/plugins/histogram.py: ########## @@ -0,0 +1,197 @@ +# 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. + +"""Histogram chart type plugin.""" + +from __future__ import annotations + +import re +from collections.abc import Mapping +from typing import Any, ClassVar + +from superset.mcp_service.chart.chart_utils import ( + _summarize_filters, + map_histogram_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import ColumnRef, HistogramChartConfig +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class HistogramChartPlugin(BaseChartPlugin): + """Plugin for histogram chart type.""" + + chart_type = "histogram" + display_name = "Histogram" + native_viz_types: ClassVar[Mapping[str, str]] = { + "histogram_v2": "Histogram", + } + + def pre_validate( + self, + config: dict[str, Any], + ) -> ChartGenerationError | None: + if "column" not in config: + return ChartGenerationError( + error_type="missing_histogram_fields", + message="Histogram missing required field: 'column'", + details=( + "Histograms bin the values of a single numeric column " + "into frequency buckets" + ), + suggestions=[ + "Add 'column' field: {'name': 'numeric_column'}", + "Example: {'chart_type': 'histogram', " + "'column': {'name': 'trip_duration'}, 'bins': 10}", + ], + error_code="MISSING_HISTOGRAM_FIELDS", + ) + return None + + def extract_column_refs(self, config: Any) -> list[ColumnRef]: + if not isinstance(config, HistogramChartConfig): + return [] + refs: list[ColumnRef] = [config.column] + refs.extend(config.groupby or []) + if config.filters: + for f in config.filters: + refs.append(ColumnRef(name=f.column)) + return refs + + def to_form_data( + self, config: Any, dataset_id: int | str | None = None + ) -> dict[str, Any]: + return map_histogram_config(config) + + def generate_name(self, config: Any, dataset_name: str | None = None) -> str: + what = f"Distribution of {config.column.label or config.column.name}" + if config.groupby: + what += " by " + ", ".join(g.label or g.name for g in config.groupby) + context = _summarize_filters(config.filters) + return self._with_context(what, context) + + def post_map_validate( + self, + config: Any, + form_data: dict[str, Any], + dataset_id: int | str | None = None, + ) -> ChartGenerationError | None: + """Require a numeric binned column, mirroring the Explore UI. + + The frontend control panel restricts the histogram column to + ``GenericDataType.Numeric``; without this check an LLM picking a + text column passes pre-validation and only fails at query time. + """ + if not isinstance(config, HistogramChartConfig) or dataset_id is None: + return None + + dataset_context = DatasetValidator._get_dataset_context(dataset_id) + if dataset_context is None: + return None + + col_info = next( + ( + col + for col in dataset_context.available_columns + if col["name"].lower() == (config.column.name or "").lower() + ), + None, + ) + if col_info is None: + # Column existence is validated separately; don't double-report. + return None + + def _is_numeric(col: dict[str, Any]) -> bool: + if col.get("is_numeric", False): + return True + # Backends report many spellings (BIGINT, SMALLINT, REAL, NUMBER, + # DOUBLE PRECISION); match numeric tokens at word boundaries so + # INTERVAL/POINT (which merely contain "INT") stay non-numeric. + type_upper = str(col.get("type", "")).upper() + return bool( + re.search( + r"\b(?:TINY|SMALL|MEDIUM|BIG)?INT(?:EGER)?\b" + r"|\bFLOAT\b|\bDOUBLE\b|\bDECIMAL\b" + r"|\bNUMERIC\b|\bREAL\b|\bNUMBER\b", + type_upper, + ) + ) + + if _is_numeric(col_info): + return None + + numeric_columns = sorted( + col["name"] for col in dataset_context.available_columns if _is_numeric(col) + ) + return ChartGenerationError( + error_type="non_numeric_histogram_column", + message=( + f"Histograms bin numeric values; column " + f"'{config.column.name}' has type " + f"{col_info.get('type', 'UNKNOWN')}." + ), + details=( + "The Explore UI restricts the histogram column to numeric " + "columns; non-numeric columns fail or produce meaningless " + "bins at query time." + ), + suggestions=( + [f"Numeric columns in this dataset: {', '.join(numeric_columns)}"] + if numeric_columns + else ["Use get_dataset_info to inspect the dataset's columns"] + ) + + ["For category frequencies, use chart_type='xy' with kind='bar'"], + error_code="NON_NUMERIC_HISTOGRAM_COLUMN", + ) + + def resolve_viz_type(self, config: Any) -> str: + return "histogram_v2" Review Comment: **Suggestion:** The plugin resolves histogram charts to `histogram_v2`, but downstream preview/type-mapping code in the MCP chart-preview path still classifies only `histogram`. This causes histogram charts created through this plugin to be misclassified and rendered through fallback preview logic. Keep the viz-type contract aligned across tools by updating downstream viz-type mappings to include `histogram_v2` wherever histogram is recognized. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Histogram Vega-Lite preview renders scatter, misrepresenting distribution. - ⚠️ LLM-driven chart previews mislead users about histogram charts. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Create or identify a chart whose Superset viz_type is "histogram_v2" (e.g., via MCP generate_chart using HistogramChartConfig, which maps to form_data with viz_type "histogram_v2" in superset/mcp_service/chart/chart_utils.py:45-47, or an existing Explore histogram chart). 2. Call the MCP tool get_chart_preview (superset/mcp_service/chart/tool/get_chart_preview.py:143-145) with a GetChartPreviewRequest (superset/mcp_service/chart/schemas.py:2498-2534) specifying identifier equal to that chart’s ID/UUID and format="vega_lite". 3. Inside VegaLitePreviewStrategy.generate() (superset/mcp_service/chart/tool/get_chart_preview.py:151-217), the helper _create_vega_lite_spec() reads viz_type = getattr(self.chart, "viz_type", "table") or "table" (line 249), which for this chart is "histogram_v2", then calls _get_chart_spec_for_type(viz_type, fields, field_types) (lines 10-12). 4. In _get_chart_spec_for_type() (superset/mcp_service/chart/tool/get_chart_preview.py:10-50), chart_type_mapping defines the histogram family as "histogram": ["histogram"] and does not include "histogram_v2" (lines 32-33). Because chart.viz_type is "histogram_v2", no mapping matches, the loop falls through to the fallback path, logging "Unknown chart type 'histogram_v2', using scatter plot fallback" and returning _scatter_chart_spec(fields, field_types) (lines 41-50). The resulting ChartPreview (lines 82-92) therefore exposes chart_type "histogram_v2" but its Vega-Lite specification renders a scatter plot instead of a histogram. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8122a69df34c4f2997f6f067569b3af4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8122a69df34c4f2997f6f067569b3af4&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/plugins/histogram.py **Line:** 162:163 **Comment:** *Api Mismatch: The plugin resolves histogram charts to `histogram_v2`, but downstream preview/type-mapping code in the MCP chart-preview path still classifies only `histogram`. This causes histogram charts created through this plugin to be misclassified and rendered through fallback preview logic. Keep the viz-type contract aligned across tools by updating downstream viz-type mappings to include `histogram_v2` wherever histogram is recognized. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41860&comment_hash=1454998747fe7275f506edbe1f7acf43c0eae47f599897707a54fb852b1d16b1&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41860&comment_hash=1454998747fe7275f506edbe1f7acf43c0eae47f599897707a54fb852b1d16b1&reaction=dislike'>👎</a> -- 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]
