gkneighb commented on PR #43572:
URL: https://github.com/apache/superset/pull/43572#issuecomment-5640016243
@aminghadersohi Both fixed in `c5439587` (a new commit on top of `13a0590c`,
so the delta is readable). Thanks also for chasing the CI job down to the
30-minute limit in the SQL coverage step — that matches what I saw locally, and
it's the same cancelled-leg-fails-the-gate pattern that showed up on the
treemap PR.
## 1. SQL-expression `x` crashing after save
Confirmed exactly as you described, including the ordering: the analyzers
run while the response is assembled, so in save mode `CreateChartCommand.run()`
has already committed. `analyze_chart_semantics` appended `config.x.name`
unguarded — `None` for a SQL-expression ref — and then joined it:
```
TypeError: sequence item 0: expected str instance, NoneType found
```
The `y` side already had the label/expression fallback in its comprehension;
`x` did not, which is the asymmetry you found. Both sides now share it:
```python
columns.append(config.x.name or config.x.label or config.x.sql_expression)
```
With `x={"sql_expression": "AVG(gdp)", "label": "GDP per capita"}` the story
now reads `This bubble_v2 chart analyzes GDP per capita, life_expectancy`.
Covered at both levels as you asked:
`test_semantics_name_a_sql_expression_metric` at the analyzer, and
`TestGenerateBubbleWithSqlExpressionMetric` driving the real `generate_chart`
body in save mode with mocked persistence, asserting the chart is reported and
`CreateChartCommand.run()` was called once. I extended the existing
`_generate_saved_chart` helper with an optional `config` argument rather than
duplicating its mock stack.
One note: a `sql_expression` ref without a label is rejected by `ColumnRef`
validation ("requires a 'label'"), so there is no unlabeled variant to cover —
I wrote that test, watched it fail on the validator, and dropped it.
## 2. `get_chart_preview` bypassing the bubble renderer
Right again, and the mechanism is slightly worse than the generic-fallback
description: `VegaLitePreviewStrategy` doesn't call
`_generate_vega_lite_preview_from_data` at all, it calls its own
`_create_vega_lite_spec(chart_data)`, which never sees form_data — hence
x=country, y=continent and fixed-size circles regardless of chart type. Gauge
was the only special case.
The strategy now routes the bubble viz types through
`generate_bubble_vega_lite_preview` too, with the shared `BUBBLE_VIZ_TYPES` set
exported from `preview_utils` so the two dispatch sites can't drift. I placed
the branch **after** the existing no-data guard, so an empty result still
returns `NoDataError` rather than a circle spec with no encodings.
Driving the strategy with your result columns:
```
mark: circle
encodings: {'x': 'AVG(gdp)', 'y': 'AVG(life_expectancy)', 'size':
'SUM(population)', 'color': 'continent'}
tooltip: country, AVG(gdp), AVG(life_expectancy), SUM(population),
continent
```
Test: `test_saved_bubble_vega_preview_encodes_its_three_metrics`, mirroring
the gauge strategy tests.
## Verification caveat
My local environment can't run the two tool-level test files (the conda env
is behind on SQLAlchemy/Flask, so the app-init conftest fails and a
`--noconftest` run trips over duplicate model registration). What I verified
locally: the analyzer unit tests, and the bubble renderer driven with the exact
saved-chart form_data and result rows the strategy passes it — output above.
The two tool-level tests are construction-by-template on the gauge equivalents;
CI is the real check on them, so please do flag it if either misbehaves at this
head.
## On the update semantics
Thanks for re-probing that — agreed on all counts, and good to know the
rebased head already preserves series/options/filters through the shared merge.
I'll open a separate issue for the omitted-field reset semantics (color scheme
and row limit defaulting rather than being left alone), since that decision
spans every registered type.
--
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]
