codeant-ai-for-open-source[bot] commented on code in PR #41860: URL: https://github.com/apache/superset/pull/41860#discussion_r3565575459
########## tests/unit_tests/mcp_service/chart/test_histogram_boxplot_charts.py: ########## @@ -0,0 +1,315 @@ +# 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. + +"""Tests for the histogram and box plot chart type plugins. + +Schema validation, form_data mapping (matching the frontend buildQuery +contracts for viz_type ``histogram_v2`` and ``box_plot``), and registry +integration. +""" + +import pytest +from pydantic import TypeAdapter, ValidationError + +from superset.mcp_service.chart.chart_utils import ( + map_box_plot_config, + map_histogram_config, +) +from superset.mcp_service.chart.schemas import ( + BoxPlotChartConfig, + ChartConfig, + HistogramChartConfig, +) + + +class TestHistogramChartConfigSchema: + """HistogramChartConfig schema validation.""" + + def test_basic_histogram_config(self) -> None: + config = HistogramChartConfig( + chart_type="histogram", + column={"name": "trip_duration"}, + ) + assert config.column.name == "trip_duration" + assert config.bins == 5 # frontend controlPanel default + assert config.normalize is False + assert config.cumulative is False + assert config.groupby is None + + def test_histogram_config_with_all_options(self) -> None: + config = HistogramChartConfig( + chart_type="histogram", + column={"name": "fare_amount"}, + groupby=[{"name": "payment_type"}], + bins=20, + normalize=True, + cumulative=True, + row_limit=500, + filters=[{"column": "year", "op": "=", "value": 2026}], + ) + assert config.bins == 20 + assert config.normalize is True + assert config.cumulative is True + assert [g.name for g in config.groupby or []] == ["payment_type"] + + def test_histogram_missing_column(self) -> None: + with pytest.raises(ValidationError): + HistogramChartConfig(chart_type="histogram") + + def test_histogram_rejects_extra_fields(self) -> None: + with pytest.raises(ValidationError): + HistogramChartConfig( + chart_type="histogram", + column={"name": "x"}, + nonsense_field=True, + ) + + def test_histogram_bins_bounds(self) -> None: + with pytest.raises(ValidationError): + HistogramChartConfig(chart_type="histogram", column={"name": "x"}, bins=0) + with pytest.raises(ValidationError): + HistogramChartConfig( + chart_type="histogram", column={"name": "x"}, bins=1001 + ) + + def test_histogram_column_rejects_saved_metric(self) -> None: + """The binned column is a physical column, not a metric.""" + with pytest.raises(ValidationError): + HistogramChartConfig( + chart_type="histogram", + column={"name": "count", "saved_metric": True}, + ) + + def test_chart_config_union_dispatches_histogram(self) -> None: + config = TypeAdapter(ChartConfig).validate_python( Review Comment: **Suggestion:** Provide an explicit annotation for this union-validated configuration object to keep variable typing explicit in new code. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This local variable is introduced without a type annotation in new Python code, and it can be annotated with the validated chart config type. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d660673ffb374907be05d0c7cc704fe6&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=d660673ffb374907be05d0c7cc704fe6&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:** tests/unit_tests/mcp_service/chart/test_histogram_boxplot_charts.py **Line:** 98:98 **Comment:** *Custom Rule: Provide an explicit annotation for this union-validated configuration object to keep variable typing explicit in new code. 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=668ea32f99624e7ac8f099a9e9add4d913368ce14952f4a85866f22e2f660701&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41860&comment_hash=668ea32f99624e7ac8f099a9e9add4d913368ce14952f4a85866f22e2f660701&reaction=dislike'>๐</a> ########## superset/mcp_service/chart/plugins/box_plot.py: ########## @@ -0,0 +1,145 @@ +# 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. + +"""Box plot chart type plugin.""" + +from __future__ import annotations + +from collections.abc import Mapping +from typing import Any, ClassVar + +from superset.mcp_service.chart.chart_utils import ( + _summarize_filters, + map_box_plot_config, +) +from superset.mcp_service.chart.plugin import BaseChartPlugin +from superset.mcp_service.chart.schemas import BoxPlotChartConfig, ColumnRef +from superset.mcp_service.chart.validation.dataset_validator import DatasetValidator +from superset.mcp_service.common.error_schemas import ChartGenerationError + + +class BoxPlotChartPlugin(BaseChartPlugin): + """Plugin for box plot chart type.""" + Review Comment: **Suggestion:** Add an explicit class-level type annotation for this string constant. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This new class-level string constant is introduced without an explicit type annotation, which matches the rule requiring type hints for annotatable Python variables. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=922219f2fa2441c384223c6ea724e807&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=922219f2fa2441c384223c6ea724e807&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/box_plot.py **Line:** 37:37 **Comment:** *Custom Rule: Add an explicit class-level type annotation for this string constant. 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=027606f6ee9f8d380dc8e4972c06b0eb7910d9248bb0371a87b6fc26cb8ad7fe&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41860&comment_hash=027606f6ee9f8d380dc8e4972c06b0eb7910d9248bb0371a87b6fc26cb8ad7fe&reaction=dislike'>๐</a> ########## tests/unit_tests/mcp_service/chart/test_histogram_boxplot_charts.py: ########## @@ -0,0 +1,315 @@ +# 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. + +"""Tests for the histogram and box plot chart type plugins. + +Schema validation, form_data mapping (matching the frontend buildQuery +contracts for viz_type ``histogram_v2`` and ``box_plot``), and registry +integration. +""" + +import pytest +from pydantic import TypeAdapter, ValidationError + +from superset.mcp_service.chart.chart_utils import ( + map_box_plot_config, + map_histogram_config, +) +from superset.mcp_service.chart.schemas import ( + BoxPlotChartConfig, + ChartConfig, + HistogramChartConfig, +) + + +class TestHistogramChartConfigSchema: + """HistogramChartConfig schema validation.""" + + def test_basic_histogram_config(self) -> None: + config = HistogramChartConfig( Review Comment: **Suggestion:** Add an explicit type annotation for this test-local configuration variable to satisfy the required type-hint rule. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This new local variable is introduced without any type annotation, which matches the custom rule requiring explicit type hints for relevant Python variables that can be annotated. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=050e5d2d0d3c4060955d51521c301be9&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=050e5d2d0d3c4060955d51521c301be9&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:** tests/unit_tests/mcp_service/chart/test_histogram_boxplot_charts.py **Line:** 43:43 **Comment:** *Custom Rule: Add an explicit type annotation for this test-local configuration variable to satisfy the required type-hint rule. 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=0fe371bdd8288f3d5760a82058fef33dd2477a6a69eee8218376083b001508d7&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41860&comment_hash=0fe371bdd8288f3d5760a82058fef33dd2477a6a69eee8218376083b001508d7&reaction=dislike'>๐</a> ########## tests/unit_tests/mcp_service/chart/test_histogram_boxplot_charts.py: ########## @@ -0,0 +1,315 @@ +# 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. + +"""Tests for the histogram and box plot chart type plugins. + +Schema validation, form_data mapping (matching the frontend buildQuery +contracts for viz_type ``histogram_v2`` and ``box_plot``), and registry +integration. +""" + +import pytest +from pydantic import TypeAdapter, ValidationError + +from superset.mcp_service.chart.chart_utils import ( + map_box_plot_config, + map_histogram_config, +) +from superset.mcp_service.chart.schemas import ( + BoxPlotChartConfig, + ChartConfig, + HistogramChartConfig, +) + + +class TestHistogramChartConfigSchema: + """HistogramChartConfig schema validation.""" + + def test_basic_histogram_config(self) -> None: + config = HistogramChartConfig( + chart_type="histogram", + column={"name": "trip_duration"}, + ) + assert config.column.name == "trip_duration" + assert config.bins == 5 # frontend controlPanel default + assert config.normalize is False + assert config.cumulative is False + assert config.groupby is None + + def test_histogram_config_with_all_options(self) -> None: + config = HistogramChartConfig( + chart_type="histogram", + column={"name": "fare_amount"}, + groupby=[{"name": "payment_type"}], + bins=20, + normalize=True, + cumulative=True, + row_limit=500, + filters=[{"column": "year", "op": "=", "value": 2026}], + ) + assert config.bins == 20 + assert config.normalize is True + assert config.cumulative is True + assert [g.name for g in config.groupby or []] == ["payment_type"] + + def test_histogram_missing_column(self) -> None: + with pytest.raises(ValidationError): + HistogramChartConfig(chart_type="histogram") + + def test_histogram_rejects_extra_fields(self) -> None: + with pytest.raises(ValidationError): + HistogramChartConfig( + chart_type="histogram", + column={"name": "x"}, + nonsense_field=True, + ) + + def test_histogram_bins_bounds(self) -> None: + with pytest.raises(ValidationError): + HistogramChartConfig(chart_type="histogram", column={"name": "x"}, bins=0) + with pytest.raises(ValidationError): + HistogramChartConfig( + chart_type="histogram", column={"name": "x"}, bins=1001 + ) + + def test_histogram_column_rejects_saved_metric(self) -> None: + """The binned column is a physical column, not a metric.""" + with pytest.raises(ValidationError): + HistogramChartConfig( + chart_type="histogram", + column={"name": "count", "saved_metric": True}, + ) + + def test_chart_config_union_dispatches_histogram(self) -> None: + config = TypeAdapter(ChartConfig).validate_python( + {"chart_type": "histogram", "column": {"name": "duration"}} + ) + assert isinstance(config, HistogramChartConfig) + + +class TestMapHistogramConfig: + """form_data mapping must match the frontend Histogram buildQuery.""" + + def test_basic_histogram_form_data(self) -> None: + config = HistogramChartConfig( + chart_type="histogram", column={"name": "trip_duration"} + ) + form_data = map_histogram_config(config) Review Comment: **Suggestion:** Add a concrete type annotation for this mapped form-data variable so the new code complies with type-hint requirements. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This is a new local variable assigned the result of a function call and it has no explicit type hint, so it violates the type-hint rule for annotatable variables. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=75b69481775a4687b333a9c5e5da2b36&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=75b69481775a4687b333a9c5e5da2b36&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:** tests/unit_tests/mcp_service/chart/test_histogram_boxplot_charts.py **Line:** 111:111 **Comment:** *Custom Rule: Add a concrete type annotation for this mapped form-data variable so the new code complies with type-hint requirements. 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=45dfd037d5f766510ec80fcd9f86a175d997552f1a6880e2046aa486cc14c9de&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41860&comment_hash=45dfd037d5f766510ec80fcd9f86a175d997552f1a6880e2046aa486cc14c9de&reaction=dislike'>๐</a> ########## tests/unit_tests/mcp_service/chart/test_histogram_boxplot_charts.py: ########## @@ -0,0 +1,315 @@ +# 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. + +"""Tests for the histogram and box plot chart type plugins. + +Schema validation, form_data mapping (matching the frontend buildQuery +contracts for viz_type ``histogram_v2`` and ``box_plot``), and registry +integration. +""" + +import pytest +from pydantic import TypeAdapter, ValidationError + +from superset.mcp_service.chart.chart_utils import ( + map_box_plot_config, + map_histogram_config, +) +from superset.mcp_service.chart.schemas import ( + BoxPlotChartConfig, + ChartConfig, + HistogramChartConfig, +) + + +class TestHistogramChartConfigSchema: + """HistogramChartConfig schema validation.""" + + def test_basic_histogram_config(self) -> None: + config = HistogramChartConfig( + chart_type="histogram", + column={"name": "trip_duration"}, + ) + assert config.column.name == "trip_duration" + assert config.bins == 5 # frontend controlPanel default + assert config.normalize is False + assert config.cumulative is False + assert config.groupby is None + + def test_histogram_config_with_all_options(self) -> None: + config = HistogramChartConfig( + chart_type="histogram", + column={"name": "fare_amount"}, + groupby=[{"name": "payment_type"}], + bins=20, + normalize=True, + cumulative=True, + row_limit=500, + filters=[{"column": "year", "op": "=", "value": 2026}], + ) + assert config.bins == 20 + assert config.normalize is True + assert config.cumulative is True + assert [g.name for g in config.groupby or []] == ["payment_type"] + + def test_histogram_missing_column(self) -> None: + with pytest.raises(ValidationError): + HistogramChartConfig(chart_type="histogram") + + def test_histogram_rejects_extra_fields(self) -> None: + with pytest.raises(ValidationError): + HistogramChartConfig( + chart_type="histogram", + column={"name": "x"}, + nonsense_field=True, + ) + + def test_histogram_bins_bounds(self) -> None: + with pytest.raises(ValidationError): + HistogramChartConfig(chart_type="histogram", column={"name": "x"}, bins=0) + with pytest.raises(ValidationError): + HistogramChartConfig( + chart_type="histogram", column={"name": "x"}, bins=1001 + ) + + def test_histogram_column_rejects_saved_metric(self) -> None: + """The binned column is a physical column, not a metric.""" + with pytest.raises(ValidationError): + HistogramChartConfig( + chart_type="histogram", + column={"name": "count", "saved_metric": True}, + ) + + def test_chart_config_union_dispatches_histogram(self) -> None: + config = TypeAdapter(ChartConfig).validate_python( + {"chart_type": "histogram", "column": {"name": "duration"}} + ) + assert isinstance(config, HistogramChartConfig) + + +class TestMapHistogramConfig: + """form_data mapping must match the frontend Histogram buildQuery.""" + + def test_basic_histogram_form_data(self) -> None: + config = HistogramChartConfig( + chart_type="histogram", column={"name": "trip_duration"} + ) + form_data = map_histogram_config(config) + assert form_data["viz_type"] == "histogram_v2" + assert form_data["column"] == "trip_duration" + assert form_data["groupby"] == [] + assert form_data["bins"] == 5 + assert form_data["normalize"] is False + assert form_data["cumulative"] is False + + def test_histogram_form_data_with_options_and_filters(self) -> None: + config = HistogramChartConfig( + chart_type="histogram", + column={"name": "fare_amount"}, + groupby=[{"name": "payment_type"}], + bins=20, + normalize=True, + cumulative=True, + filters=[{"column": "year", "op": "=", "value": 2026}], + ) + form_data = map_histogram_config(config) + assert form_data["groupby"] == ["payment_type"] + assert form_data["bins"] == 20 + assert form_data["normalize"] is True + assert form_data["cumulative"] is True + assert form_data["adhoc_filters"], "filters must map to adhoc_filters" + + +class TestBoxPlotChartConfigSchema: + """BoxPlotChartConfig schema validation.""" + + def test_basic_box_plot_config(self) -> None: + config = BoxPlotChartConfig( + chart_type="box_plot", + metrics=[{"name": "fare_amount", "aggregate": "AVG"}], + distribute_across=[{"name": "day_of_week"}], + ) + assert config.whisker_type == "tukey" + assert config.dimensions is None + assert [c.name for c in config.distribute_across] == ["day_of_week"] + + def test_box_plot_missing_metrics(self) -> None: + with pytest.raises(ValidationError): + BoxPlotChartConfig( + chart_type="box_plot", + distribute_across=[{"name": "day"}], + ) + + def test_box_plot_empty_distribute_across(self) -> None: + with pytest.raises(ValidationError): + BoxPlotChartConfig( + chart_type="box_plot", + metrics=[{"name": "x", "aggregate": "AVG"}], + distribute_across=[], + ) + + def test_box_plot_rejects_extra_fields(self) -> None: + with pytest.raises(ValidationError): + BoxPlotChartConfig( + chart_type="box_plot", + metrics=[{"name": "x", "aggregate": "AVG"}], + distribute_across=[{"name": "day"}], + bogus=1, + ) + + def test_box_plot_percentile_requires_bounds(self) -> None: + with pytest.raises(ValidationError): + BoxPlotChartConfig( + chart_type="box_plot", + metrics=[{"name": "x", "aggregate": "AVG"}], + distribute_across=[{"name": "day"}], + whisker_type="percentile", + ) + + def test_box_plot_percentile_bounds_must_be_ordered(self) -> None: + with pytest.raises(ValidationError): + BoxPlotChartConfig( + chart_type="box_plot", + metrics=[{"name": "x", "aggregate": "AVG"}], + distribute_across=[{"name": "day"}], + whisker_type="percentile", + percentile_low=90, + percentile_high=10, + ) + + def test_chart_config_union_dispatches_box_plot(self) -> None: + config = TypeAdapter(ChartConfig).validate_python( + { + "chart_type": "box_plot", + "metrics": [{"name": "fare", "aggregate": "AVG"}], + "distribute_across": [{"name": "day"}], + } + ) + assert isinstance(config, BoxPlotChartConfig) + + +class TestMapBoxPlotConfig: + """form_data mapping must match the frontend BoxPlot buildQuery.""" + + def test_basic_box_plot_form_data(self) -> None: + config = BoxPlotChartConfig( + chart_type="box_plot", + metrics=[{"name": "fare_amount", "aggregate": "AVG"}], + distribute_across=[{"name": "day_of_week"}], + dimensions=[{"name": "payment_type"}], + ) + form_data = map_box_plot_config(config) + assert form_data["viz_type"] == "box_plot" + assert form_data["columns"] == ["day_of_week"] + assert form_data["groupby"] == ["payment_type"] + assert form_data["whiskerOptions"] == "Tukey" + assert len(form_data["metrics"]) == 1 + + def test_box_plot_whisker_min_max(self) -> None: + config = BoxPlotChartConfig( + chart_type="box_plot", + metrics=[{"name": "x", "aggregate": "AVG"}], + distribute_across=[{"name": "day"}], + whisker_type="min_max", + ) + assert map_box_plot_config(config)["whiskerOptions"] == ( + "Min/max (no outliers)" + ) + + def test_box_plot_whisker_percentiles(self) -> None: + """Percentiles serialize to the exact string the frontend + boxplotOperator PERCENTILE_REGEX parses: '<low>/<high> percentiles'.""" + config = BoxPlotChartConfig( + chart_type="box_plot", + metrics=[{"name": "x", "aggregate": "AVG"}], + distribute_across=[{"name": "day"}], + whisker_type="percentile", + percentile_low=10, + percentile_high=90, + ) + assert map_box_plot_config(config)["whiskerOptions"] == "10/90 percentiles" + + +class TestPluginRegistry: + """Both plugins must be registered and resolve their viz types.""" + + def test_histogram_plugin_registered(self) -> None: + from superset.mcp_service.chart import registry + + plugin = registry.get("histogram") + assert plugin is not None + assert plugin.resolve_viz_type(None) == "histogram_v2" + + def test_box_plot_plugin_registered(self) -> None: + from superset.mcp_service.chart import registry + + plugin = registry.get("box_plot") + assert plugin is not None + assert plugin.resolve_viz_type(None) == "box_plot" + + def test_display_names_resolve(self) -> None: + from superset.mcp_service.chart.registry import display_name_for_viz_type + + assert display_name_for_viz_type("histogram_v2") == "Histogram" + assert display_name_for_viz_type("box_plot") == "Box Plot" + + def test_histogram_pre_validate_missing_column(self) -> None: + from superset.mcp_service.chart import registry + + plugin = registry.get("histogram") + assert plugin is not None + error = plugin.pre_validate({"chart_type": "histogram"}) Review Comment: **Suggestion:** Add an explicit type annotation for this pre-validation result variable to comply with the type-hint rule. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> The variable is newly introduced without a type annotation and stores a validation result, which is a relevant variable that can be annotated under the rule. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cf7c396ec26b4cc99e9164deafadeb97&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=cf7c396ec26b4cc99e9164deafadeb97&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:** tests/unit_tests/mcp_service/chart/test_histogram_boxplot_charts.py **Line:** 275:275 **Comment:** *Custom Rule: Add an explicit type annotation for this pre-validation result variable to comply with the type-hint rule. 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=283c9c5b6cbd519ba3e8946ca648aedf865b8a4995ed7a96c59562018ffabbb4&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41860&comment_hash=283c9c5b6cbd519ba3e8946ca648aedf865b8a4995ed7a96c59562018ffabbb4&reaction=dislike'>๐</a> ########## tests/unit_tests/mcp_service/chart/test_histogram_boxplot_charts.py: ########## @@ -0,0 +1,315 @@ +# 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. + +"""Tests for the histogram and box plot chart type plugins. + +Schema validation, form_data mapping (matching the frontend buildQuery +contracts for viz_type ``histogram_v2`` and ``box_plot``), and registry +integration. +""" + +import pytest +from pydantic import TypeAdapter, ValidationError + +from superset.mcp_service.chart.chart_utils import ( + map_box_plot_config, + map_histogram_config, +) +from superset.mcp_service.chart.schemas import ( + BoxPlotChartConfig, + ChartConfig, + HistogramChartConfig, +) + + +class TestHistogramChartConfigSchema: + """HistogramChartConfig schema validation.""" + + def test_basic_histogram_config(self) -> None: + config = HistogramChartConfig( + chart_type="histogram", + column={"name": "trip_duration"}, + ) + assert config.column.name == "trip_duration" + assert config.bins == 5 # frontend controlPanel default + assert config.normalize is False + assert config.cumulative is False + assert config.groupby is None + + def test_histogram_config_with_all_options(self) -> None: + config = HistogramChartConfig( + chart_type="histogram", + column={"name": "fare_amount"}, + groupby=[{"name": "payment_type"}], + bins=20, + normalize=True, + cumulative=True, + row_limit=500, + filters=[{"column": "year", "op": "=", "value": 2026}], + ) + assert config.bins == 20 + assert config.normalize is True + assert config.cumulative is True + assert [g.name for g in config.groupby or []] == ["payment_type"] + + def test_histogram_missing_column(self) -> None: + with pytest.raises(ValidationError): + HistogramChartConfig(chart_type="histogram") + + def test_histogram_rejects_extra_fields(self) -> None: + with pytest.raises(ValidationError): + HistogramChartConfig( + chart_type="histogram", + column={"name": "x"}, + nonsense_field=True, + ) + + def test_histogram_bins_bounds(self) -> None: + with pytest.raises(ValidationError): + HistogramChartConfig(chart_type="histogram", column={"name": "x"}, bins=0) + with pytest.raises(ValidationError): + HistogramChartConfig( + chart_type="histogram", column={"name": "x"}, bins=1001 + ) + + def test_histogram_column_rejects_saved_metric(self) -> None: + """The binned column is a physical column, not a metric.""" + with pytest.raises(ValidationError): + HistogramChartConfig( + chart_type="histogram", + column={"name": "count", "saved_metric": True}, + ) + + def test_chart_config_union_dispatches_histogram(self) -> None: + config = TypeAdapter(ChartConfig).validate_python( + {"chart_type": "histogram", "column": {"name": "duration"}} + ) + assert isinstance(config, HistogramChartConfig) + + +class TestMapHistogramConfig: + """form_data mapping must match the frontend Histogram buildQuery.""" + + def test_basic_histogram_form_data(self) -> None: + config = HistogramChartConfig( + chart_type="histogram", column={"name": "trip_duration"} + ) + form_data = map_histogram_config(config) + assert form_data["viz_type"] == "histogram_v2" + assert form_data["column"] == "trip_duration" + assert form_data["groupby"] == [] + assert form_data["bins"] == 5 + assert form_data["normalize"] is False + assert form_data["cumulative"] is False + + def test_histogram_form_data_with_options_and_filters(self) -> None: + config = HistogramChartConfig( + chart_type="histogram", + column={"name": "fare_amount"}, + groupby=[{"name": "payment_type"}], + bins=20, + normalize=True, + cumulative=True, + filters=[{"column": "year", "op": "=", "value": 2026}], + ) + form_data = map_histogram_config(config) + assert form_data["groupby"] == ["payment_type"] + assert form_data["bins"] == 20 + assert form_data["normalize"] is True + assert form_data["cumulative"] is True + assert form_data["adhoc_filters"], "filters must map to adhoc_filters" + + +class TestBoxPlotChartConfigSchema: + """BoxPlotChartConfig schema validation.""" + + def test_basic_box_plot_config(self) -> None: + config = BoxPlotChartConfig( + chart_type="box_plot", + metrics=[{"name": "fare_amount", "aggregate": "AVG"}], + distribute_across=[{"name": "day_of_week"}], + ) + assert config.whisker_type == "tukey" + assert config.dimensions is None + assert [c.name for c in config.distribute_across] == ["day_of_week"] + + def test_box_plot_missing_metrics(self) -> None: + with pytest.raises(ValidationError): + BoxPlotChartConfig( + chart_type="box_plot", + distribute_across=[{"name": "day"}], + ) + + def test_box_plot_empty_distribute_across(self) -> None: + with pytest.raises(ValidationError): + BoxPlotChartConfig( + chart_type="box_plot", + metrics=[{"name": "x", "aggregate": "AVG"}], + distribute_across=[], + ) + + def test_box_plot_rejects_extra_fields(self) -> None: + with pytest.raises(ValidationError): + BoxPlotChartConfig( + chart_type="box_plot", + metrics=[{"name": "x", "aggregate": "AVG"}], + distribute_across=[{"name": "day"}], + bogus=1, + ) + + def test_box_plot_percentile_requires_bounds(self) -> None: + with pytest.raises(ValidationError): + BoxPlotChartConfig( + chart_type="box_plot", + metrics=[{"name": "x", "aggregate": "AVG"}], + distribute_across=[{"name": "day"}], + whisker_type="percentile", + ) + + def test_box_plot_percentile_bounds_must_be_ordered(self) -> None: + with pytest.raises(ValidationError): + BoxPlotChartConfig( + chart_type="box_plot", + metrics=[{"name": "x", "aggregate": "AVG"}], + distribute_across=[{"name": "day"}], + whisker_type="percentile", + percentile_low=90, + percentile_high=10, + ) + + def test_chart_config_union_dispatches_box_plot(self) -> None: + config = TypeAdapter(ChartConfig).validate_python( + { + "chart_type": "box_plot", + "metrics": [{"name": "fare", "aggregate": "AVG"}], + "distribute_across": [{"name": "day"}], + } + ) + assert isinstance(config, BoxPlotChartConfig) + + +class TestMapBoxPlotConfig: + """form_data mapping must match the frontend BoxPlot buildQuery.""" + + def test_basic_box_plot_form_data(self) -> None: + config = BoxPlotChartConfig( + chart_type="box_plot", + metrics=[{"name": "fare_amount", "aggregate": "AVG"}], + distribute_across=[{"name": "day_of_week"}], + dimensions=[{"name": "payment_type"}], + ) + form_data = map_box_plot_config(config) + assert form_data["viz_type"] == "box_plot" + assert form_data["columns"] == ["day_of_week"] + assert form_data["groupby"] == ["payment_type"] + assert form_data["whiskerOptions"] == "Tukey" + assert len(form_data["metrics"]) == 1 + + def test_box_plot_whisker_min_max(self) -> None: + config = BoxPlotChartConfig( + chart_type="box_plot", + metrics=[{"name": "x", "aggregate": "AVG"}], + distribute_across=[{"name": "day"}], + whisker_type="min_max", + ) + assert map_box_plot_config(config)["whiskerOptions"] == ( + "Min/max (no outliers)" + ) + + def test_box_plot_whisker_percentiles(self) -> None: + """Percentiles serialize to the exact string the frontend + boxplotOperator PERCENTILE_REGEX parses: '<low>/<high> percentiles'.""" + config = BoxPlotChartConfig( + chart_type="box_plot", + metrics=[{"name": "x", "aggregate": "AVG"}], + distribute_across=[{"name": "day"}], + whisker_type="percentile", + percentile_low=10, + percentile_high=90, + ) + assert map_box_plot_config(config)["whiskerOptions"] == "10/90 percentiles" + + +class TestPluginRegistry: + """Both plugins must be registered and resolve their viz types.""" + + def test_histogram_plugin_registered(self) -> None: + from superset.mcp_service.chart import registry + + plugin = registry.get("histogram") Review Comment: **Suggestion:** Add a type hint to this plugin lookup variable so the new test code follows the required typing standard. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> This new local variable is unannotated even though its type can be expressed, so it fits the custom rule for missing type hints on relevant variables. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bf1e157a03234a43b13ba245e74dcbd5&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=bf1e157a03234a43b13ba245e74dcbd5&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:** tests/unit_tests/mcp_service/chart/test_histogram_boxplot_charts.py **Line:** 253:253 **Comment:** *Custom Rule: Add a type hint to this plugin lookup variable so the new test code follows the required typing standard. 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=690b19971cb7b8cb5e8c689f485d18359adbbf11d86f6af576d2eee2b9f0f371&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41860&comment_hash=690b19971cb7b8cb5e8c689f485d18359adbbf11d86f6af576d2eee2b9f0f371&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]
