gkneighb commented on PR #43572:
URL: https://github.com/apache/superset/pull/43572#issuecomment-5633740990
@aminghadersohi Thanks for the re-check at `19a14c2719` — every item you
listed was real at that head. All of them are addressed at `13a0590c` except
one I'd like to argue belongs in its own PR. Evidence below is from running the
code at this head, not reading it.
## Creation failure points — both fixed
**Compile/preview extraction.** Correct at your SHA: `compile.py` built its
query inline (`metrics = form_data.get("metrics", [])`), which is empty for
bubble because the metrics live under `x`/`y`/`size`. Current master routes
both `compile.py:136` and `preview_utils.py:92` through
`build_query_context_from_form_data`, which calls the corrected
`resolve_metrics`. Rebasing onto master is what fixed this, so I can't claim
the credit — but it is fixed, and the query the compile check now builds is:
```
metrics: ['AVG(gdp)', 'AVG(life_expectancy)', 'SUM(population)']
columns: ['country', 'continent']
```
**`TypeError: object of type 'ColumnRef' has no len()`.** Fixed, and thank
you for catching that save mode had already called `CreateChartCommand.run()` —
the orphaned-chart risk was the worst part of it. Bubble is the first config
whose `y` is a single column; `_as_column_list()` in `chart_utils.py` now
normalizes at both analyzer sites (`analyze_chart_semantics` had the matching
`AttributeError` via the pydantic `__iter__`). Tests were written to fail first
on both exceptions.
## CI red — fixed
`_VIZ_CATEGORY.get("bubble_v2")` returned `None` because the entry was lost
when I re-applied this branch onto master as a single clean commit; it is back,
so `test_bubble_in_recommendation_category_map` passes. Fresh CI is running on
this head.
## Validation of the implicit aggregate — fixed
This was the sharpest finding. `create_metric_object` defaults a bare column
to `SUM`, but the ref reported `is_metric` False, so `_validate_aggregations`
skipped it — hence `SUM(country)` on a VARCHAR reaching the database while the
explicit spelling was rejected. `BubbleChartConfig` now records the default it
is going to get:
```
x={"name": "country"} -> aggregate: SUM | is_metric: True
```
so the existing aggregate-compatibility check sees it and returns
`invalid_aggregation`. Numeric columns are unaffected. Tests cover all three
metric slots plus a guard that `entity` stays aggregate-free.
## Fidelity
**Ordering — fixed.** Bubble's `buildQuery` pairs `form_data['orderby']`
with `!order_desc` (the control is a single `dndSortByControl` metric,
`order_desc` defaults true). The MCP builder never read a top-level `orderby`,
so `row_limit` truncated an unordered result. `_build_single_query_dict` now
resolves it for the bubble viz types, with `sort_by_metric` still winning for
the pie family:
```
saved orderby=[SUM(population)], row_limit=1 -> orderby [('SUM(population)',
False)]
```
**Vega-Lite — fixed.** The generic spec builder reads
`x_axis`/`metrics`/`groupby`, so bubble produced a `bar` mark with no
`encoding` key at all. Bubble now has its own spec:
```
mark: circle
encodings: {'x': 'AVG(gdp)', 'y': 'AVG(life_expectancy)', 'size':
'SUM(population)', 'color': 'continent'}
```
with the series coloring the bubbles, falling back to the entity when no
series is set, plus tooltips. Saved metrics (bare name strings) resolve as
fields too.
**Update builders — deferred, deliberately.** You're right that preview
retains existing series/axis options/filters while immediate-save replaces
them, and that an omitted color scheme or row limit resets in both. As you
noted, this one doesn't originate in this diff: it's `update_chart`'s
replace-vs-merge semantics, which is the same for all ten registered types.
Changing it here would silently alter behavior for pie, gauge, treemap and the
rest under cover of a plugin PR, and "omitted field means keep existing" vs
"means reset to default" is a product decision worth its own discussion. Happy
to open a tracking issue for it — or to fold it in if you'd rather it land with
this.
## Discovery docs — fixed
`generate_chart`'s docstring (one-of list, per-type entry, quick-lookup
alias) and `app.py` (creation bullet + registry-known list) both carry bubble
now, and the PR description's example is corrected from `chart_type="bubble"`
to `bubble_v2`.
The test file is 37 tests; the branch is rebased onto current master and
mergeable again.
--
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]
