You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "DamianPendrak (via GitHub)" <gi...@apache.org> on 2025/10/06 18:23:25 UTC
[PR] fix(deckgl): scatterplot fix categorical color [superset]
DamianPendrak opened a new pull request, #35537:
URL: https://github.com/apache/superset/pull/35537
### SUMMARY
There was an issue with the deck.gl Scatterplot chart categorical color after refactor from legacy API.
1. Fixes the categorical colors. It was not showing points at all when categorical color was selected
2. When no categorical column is selected, it falls back to the fixed color value if available
3. Changes the default color palette from fixed to categorical
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): scatterplot fix categorical color [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35537:
URL: https://github.com/apache/superset/pull/35537#issuecomment-3373241932
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): scatterplot fix categorical color [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35537:
URL: https://github.com/apache/superset/pull/35537#discussion_r2407848542
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/CategoricalDeckGLContainer.tsx:
##########
@@ -163,6 +163,25 @@ const CategoricalDeckGLContainer = (props: CategoricalDeckGLContainerProps) => {
}));
}
case COLOR_SCHEME_TYPES.categorical_palette: {
+ // If no dimension selected, fall back to fixed color
+ if (!fd.dimension) {
+ const fallbackColor = fd.color_picker || {
+ r: 0,
+ g: 0,
+ b: 0,
+ a: 100,
+ };
+ return data.map(d => ({
+ ...d,
+ color: [
+ fallbackColor.r,
+ fallbackColor.g,
+ fallbackColor.b,
+ fallbackColor.a * 255,
+ ],
+ }));
Review Comment:
### Inefficient array creation in data mapping <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The color array is recreated for every data point when using fallback color, causing O(n) unnecessary array allocations.
###### Why this matters
With large datasets, this creates significant memory pressure and CPU overhead from repeated array creation and the spread operator on each data object.
###### Suggested change ∙ *Feature Preview*
Pre-compute the color array outside the map function:
```typescript
const colorArray = [
fallbackColor.r,
fallbackColor.g,
fallbackColor.b,
fallbackColor.a * 255,
];
return data.map(d => ({ ...d, color: colorArray }));
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95b80dac-48a4-44e3-a438-39d672512dcf/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95b80dac-48a4-44e3-a438-39d672512dcf?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95b80dac-48a4-44e3-a438-39d672512dcf?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95b80dac-48a4-44e3-a438-39d672512dcf?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95b80dac-48a4-44e3-a438-39d672512dcf)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5f9e834b-ded3-4f58-a4ae-0575c1901c56 -->
[](5f9e834b-ded3-4f58-a4ae-0575c1901c56)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): scatterplot fix categorical color [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe merged PR #35537:
URL: https://github.com/apache/superset/pull/35537
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): scatterplot fix categorical color [superset]
Posted by "DamianPendrak (via GitHub)" <gi...@apache.org>.
DamianPendrak closed pull request #35537: fix(deckgl): scatterplot fix categorical color
URL: https://github.com/apache/superset/pull/35537
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(deckgl): scatterplot fix categorical color [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #35537:
URL: https://github.com/apache/superset/pull/35537#discussion_r2407879765
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/controlPanel.ts:
##########
@@ -134,9 +133,7 @@ const config: ControlPanelConfig = {
controlSetRows: [
[legendPosition],
[legendFormat],
- ...generateDeckGLColorSchemeControls({
- defaultSchemeType: COLOR_SCHEME_TYPES.fixed_color,
- }),
+ ...generateDeckGLColorSchemeControls({}),
],
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing color scheme parameter</b></div>
<div id="fix">
The PR incorrectly removes the `defaultSchemeType: COLOR_SCHEME_TYPES.fixed_color` parameter from the `generateDeckGLColorSchemeControls` call. This changes the default color scheme behavior from `fixed_color` to `categorical_palette`, which breaks the intended Scatter plot color configuration. Based on the function signature in `Shared_DeckGL.tsx`, the parameter is optional but has specific behavior implications. Restore the original parameter to maintain consistent color scheme behavior across Scatter plots.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
...generateDeckGLColorSchemeControls({
defaultSchemeType: COLOR_SCHEME_TYPES.fixed_color,
}),
```
</div>
</details>
</div>
<small><i>Code Review Run <a href=https://github.com/apache/superset/pull/35537#issuecomment-3373241932>#a4c27a</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/CategoricalDeckGLContainer.tsx:
##########
@@ -156,13 +156,27 @@ const CategoricalDeckGLContainer = (props: CategoricalDeckGLContainerProps) => {
switch (selectedColorScheme) {
case COLOR_SCHEME_TYPES.fixed_color: {
color = fd.color_picker || { r: 0, g: 0, b: 0, a: 100 };
+ const colorArray = [color.r, color.g, color.b, color.a * 255];
- return data.map(d => ({
- ...d,
- color: [color.r, color.g, color.b, color.a * 255],
- }));
+ return data.map(d => ({ ...d, color: colorArray }));
}
case COLOR_SCHEME_TYPES.categorical_palette: {
+ if (!fd.dimension) {
+ const fallbackColor = fd.color_picker || {
+ r: 0,
+ g: 0,
+ b: 0,
+ a: 100,
+ };
+ const colorArray = [
+ fallbackColor.r,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Invalid alpha value range</b></div>
<div id="fix">
The alpha value in the fallback color is set to 100, which when multiplied by 255 results in 25500, exceeding the valid alpha range (0-255). This will cause color rendering issues in `DeckGLContainerStyledWrapper` and downstream components. Change the alpha to 1 (0-1 range) to match the pattern used in `getCategories` function.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```suggestion
a: 1,
```
</div>
</details>
</div>
<small><i>Code Review Run <a href=https://github.com/apache/superset/pull/35537#issuecomment-3373241932>#a4c27a</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org