bito-code-review[bot] commented on code in PR #43570:
URL: https://github.com/apache/superset/pull/43570#discussion_r4130314914
##########
superset/common/form_data_query_context.py:
##########
@@ -130,6 +130,23 @@ def freeform_where_having(form_data: dict[str, Any]) ->
dict[str, str]:
return extras
+def _as_column_list(value: Any) -> list[Any]:
+ """
+ Normalize a ``groupby``/``columns`` value into a list.
+
+ Single-select controls (e.g. the heatmap ``groupby`` Y axis, which is
+ ``multi: false``, and heatmap charts migrated via ``MigrateHeatmapChart``)
+ store the dimension as a bare string. Wrap a scalar in a one-element list,
+ mirroring ``chart_helpers.resolve_groupby``, so downstream list operations
+ (``.copy()``, ``.insert()``) do not blow up on a ``str``.
+ """
+ if value is None:
+ return []
+ if isinstance(value, str):
+ return [value]
+ return list(value)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated column-list helper</b></div>
<div id="fix">
This new helper duplicates `_as_column_list` in
`superset/mcp_service/chart/chart_utils.py:2416` (same name, same purpose) with
divergent edge semantics: that version wraps any non-list scalar, while this
one calls `list(value)`, which raises `TypeError` on a non-iterable scalar.
`as_list` from `superset.utils.core` is already imported in this module.
Consolidate in a shared util to prevent divergence. ([CWE not applicable])
</div>
</div>
<details>
<summary><b>Citations</b></summary>
<ul>
<li>
Rule Violated: <a
href="https://github.com/apache/superset/blob/1147bf7/.cursor/rules/dev-standard.mdc#L108";>dev-standard.mdc:108</a>
</li>
</ul>
</details>
<small><i>Code Review Run #c21aa0</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/chart/plugins/heatmap.py:
##########
@@ -0,0 +1,150 @@
+# 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.
+
+"""Heatmap 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 (
+ _heatmap_chart_what,
+ _summarize_filters,
+ map_heatmap_config,
+)
+from superset.mcp_service.chart.plugin import BaseChartPlugin
+from superset.mcp_service.chart.schemas import ColumnRef, HeatmapChartConfig
+from superset.mcp_service.chart.validation.dataset_validator import
DatasetValidator
+from superset.mcp_service.common.error_schemas import ChartGenerationError
+
+
+class HeatmapChartPlugin(BaseChartPlugin):
+ """Plugin for heatmap chart type."""
+
+ chart_type = "heatmap_v2"
+ display_name = "Heatmap"
+ native_viz_types: ClassVar[Mapping[str, str]] = {
+ "heatmap_v2": "Heatmap",
+ }
+
+ def pre_validate(
+ self,
+ config: dict[str, Any],
+ ) -> ChartGenerationError | None:
+ missing_fields = []
+
+ if "x_axis" not in config:
+ missing_fields.append("'x_axis' (column along the X axis)")
+ if "y_axis" not in config and "groupby" not in config:
+ missing_fields.append("'y_axis' (column along the Y axis)")
+ if "metric" not in config:
+ missing_fields.append("'metric' (value colouring each cell)")
+
+ if missing_fields:
+ return ChartGenerationError(
+ error_type="missing_heatmap_fields",
+ message=(
+ f"Heatmap chart missing required fields: "
+ f"{', '.join(missing_fields)}"
+ ),
+ details=(
+ "Heatmaps plot a metric across two dimensions — one on the
"
+ "x_axis and one on the y_axis — colouring each cell by the
"
+ "metric value"
+ ),
+ suggestions=[
+ "Add 'x_axis': {'name': 'day_of_week'}",
+ "Add 'y_axis': {'name': 'hour'}",
+ "Add 'metric': {'name': 'trips', 'aggregate': 'COUNT'}",
+ "Example: {'chart_type': 'heatmap_v2', "
+ "'x_axis': {'name': 'day_of_week'}, "
+ "'y_axis': {'name': 'hour'}, "
+ "'metric': {'name': 'trips', 'aggregate': 'COUNT'}}",
+ ],
+ error_code="MISSING_HEATMAP_FIELDS",
+ )
+
+ return None
+
+ def extract_column_refs(self, config: Any) -> list[ColumnRef]:
+ if not isinstance(config, HeatmapChartConfig):
+ return []
+ refs: list[ColumnRef] = [config.x_axis, config.y_axis, config.metric]
+ 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_heatmap_config(config)
+
+ def generate_name(self, config: Any, dataset_name: str | None = None) ->
str:
+ what = _heatmap_chart_what(config)
+ context = _summarize_filters(config.filters)
+ return self._with_context(what, context)
+
+ def resolve_viz_type(self, config: Any) -> str:
+ return "heatmap_v2"
+
+ def normalize_column_refs(self, config: Any, dataset_context: Any) -> Any:
+ config_dict = config.model_dump()
+
+ for key in ("x_axis", "y_axis"):
+ col = config_dict.get(key)
+ if col and not col.get("sql_expression") and not
col.get("saved_metric"):
+ col["name"] = DatasetValidator.get_canonical_column_name(
+ col["name"], dataset_context
+ )
+ if config_dict.get("metric"):
+ if config_dict["metric"].get("sql_expression"):
+ pass
+ elif config_dict["metric"].get("saved_metric"):
+ config_dict["metric"]["name"] = (
+ DatasetValidator.get_canonical_metric_name(
+ config_dict["metric"]["name"], dataset_context
+ )
+ )
+ else:
+ config_dict["metric"]["name"] = (
+ DatasetValidator.get_canonical_column_name(
+ config_dict["metric"]["name"], dataset_context
+ )
+ )
+ DatasetValidator.normalize_filters(config_dict, dataset_context)
+ return HeatmapChartConfig.model_validate(config_dict)
+
+ def schema_error_hint(self) -> ChartGenerationError | None:
+ return ChartGenerationError(
+ error_type="heatmap_validation_error",
+ message="Heatmap chart configuration validation failed",
+ details=(
+ "The heatmap chart configuration is missing required "
+ "fields or has invalid structure"
+ ),
+ suggestions=[
+ "Ensure 'x_axis' and 'y_axis' each have a 'name'",
+ "Ensure 'metric' field has 'name' and 'aggregate'",
+ "Example: {'chart_type': 'heatmap_v2', "
+ "'x_axis': {'name': 'day_of_week'}, "
+ "'y_axis': {'name': 'hour'}, "
+ "'metric': {'name': 'trips', 'aggregate': 'COUNT'}}",
+ ],
+ error_code="HEATMAP_VALIDATION_ERROR",
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing method docstrings (BITO 12147)</b></div>
<div id="fix">
None of the seven overridden methods carries an inline docstring; their
contracts live only on `BaseChartPlugin`/`ChartTypePlugin`. BITO adaptive rule
12147 requires a docstring on every newly introduced function. Brief per-method
docstrings (even one line noting heatmap-specific behavior, e.g. the
y_axis/groupby alias handling in `pre_validate`) keep this file self-describing.
</div>
</div>
<small><i>Code Review Run #15921f</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>Duplicated metric normalization</b></div>
<div id="fix">
The metric-normalization block (`sql_expression` pass, `saved_metric` ->
`get_canonical_metric_name`, else `get_canonical_column_name`) is copied
verbatim from `PieChartPlugin` and `TreemapChartPlugin` (and near-verbatim
`GaugeChartPlugin`). A future fix to metric normalization must be applied in
four files; extracting one shared helper removes that divergence risk.
</div>
</div>
<small><i>Code Review Run #c21aa0</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
