codeant-ai-for-open-source[bot] commented on code in PR #41860:
URL: https://github.com/apache/superset/pull/41860#discussion_r3575591076
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1846,6 +1846,123 @@ def validate_unique_column_labels(self) ->
"XYChartConfig":
return self
+class HistogramChartConfig(UnknownFieldCheckMixin):
+ """Config for histogram charts (viz_type ``histogram_v2``)."""
+
+ model_config = ConfigDict(extra="ignore", populate_by_name=True)
+
+ chart_type: Literal["histogram"] = "histogram"
+ column: ColumnRef = Field(
+ ...,
+ description="Numeric column to bin (a physical dataset column)",
+ )
+ groupby: List[ColumnRef] | None = Field(
+ None,
+ description="Optional dimensions to split the distribution into
series",
+ )
+ bins: int = Field(5, description="Number of histogram bins", ge=1, le=1000)
+ normalize: bool = Field(False, description="Normalize bin counts to
proportions")
+ cumulative: bool = Field(False, description="Accumulate bin counts left to
right")
+ filters: List[FilterConfig] | None = Field(
+ None,
+ description="Structured filters (column/op/value). "
+ "Do NOT use adhoc_filters or raw SQL expressions.",
+ )
+ row_limit: int = Field(10000, description="Max rows sampled", ge=1,
le=100000)
+
+ @model_validator(mode="after")
+ def reject_metric_style_column(self) -> "HistogramChartConfig":
+ """The binned column is a physical column, not a metric."""
+ _reject_sql_expression_on_dimension(self.column, "column")
+ if self.column and self.column.saved_metric:
+ raise ValueError(
+ "column cannot use saved_metric=True; histograms bin a "
+ "physical numeric column"
+ )
+ for i, col in enumerate(self.groupby or []):
+ _reject_sql_expression_on_dimension(col, f"groupby[{i}]")
+ if col.saved_metric:
+ raise ValueError(
+ f"groupby[{i}] cannot use saved_metric=True; "
+ "saved metrics are not dimensions"
+ )
+ return self
+
+
+class BoxPlotChartConfig(UnknownFieldCheckMixin):
+ """Config for box plot charts (viz_type ``box_plot``)."""
+
+ model_config = ConfigDict(extra="ignore", populate_by_name=True)
+
+ chart_type: Literal["box_plot"] = "box_plot"
+ metrics: List[ColumnRef] = Field(
+ ...,
+ min_length=1,
+ description="Metrics whose distributions are plotted (use aggregate "
+ "e.g. AVG, SUM for ad-hoc, or saved_metric=True for saved metrics)",
+ )
+ distribute_across: List[ColumnRef] = Field(
+ ...,
+ min_length=1,
+ description="Columns whose values form the boxes along the x-axis "
+ "(one box per value)",
+ )
+ dimensions: List[ColumnRef] | None = Field(
+ None,
+ description="Optional series dimensions (one colored box group per
value)",
+ )
+ whisker_type: Literal["tukey", "min_max", "percentile"] = Field(
+ "tukey",
+ description="Whisker algorithm: 'tukey' (1.5 IQR), 'min_max' (no "
+ "outliers), or 'percentile' (requires percentile_low/percentile_high)",
+ )
+ percentile_low: int | None = Field(
+ None, description="Lower whisker percentile (0-100)", ge=0, le=100
+ )
+ percentile_high: int | None = Field(
+ None, description="Upper whisker percentile (0-100)", ge=0, le=100
+ )
+ filters: List[FilterConfig] | None = Field(
+ None,
+ description="Structured filters (column/op/value). "
+ "Do NOT use adhoc_filters or raw SQL expressions.",
+ )
+ row_limit: int = Field(100, description="Max boxes", ge=1, le=10000)
Review Comment:
**Suggestion:** The default `row_limit` for box plots is set to `100`, but
the frontend box-plot control uses the shared row-limit default of `10000`.
This mismatch will silently truncate query rows for MCP-generated box plots,
which can materially distort quartiles/outlier detection compared with the same
chart built in Explore. Align the default with the frontend/shared control
default to avoid biased statistical results. [logic error]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β MCP box-plot charts truncate data to 100 boxes.
- β Quartile and outlier statistics differ from Explore box plots.
- β οΈ Agents misinterpret distributions due to silently truncated series.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Start the MCP service and ensure the `generate_chart` tool is available;
its chart
config discriminator is defined in
`superset/mcp_service/chart/schemas.py:1966-1975` as
`ChartConfig = Annotated[..., Field(discriminator="chart_type")]`, which
includes
`BoxPlotChartConfig`.
2. Call the MCP `generate_chart` tool with a request that resolves to
`chart_type="box_plot"` (via either explicit `chart_type` or
`viz_type="box_plot"` mapped
in the plugin under `superset/mcp_service/chart/plugins/box_plot.py:1-200`),
providing
required `metrics` and `distribute_across` fields but omitting `row_limit`.
3. The request normalization and Pydantic validation pipeline constructs a
`BoxPlotChartConfig` instance
(`superset/mcp_service/chart/schemas.py:1892-1933`); because
`row_limit` is not provided in the request, it takes the default `100` from
`row_limit:
int = Field(100, description="Max boxes", ge=1, le=10000)` at line 1930.
4. The box-plot plugin then maps this config into Superset-style `form_data`
(in
`superset/mcp_service/chart/plugins/box_plot.py`, around the form-data
builder function),
setting `row_limit=100`; the resulting query for the MCP-generated box plot
is limited to
100 category values, whereas the Explore frontend box-plot control (in the
main Superset
UI, e.g. `superset-frontend/src/explore/controlPanels/BoxPlot.tsx`) uses the
shared
default `row_limit=10000`. Comparing the MCP-generated chart to an
Explore-created box
plot on the same dataset shows truncated series and materially different
quartiles/outlier
detection because 9,900 potential rows are silently dropped in the MCP path.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=531a56495865468e8060f11f6b46c677&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=531a56495865468e8060f11f6b46c677&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/schemas.py
**Line:** 1930:1930
**Comment:**
*Logic Error: The default `row_limit` for box plots is set to `100`,
but the frontend box-plot control uses the shared row-limit default of `10000`.
This mismatch will silently truncate query rows for MCP-generated box plots,
which can materially distort quartiles/outlier detection compared with the same
chart built in Explore. Align the default with the frontend/shared control
default to avoid biased statistical results.
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=43c427473961cd1da570c954e5092e0091ec613212aaf1b239e13eb105fd5d46&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41860&comment_hash=43c427473961cd1da570c954e5092e0091ec613212aaf1b239e13eb105fd5d46&reaction=dislike'>π</a>
##########
tests/unit_tests/mcp_service/chart/test_histogram_boxplot_charts.py:
##########
@@ -0,0 +1,371 @@
+# 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,
+)
+from superset.mcp_service.common.error_schemas import ChartGenerationError
Review Comment:
**Suggestion:** The import is unused because the only reference is inside a
quoted forward annotation, so Ruff/Pyflakes will raise `F401` and fail CI.
Remove the import or switch the annotation to a real (non-string) type
reference with postponed evaluation so the symbol is actually used. [code
quality]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β Linting stage fails due to unused import F401.
- β οΈ PRs blocked until import removed or annotation reference updated.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Open
`tests/unit_tests/mcp_service/chart/test_histogram_boxplot_charts.py` and note
the
import of `ChartGenerationError` at line 37: `from
superset.mcp_service.common.error_schemas import ChartGenerationError`.
2. In the same file, observe that the only mention of `ChartGenerationError`
is in a
quoted return annotation at line 333 inside
`TestHistogramNumericColumnValidation._validate`: `) ->
"ChartGenerationError | None":`,
so the name never appears as a real Python symbol reference.
3. Run the linter on this file, for example `ruff
tests/unit_tests/mcp_service/chart/test_histogram_boxplot_charts.py`;
Ruff/Pyflakes do not
treat strings as symbol usage, so they report `F401: 'ChartGenerationError'
imported but
unused` for the import at line 37.
4. Because the repositoryβs CI runs linting over the test suite, this F401
causes the lint
job (and thus the overall CI for the PR) to fail until the import is removed
or the
annotation is changed to a real type reference (e.g., `ChartGenerationError
| None`).
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4ec486404bc64fefaeeef57b84175b17&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=4ec486404bc64fefaeeef57b84175b17&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:** 37:37
**Comment:**
*Code Quality: The import is unused because the only reference is
inside a quoted forward annotation, so Ruff/Pyflakes will raise `F401` and fail
CI. Remove the import or switch the annotation to a real (non-string) type
reference with postponed evaluation so the symbol is actually used.
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=e474057bb5b7d24295546ee60a75253bdbcc9058d1d8347a3062a3196b0a483e&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41860&comment_hash=e474057bb5b7d24295546ee60a75253bdbcc9058d1d8347a3062a3196b0a483e&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]
