gabotorresruiz commented on code in PR #43570:
URL: https://github.com/apache/superset/pull/43570#discussion_r3961103175
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1121,6 +1121,61 @@ def reject_inverted_bounds(self) -> "GaugeChartConfig":
return self
+class HeatmapChartConfig(BaseChartConfig):
Review Comment:
Just a question, not a blocker: any reason to leave out `time_grain`? The
heatmap control panel's Query section includes `time_grain_sqla`, and the
waterfall config exposes `time_grain` with the `granularity_sqla` mirroring.
The PR text frames the deferred fields as cosmetic, but time grain is part of
the query contract; a heatmap with a temporal x_axis (say month vs region)
cannot be bucketed without it. Fine as a follow-up if intentional.
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1067,6 +1068,26 @@ def map_gauge_config(config: GaugeChartConfig) ->
Dict[str, Any]:
return form_data
+def map_heatmap_config(config: HeatmapChartConfig) -> Dict[str, Any]:
+ """Map heatmap config to Superset form_data (viz_type ``heatmap_v2``).
+
+ Matches the frontend Heatmap buildQuery contract: an ``x_axis`` column and
+ a single ``groupby`` Y column form the two axes, one ``metric`` colours
+ the cells, and ``normalize_across`` selects the rank-normalization range.
+ The Y axis is a single-select ``groupby`` (not a list).
+ """
+ form_data: Dict[str, Any] = {
+ "viz_type": "heatmap_v2",
+ "x_axis": config.x_axis.name,
+ "groupby": config.y_axis.name,
Review Comment:
Hey Greg, one more query-path issue, and I am afraid this one is a blocker:
`generate_chart` still cannot produce a heatmap on this branch. It is a
different builder than the one fixed for the GROUP BY fold.
The compile check that runs on every `generate_chart` call (both the save
path and the preview-only path) builds its columns through
`preview_utils._build_query_columns`, which delegates to
`columns_from_form_data` (`superset/common/form_data_query_context.py:151`).
That helper calls `.copy()` on `form_data["groupby"]`, and this mapper emits it
as a bare string, so it raises `AttributeError: 'str' object has no attribute
'copy'`. Neither `_compile_chart`'s except clauses nor the tool's outer handler
catch `AttributeError`, so the whole tool call blows up.
I verified it at this head (`e8b270167a`), in a real app context:
```python
fd = map_heatmap_config(HeatmapChartConfig(
chart_type="heatmap_v2",
x_axis={"name": "day_of_week"},
y_axis={"name": "hour"},
metric={"name": "trips", "aggregate": "COUNT"},
))
_compile_chart(fd, 1)
# AttributeError: 'str' object has no attribute 'copy'
```
The same call with a waterfall config returns a structured `CompileResult`
instead of raising.
The scalar itself is the faithful shape (the `groupby` control is `multi:
false`, and `MigrateHeatmapChart` renames the scalar `all_columns_y` straight
to `groupby`), so I would fix the shared helper rather than this mapper: coerce
a string `groupby` into a one-element list inside `columns_from_form_data`,
mirroring what `chart_helpers.resolve_groupby` already does and what your GROUP
BY fold fix effectively assumes. That also fixes the same latent crash for
migrated heatmap charts in the dashboard Excel export path, which reaches this
helper via `_columns_and_metrics`.
For tests: one case in
`tests/unit_tests/common/test_form_data_query_context.py` with `{"x_axis":
"day", "groupby": "hour"}` expecting `["day", "hour"]`, plus a sibling of your
`test_x_axis_reaches_group_by` that goes through `columns_from_form_data`
instead of `chart_helpers`, since the two builders do not share code. That
split is exactly why the suite stays green with this crash present. Happy to
dig in with you if it does not reproduce on your side.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1121,6 +1121,61 @@ def reject_inverted_bounds(self) -> "GaugeChartConfig":
return self
+class HeatmapChartConfig(BaseChartConfig):
+ """Config for heatmap charts (viz_type ``heatmap_v2``).
+
+ Matches the frontend Heatmap buildQuery contract: an ``x_axis`` column, a
+ single ``groupby`` column for the Y axis, and one ``metric`` colouring each
+ cell. ``normalize_across`` drives the server-side rank normalization
+ (whole heatmap, per-x, or per-y).
+ """
+
+ model_config = ConfigDict(extra="ignore", populate_by_name=True)
+
+ chart_type: Literal["heatmap_v2"] = "heatmap_v2"
+ x_axis: ColumnRef = Field(
+ ...,
+ description="Column along the X axis",
+ )
+ y_axis: ColumnRef = Field(
+ ...,
+ description="Column along the Y axis (form_data 'groupby';
single-select)",
+ validation_alias=AliasChoices("y_axis", "groupby"),
+ )
+ metric: ColumnRef = Field(
+ ...,
+ description="Value metric colouring each cell (use aggregate e.g. SUM,
"
+ "COUNT for ad-hoc, or set saved_metric=True for a saved dataset
metric)",
+ )
+ normalize_across: Literal["heatmap", "x", "y"] = Field(
Review Comment:
Building on the `normalize_across` post-processing gap already agreed as a
follow-up: there is a second half that is cheap to fix now. Even on the path
where the frontend `buildQuery` does run (a saved chart rendered in Explore or
a dashboard), the rank column is computed but never used for color, because
`transformProps.ts` has `colorColumn = normalized ? RANK_COLUMN_NAME :
metricLabel` and the `normalized` checkbox defaults to false and is not exposed
here. So today a caller setting `normalize_across` sees no visual difference
anywhere.
Exposing `normalized: bool = False` in this config and passing it through
the mapper makes the knob real on the frontend path immediately, independent of
the heavier server-side work. A mapping test asserting
`form_data["normalized"]` would lock it in, and this field's description should
mention it only takes effect with `normalized=true`.
--
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]
