diffray-bot commented on PR #36890:
URL: https://github.com/apache/superset/pull/36890#issuecomment-3706030467
## Changes Summary
This PR fixes a critical bug in the deck.gl Scatterplot chart where fixed
point size values were incorrectly treated as metric names, causing query
errors. The fix updates the query builder and transformation layer to properly
distinguish between fixed-value and metric-based radius modes, with
comprehensive test coverage.
**Type:** bugfix
**Components Affected:** deck.gl Scatter chart layer, Query building for
Scatter charts, Data transformation utilities, Tooltip generation
<details>
<summary><strong>Files Changed</strong></summary>
| File | Summary | Change | Impact |
|:-----|:--------|:------:|:------:|
| `...set-chart-deckgl/src/layers/Scatter/Scatter.tsx` | Updated tooltip
generation logic to only show metric labels when radius is metric-based, not
for fixed values | ✏️ | 🟢 |
| `...t-chart-deckgl/src/layers/Scatter/buildQuery.ts` | Added type checking
to only include metrics in query when point_radius_fixed.type is 'metric',
preventing non-existent metric fields from being queried | ✏️ | 🔴 |
| `...art-deckgl/src/layers/Scatter/transformProps.ts` | Updated data
processing to handle fixed radius values separately from metric values,
applying fixed values directly to all points | ✏️ | 🔴 |
| `...reset-chart-deckgl/src/layers/transformUtils.ts` | Enhanced
getMetricLabelFromFormData() to only return metric labels when type is
'metric', returning undefined for fixed values | ✏️ | 🟡 |
| `...rt-deckgl/src/layers/Scatter/buildQuery.test.ts` | Added 294 lines of
comprehensive tests for query building with both fixed and metric-based radius
scenarios | ➕ | 🟡 |
| `...eckgl/src/layers/Scatter/transformProps.test.ts` | Added 303 lines of
comprehensive tests for data transformation with various radius and spatial
configurations | ➕ | 🟡 |
| `...-chart-deckgl/src/layers/transformUtils.test.ts` | Added 184 lines of
tests for utility functions including metric label extraction from form data |
➕ | 🟡 |
</details>
<details>
<summary><strong>Architecture Impact</strong></summary>
- **Coupling:** Updated type signature of point_radius_fixed to include type
discriminator ('fix' | 'metric'), requiring consumers to check the type field
before processing values
- **Breaking Changes:** point_radius_fixed interface now requires type field
to be 'fix' or 'metric' for proper behavior; existing code passing plain string
values may not work as expected
</details>
**Risk Areas:** Type signature change in
DeckScatterFormData.point_radius_fixed may affect other parts of the codebase
that construct this form data without the new 'type' field, The fix assumes
metric values are always numeric; edge cases with non-numeric metrics could
cause parseMetricValue to return undefined, Backward compatibility: existing
saved charts with point_radius_fixed may have undefined 'type' field,
defaulting to metric behavior rather than fixed
<details>
<summary><strong>Suggestions</strong></summary>
- Consider adding a migration or fallback to handle existing charts that may
have point_radius_fixed without the type field
- Add validation in the form/UI layer to ensure type field is always set
when point_radius_fixed is defined
- Consider testing with actual Superset UI to verify the form properly sets
the type field for new and edited charts
</details>
<sub>Full review in progress... | Powered by <a
href="https://diffray.ai?utm_source=github-summary";>diffray</a></sub>
--
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]
