codeant-ai-for-open-source[bot] commented on code in PR #41860:
URL: https://github.com/apache/superset/pull/41860#discussion_r3575646541
##########
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)",
+ )
Review Comment:
**Suggestion:** `box_plot` currently rejects Superset-native payload keys
(`columns`/`groupby`) because the schema only accepts
`distribute_across`/`dimensions` and the pre-validation path checks only those
names. This breaks compatibility when callers send existing form_data
vocabulary (which the chart request normalizer is intended to accept). Add
input aliases (and matching pre-validate support) so `columns` maps to
`distribute_across` and `groupby` maps to `dimensions`. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ generate_chart rejects box_plot configs using columns/groupby.
- ⚠️ Superset form_data compatibility broken for box_plot inputs.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the MCP `generate_chart` tool (entrypoint at
`superset/mcp_service/chart/tool/generate_chart.py:240`) with a request body
containing
`dataset_id` and `config`, where `config` uses Superset box-plot form_data
keys:
`"chart_type": "box_plot"`, `"metrics": [...]`, `"columns": [{"name":
"day_of_week"}]`,
and `"groupby": [{"name": "region"}]`, matching the frontend contract
documented in
`map_box_plot_config()` at `superset/mcp_service/chart/chart_utils.py:21-27`.
2. The tool passes this raw `request_data` into
`ValidationPipeline.validate_request_with_warnings()` at
`superset/mcp_service/chart/validation/pipeline.py:90-103`, which in turn
calls
`SchemaValidator.validate_request()` at
`superset/mcp_service/chart/validation/schema_validator.py:5-27`.
3. During pre-validation, `_pre_validate_chart_type()` at
`schema_validator.py:38-52`
fetches `BoxPlotChartPlugin` and invokes its `pre_validate()` method at
`superset/mcp_service/chart/plugins/box_plot.py:44-75`, which only checks
for the presence
of `"metrics"` and `"distribute_across"` keys; because the request uses
`"columns"`
instead of `"distribute_across"`, `pre_validate()` adds the missing
`'distribute_across'`
field to `missing_fields` and returns a `ChartGenerationError` with error
code
`MISSING_BOX_PLOT_FIELDS`.
4. Even if plugin pre-validation were bypassed or updated, the Pydantic
layer still
rejects the same request: `GenerateChartRequest` at
`superset/mcp_service/chart/schemas.py:2151-2227` validates its `config`
field as
`ChartConfig`, whose `box_plot` variant is `BoxPlotChartConfig` at
`schemas.py:1892-76`;
`UnknownFieldCheckMixin.check_unknown_fields()` at `schemas.py:22-28`
computes known keys
from model fields (including validation aliases via `_get_known_fields()` at
`schemas.py:11-26`), and because `BoxPlotChartConfig` declares only
`distribute_across`
and `dimensions` (lines 1904-1925) with no `validation_alias` for
`"columns"` or
`"groupby"`, `_check_unknown_fields()` flags `columns`/`groupby` as unknown
and raises a
`ValueError`, causing valid Superset-style payloads to fail schema
validation.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=86b776d64bd64d378491d19a7ab0ba80&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=86b776d64bd64d378491d19a7ab0ba80&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:** 1904:1913
**Comment:**
*Api Mismatch: `box_plot` currently rejects Superset-native payload
keys (`columns`/`groupby`) because the schema only accepts
`distribute_across`/`dimensions` and the pre-validation path checks only those
names. This breaks compatibility when callers send existing form_data
vocabulary (which the chart request normalizer is intended to accept). Add
input aliases (and matching pre-validate support) so `columns` maps to
`distribute_across` and `groupby` maps to `dimensions`.
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=134c2808a8c3918c091d32b3a6f7b3e5a095e729c4aa12844b2a68838d9d0f01&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41860&comment_hash=134c2808a8c3918c091d32b3a6f7b3e5a095e729c4aa12844b2a68838d9d0f01&reaction=dislike'>👎</a>
##########
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
+ )
Review Comment:
**Suggestion:** The schema does not accept frontend `whiskerOptions` input,
so requests based on existing Superset box-plot form_data fail with
unknown-field errors even though this plugin emits/consumes that contract
downstream. Add an input compatibility path that accepts `whiskerOptions` and
converts it to `whisker_type` (+ percentiles when needed) before validation.
[api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Box-plot configs with whiskerOptions fail validation.
- ⚠️ Native Superset box_plot form_data cannot be reused.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Construct a `generate_chart` MCP request targeting a box plot where the
caller reuses
Superset-native form_data produced by existing charts (or by
`map_box_plot_config()`),
with `config` containing `"chart_type": "box_plot"`, `"metrics": [...]`,
`"distribute_across": [...]`, and `"whiskerOptions": "Tukey"` (or the
`<low>/<high>
percentiles` string), instead of the new schema fields `whisker_type`,
`percentile_low`,
and `percentile_high`; this mirrors the frontend contract documented in
`map_box_plot_config()` at `superset/mcp_service/chart/chart_utils.py:6-27`.
2. The request flows through
`ValidationPipeline.validate_request_with_warnings()` at
`superset/mcp_service/chart/validation/pipeline.py:90-103`, which calls
`SchemaValidator.validate_request()` at
`superset/mcp_service/chart/validation/schema_validator.py:5-27`;
`_pre_validate_chart_type()` invokes `BoxPlotChartPlugin.pre_validate()` at
`superset/mcp_service/chart/plugins/box_plot.py:44-75`, which checks only
for `"metrics"`
and `"distribute_across"`, so this step passes because those keys are
present and
`whiskerOptions` is ignored.
3. After pre-validation succeeds, `GenerateChartRequest` at
`superset/mcp_service/chart/schemas.py:2151-2227` validates `config` as
`ChartConfig`,
selecting `BoxPlotChartConfig` at `schemas.py:1892-76` based on
`chart_type="box_plot"`;
`UnknownFieldCheckMixin.check_unknown_fields()` at `schemas.py:22-28`
computes known keys
from `BoxPlotChartConfig`’s declared fields (`metrics`, `distribute_across`,
`dimensions`,
`whisker_type`, `percentile_low`, `percentile_high`, etc.) via
`_get_known_fields()` at
`schemas.py:11-26.
4. Because `BoxPlotChartConfig`’ whisker configuration only accepts
`whisker_type` plus
optional `percentile_low`/`percentile_high` (lines 1914-1924) and defines no
`validation_alias` or pre-normalization for `whiskerOptions`, the top-level
key
`"whiskerOptions"` is treated as unknown, `_check_unknown_fields()` raises a
`ValueError`
with an unknown-field message, and the box-plot request fails schema
validation instead of
being translated into `whisker_type` and percentiles as expected for
compatibility with
existing Superset form_data.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4bf81db688b145f1bc7bbf4448ed9a52&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=4bf81db688b145f1bc7bbf4448ed9a52&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:** 1914:1924
**Comment:**
*Api Mismatch: The schema does not accept frontend `whiskerOptions`
input, so requests based on existing Superset box-plot form_data fail with
unknown-field errors even though this plugin emits/consumes that contract
downstream. Add an input compatibility path that accepts `whiskerOptions` and
converts it to `whisker_type` (+ percentiles when needed) before validation.
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=7ae400abe9f0f8b507cda14df2f022affbb106cad77963e93e57fb03dd307c62&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41860&comment_hash=7ae400abe9f0f8b507cda14df2f022affbb106cad77963e93e57fb03dd307c62&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]
