codeant-ai-for-open-source[bot] commented on code in PR #41860:
URL: https://github.com/apache/superset/pull/41860#discussion_r3565457021
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1846,6 +1846,108 @@ 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}]")
+ 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)
+ number_format: str = Field("SMART_NUMBER", max_length=50)
+ date_format: str = Field("smart_date", max_length=50)
+
+ @model_validator(mode="after")
+ def validate_percentiles_and_dimensions(self) -> "BoxPlotChartConfig":
+ if self.whisker_type == "percentile":
+ if self.percentile_low is None or self.percentile_high is None:
+ raise ValueError(
+ "whisker_type='percentile' requires both percentile_low "
+ "and percentile_high"
+ )
+ if self.percentile_low >= self.percentile_high:
+ raise ValueError("percentile_low must be less than
percentile_high")
+ elif self.percentile_low is not None or self.percentile_high is not
None:
+ raise ValueError(
+ "percentile_low/percentile_high only apply when "
+ "whisker_type='percentile'"
+ )
+ for i, col in enumerate(self.distribute_across):
+ _reject_sql_expression_on_dimension(col, f"distribute_across[{i}]")
+ for i, col in enumerate(self.dimensions or []):
+ _reject_sql_expression_on_dimension(col, f"dimensions[{i}]")
Review Comment:
**Suggestion:** `dimensions` in box plot config is also a dimension-only
field but currently permits `saved_metric=True`. Those values are mapped into
`form_data["groupby"]`, where saved metrics are not valid and can trigger
runtime query errors. Add a validation check to reject `saved_metric=True` in
`dimensions` entries. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Box plot dimension metrics break query group-by generation.
- ⚠️ Box plot MCP results fail for misconfigured series dimensions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use the MCP `generate_chart` tool in
`superset/mcp_service/chart/tool/generate_chart.py:89-101` to submit a
box-plot request
whose `config` includes `{"chart_type": "box_plot", "metrics": [{"name":
"fare",
"aggregate": "AVG"}], "distribute_across": [{"name": "day_of_week"}],
"dimensions":
[{"name": "fare_metric", "saved_metric": true}]}` as permitted by the
box-plot docstring
at lines 141-143.
2. The `ChartConfig` union in `superset/mcp_service/chart/schemas.py:93-112`
dispatches
this config to `BoxPlotChartConfig`; the
`validate_percentiles_and_dimensions` validator
at lines 71-90 iterates over `self.dimensions` and calls
`_reject_sql_expression_on_dimension(col, f"dimensions[{i}]")` (lines 88-89)
but does not
check `col.saved_metric`, so a `dimensions` entry with `saved_metric=True`
passes schema
validation.
3. `generate_chart` calls `map_config_to_form_data` in
`superset/mcp_service/chart/chart_utils.py:368-415`, which uses the
`BoxPlotChartPlugin`
(registered in `plugins/__init__.py:50-52`) and its `to_form_data`
implementation in
`superset/mcp_service/chart/plugins/box_plot.py:89-92` to convert the config
via
`map_box_plot_config`.
4. `map_box_plot_config` in `chart_utils.py:925-951` builds `form_data` with
`"groupby":
[d.name for d in (config.dimensions or [])]`, so the saved metric name from
the
`dimensions` field is emitted into `form_data["groupby"]` even though
group-by fields in
Superset’s box-plot query path are dimension columns; when `_compile_chart`
runs in
`generate_chart.py:443-449` or `generate_chart.py:585-589`, the query
builder treats these
names as columns, and if they do not match real columns the compile step
fails and
`generate_chart` returns a `CHART_COMPILE_FAILED` style error instead of a
valid box plot.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fba29f039637410d96ce24482780b49d&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=fba29f039637410d96ce24482780b49d&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:** 1946:1947
**Comment:**
*Api Mismatch: `dimensions` in box plot config is also a dimension-only
field but currently permits `saved_metric=True`. Those values are mapped into
`form_data["groupby"]`, where saved metrics are not valid and can trigger
runtime query errors. Add a validation check to reject `saved_metric=True` in
`dimensions` entries.
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=385778d3fd428612b8e2a0084eb3a06b3ff596ca761d2aaa208cc797f1b329da&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41860&comment_hash=385778d3fd428612b8e2a0084eb3a06b3ff596ca761d2aaa208cc797f1b329da&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1846,6 +1846,108 @@ 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}]")
Review Comment:
**Suggestion:** `groupby` entries in histogram config currently only reject
`sql_expression` but still allow `saved_metric=True`. That lets saved metrics
flow into `form_data["groupby"]` as if they were dimension columns, which will
break query generation at runtime because group-by fields must be real columns.
Add a schema-level check rejecting `saved_metric=True` for each `groupby` item.
[api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ generate_chart histogram requests with metric groupby fail compile check.
- ⚠️ Histogram MCP previews fail for misconfigured groupby dimensions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the MCP `generate_chart` tool defined in
`superset/mcp_service/chart/tool/generate_chart.py:89-101` with a
`GenerateChartRequest`
whose `config` payload includes `{"chart_type": "histogram", "column":
{"name":
"trip_duration"}, "groupby": [{"name": "fare_avg", "saved_metric": true}]}`
as allowed by
the docstring’s histogram section at lines 137-140.
2. The request config is validated via the `ChartConfig` discriminated union
in
`superset/mcp_service/chart/schemas.py:93-112`, which dispatches to
`HistogramChartConfig`; its `reject_metric_style_column` validator at lines
15-26 only
rejects `sql_expression` and `saved_metric` on `self.column` and calls
`_reject_sql_expression_on_dimension` for each `groupby` entry, but never
checks
`col.saved_metric`, so a `groupby` entry with `saved_metric=True` is
accepted.
3. `generate_chart` then calls `map_config_to_form_data` in
`superset/mcp_service/chart/chart_utils.py:368-415`, which looks up the
`HistogramChartPlugin` registered in
`superset/mcp_service/chart/plugins/__init__.py:50-52`;
`HistogramChartPlugin.to_form_data` in `plugins/histogram.py:75-78`
delegates to
`map_histogram_config`.
4. `map_histogram_config` in `chart_utils.py:896-913` builds `form_data`
with `groupby:
[g.name for g in (config.groupby or [])]`, so the saved metric name (for
example
`"fare_avg"`) is emitted into `form_data["groupby"]` as if it were a
physical column; when
`_compile_chart` is invoked in `generate_chart.py:443-449` or
`generate_chart.py:585-589`,
Superset’s query builder treats these group-by entries as real dataset
columns, leading to
a compile failure (e.g. column-not-found) and a `CHART_COMPILE_FAILED` error
instead of a
valid histogram.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7f77d9eabef643f7836150c24054a7b5&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=7f77d9eabef643f7836150c24054a7b5&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:** 1882:1883
**Comment:**
*Api Mismatch: `groupby` entries in histogram config currently only
reject `sql_expression` but still allow `saved_metric=True`. That lets saved
metrics flow into `form_data["groupby"]` as if they were dimension columns,
which will break query generation at runtime because group-by fields must be
real columns. Add a schema-level check rejecting `saved_metric=True` for each
`groupby` item.
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=16d858d1e4a902de3162967a72eec796e8e48b540e8baed08639288e99d92484&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41860&comment_hash=16d858d1e4a902de3162967a72eec796e8e48b540e8baed08639288e99d92484&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1846,6 +1846,108 @@ 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}]")
+ 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)
+ number_format: str = Field("SMART_NUMBER", max_length=50)
+ date_format: str = Field("smart_date", max_length=50)
+
+ @model_validator(mode="after")
+ def validate_percentiles_and_dimensions(self) -> "BoxPlotChartConfig":
+ if self.whisker_type == "percentile":
+ if self.percentile_low is None or self.percentile_high is None:
+ raise ValueError(
+ "whisker_type='percentile' requires both percentile_low "
+ "and percentile_high"
+ )
+ if self.percentile_low >= self.percentile_high:
+ raise ValueError("percentile_low must be less than
percentile_high")
+ elif self.percentile_low is not None or self.percentile_high is not
None:
+ raise ValueError(
+ "percentile_low/percentile_high only apply when "
+ "whisker_type='percentile'"
+ )
+ for i, col in enumerate(self.distribute_across):
+ _reject_sql_expression_on_dimension(col, f"distribute_across[{i}]")
Review Comment:
**Suggestion:** `distribute_across` in box plot config is a dimension axis
but the validator only blocks `sql_expression`, not `saved_metric=True`. This
allows saved metrics to be emitted in `form_data["columns"]`, which are
expected to be real dataset columns and can fail during query compilation.
Reject `saved_metric=True` for `distribute_across` items at schema validation
time. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Box plot distribute_across metrics cause compile_error in generate_chart.
- ⚠️ Box plot previews fail for misconfigured distribute_across dimensions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the MCP `generate_chart` tool in
`superset/mcp_service/chart/tool/generate_chart.py:89-101` with a
`GenerateChartRequest`
whose `config` contains a box-plot definition such as `{"chart_type":
"box_plot",
"metrics": [{"name": "fare", "aggregate": "AVG"}], "distribute_across":
[{"name":
"fare_metric", "saved_metric": true}]}` as described in the box-plot section
of the
docstring at lines 141-143.
2. The request is validated by the `ChartConfig` union in
`superset/mcp_service/chart/schemas.py:93-112`, which dispatches to
`BoxPlotChartConfig`;
its `validate_percentiles_and_dimensions` validator at lines 71-90 only calls
`_reject_sql_expression_on_dimension` for each `distribute_across` entry
(line 86) and
does not check `col.saved_metric`, so a `distribute_across` item with
`saved_metric=True`
passes schema validation.
3. `generate_chart` then invokes `map_config_to_form_data` in
`superset/mcp_service/chart/chart_utils.py:368-415`, which retrieves the
`BoxPlotChartPlugin` from the registry (registered in
`superset/mcp_service/chart/plugins/__init__.py:50-52`) and calls
`BoxPlotChartPlugin.to_form_data` in `plugins/box_plot.py:89-92`, delegating
to
`map_box_plot_config`.
4. `map_box_plot_config` in `chart_utils.py:925-951` builds `form_data` with
`"columns":
[c.name for c in config.distribute_across]` and `"metrics":
[create_metric_object(m) for m
in config.metrics]`; because the `distribute_across` entry carries
`saved_metric=True`,
its name (e.g. `"fare_metric"`) is emitted into `form_data["columns"]` even
though
Superset’s box-plot query builder expects physical dataset columns there, so
when
`_compile_chart` is executed in `generate_chart.py:443-449` or
`generate_chart.py:585-589`, the query compile fails due to an invalid or
missing column
and `generate_chart` returns a compile-error response instead of a working
box plot.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=41f9cd09354447c29ca0a6781f04a9d1&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=41f9cd09354447c29ca0a6781f04a9d1&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:** 1944:1945
**Comment:**
*Api Mismatch: `distribute_across` in box plot config is a dimension
axis but the validator only blocks `sql_expression`, not `saved_metric=True`.
This allows saved metrics to be emitted in `form_data["columns"]`, which are
expected to be real dataset columns and can fail during query compilation.
Reject `saved_metric=True` for `distribute_across` items at schema validation
time.
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=6a27dbc783f5611fe50e4ec7010ad4a519272962de43bbe64a960eed033dc8b8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41860&comment_hash=6a27dbc783f5611fe50e4ec7010ad4a519272962de43bbe64a960eed033dc8b8&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]
