aminghadersohi commented on PR #43572:
URL: https://github.com/apache/superset/pull/43572#issuecomment-5635371012
Rechecked **13a0590cf6c8**: all **37 Bubble tests pass**. Ordinary scalar-y
generation, compile extraction, implicit-SUM validation, recommendation
mapping, ordering, and discovery fixes check out. Two product-path gaps remain:
1. **Valid SQL-expression x still crashes after saving.** Replace the
example's x with `{"sql_expression":"AVG(gdp)","label":"GDP"}`. Schema/dataset
validation accepts it, but
[analyze_chart_semantics](https://github.com/apache/superset/blob/13a0590cf6c8ad11983ab32f46fac3a9034fa49b/superset/mcp_service/chart/chart_utils.py#L2065-L2082)
appends `config.x.name` (None), then joins it: `TypeError: sequence item 0:
expected str instance, NoneType found`. The actual generate_chart body
reproduces this in preview and save modes with successful compilation/mocked
persistence; in save mode the create command has already run. Please apply the
label/expression fallback to x too and cover this through generation, not just
the ordinary-column analyzer case.
2. **The new Bubble renderer is bypassed by get_chart_preview.**
[VegaLitePreviewStrategy.generate](https://github.com/apache/superset/blob/13a0590cf6c8ad11983ab32f46fac3a9034fa49b/superset/mcp_service/chart/tool/get_chart_preview.py#L472-L490)
only special-cases Gauge, then takes the generic fallback. For result columns
country, continent, AVG(gdp), AVG(life_expectancy), SUM(population), I get
x=country, y=continent and fixed-size circles, without metric size/color
encodings. The direct form-data preview correctly uses the three metrics. Route
this strategy through the Bubble renderer too; it serves saved charts and
cached/transient charts, including saved generate_chart previews.
**CI clarification:** [the unit
job](https://github.com/apache/superset/actions/runs/34593773639/job/103244873666)
completed its main suite with **15164 passed, 5 skipped, 2 xfailed**, then
exceeded the **30-minute job limit during the separate SQL coverage run**. The
required check is red because of cancellation, not the old Bubble assertion.
On the deferred update issue: the rebased head already uses shared merging
in both preview/save builders and preserves series/options/filters in my probe.
Omitted color/row-limit defaults still reset; I agree broader omission/reset
semantics should be tracked separately, not require a cross-plugin refactor
here.
Checks used isolated code/unit harnesses with database boundaries mocked,
not a live database/browser.
--
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]
