gabotorresruiz commented on code in PR #43572:
URL: https://github.com/apache/superset/pull/43572#discussion_r3961024147
##########
superset/mcp_service/chart/tool/get_chart_type_schema.py:
##########
@@ -291,7 +302,7 @@ def get_chart_type_schema(
for a chart configuration before calling generate_chart or update_chart.
Valid chart_type values depend on the host deployment. Core types are xy,
Review Comment:
Follow-through on this docstring fix, not a blocker: the other two
LLM-facing enumerations still end at waterfall. The `generate_chart`
docstring's chart_type list and quick lookup have no bubble entry (so a client
asked for a bubble chart is steered to chart_type='xy', kind='scatter'), and
`app.py`'s "Chart Types You Can CREATE" and registry list omit bubble_v2 even
though `display_name_for_viz_type("bubble_v2")` now resolves. Your waterfall PR
(#42070) updated all three, which is what made me look.
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -1045,6 +1045,71 @@ def reject_sql_expression_on_dimensions(self) ->
"PieChartConfig":
return self
+class BubbleChartConfig(BaseChartConfig):
+ """Config for bubble charts (viz_type ``bubble_v2``).
+
+ Matches the frontend Bubble buildQuery contract: an ``entity`` dimension
+ identifies each bubble, three separate metrics position and size it
+ (``x``, ``y``, ``size``), and an optional ``series`` dimension colours the
+ bubbles by group.
+ """
+
+ model_config = ConfigDict(extra="ignore", populate_by_name=True)
+
+ chart_type: Literal["bubble_v2"] = "bubble_v2"
+ entity: ColumnRef = Field(
+ ...,
+ description="Category column identifying each bubble (e.g. country)",
+ )
+ x: ColumnRef = Field(
+ ...,
+ description="Metric for the bubble's horizontal position (use "
+ "aggregate e.g. AVG, or saved_metric=True for a saved metric)",
+ )
+ y: ColumnRef = Field(
Review Comment:
This block worries me a bit: BubbleChartConfig is the first config in the
union whose `y` is a single ColumnRef instead of a list, and the shared
analyzers duck-type on `config.y` assuming a list. `analyze_chart_capabilities`
does `len(config.y)` (chart_utils.py:1734), which raises `TypeError: object of
type 'ColumnRef' has no len()`, and `analyze_chart_semantics` iterates it
(chart_utils.py:1800), yielding `(field, value)` tuples so `col.name` raises
AttributeError. `generate_chart` calls both unguarded and its outer except
doesn't catch TypeError; with `save_chart=True` the chart row is created before
the crash, so the user also gets an orphaned saved chart. I verified the crash
at this head with the PR description's own example config through the real tool
harness.
The fix is small; I applied exactly this locally and the full flow completes:
```python
if hasattr(config, "y") and config.y:
y_refs = config.y if isinstance(config.y, list) else [config.y]
data_types.extend(["metric"] * len(y_refs))
```
(same normalization in `analyze_chart_semantics`). A
TestAnalyzeChartCapabilitiesBubble / TestAnalyzeChartSemanticsBubble pair
mirroring the big_number ones would lock it in; the current tests cover schema,
mapper, and registry but never the generate_chart metadata path.
--
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]
