You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mystichronicle (via GitHub)" <gi...@apache.org> on 2025/10/31 17:23:58 UTC
[PR] fix(charts): support integer column/metric IDs in deck.gl charts [superset]
mystichronicle opened a new pull request, #35933:
URL: https://github.com/apache/superset/pull/35933
### SUMMARY
This PR fixes issue #35930 where deck.gl scatterplot charts failed to create with the error `'int' object has no len()`. The root cause was that deck.gl visualizations send integer column and metric IDs, but the backend validation and type system only expected strings or adhoc metric/column objects.
**Changes:**
**Backend:**
- Updated `Column` and `Metric` type definitions to include `int` in addition to strings and adhoc objects
- Added custom `validate_orderby_column()` validator to accept integers, replacing Marshmallow's `Length` validator
- Updated `_set_metrics()` in `QueryObject` to handle integer metric IDs
- Added integer handling in `get_metric_name()` and `get_column_name()` utility functions
**Frontend:**
- Added column deduplication in deck.gl Scatter `buildQuery` to prevent duplicate columns when the same column is used for both latitude and longitude
- Fixed static point size handling to only use as metric when `type === 'metric'`, not when `type === 'fix'`
- Added duplicate filter prevention in `addSpatialNullFilters()` for spatial columns
**Tests:**
- Added unit tests for orderby and metrics field validation with integers
- Added unit tests for `get_metric_name()` and `get_column_name()` with integer IDs
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
**Before:** Creating a deck.gl scatterplot chart resulted in error:
```
TypeError: object of type 'int' has no len()
```
**After:** deck.gl scatterplot charts create successfully with integer column/metric IDs.
### TESTING INSTRUCTIONS
1. Ensure Superset is running with the changes
2. Navigate to a dataset with numeric columns
3. Create a new deck.gl Scatterplot chart
4. Select columns for Longitude and Latitude (using the same column for both to test deduplication)
5. Configure point radius as either:
- Static value (type: "fix") - should not be treated as metric
- Dynamic metric (type: "metric") - should be included in query
6. Click "Update Chart" or "Run"
7. Verify chart creates successfully without errors
8. Run backend tests:
```bash
pytest tests/unit_tests/charts/test_schemas.py::test_chart_data_query_object_schema_orderby_validation
pytest tests/unit_tests/charts/test_schemas.py::test_chart_data_query_object_schema_metrics_validation
pytest tests/unit_tests/utils/test_core.py::test_get_metric_name_with_integers
pytest tests/unit_tests/utils/test_core.py::test_get_column_name_with_integers
```
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #35930
- [ ] 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(charts): support integer column/metric IDs in deck.gl charts [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #35933:
URL: https://github.com/apache/superset/pull/35933#issuecomment-3504336756
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/35933?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `50.00000%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 68.20%. Comparing base ([`7265567`](https://app.codecov.io/gh/apache/superset/commit/72655675611adac3236de42c7c08e2a848f5e189?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e3a329d`](https://app.codecov.io/gh/apache/superset/commit/e3a329dc116b969e7cc5f175f3c66a19330dd891?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 21 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/35933?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/utils/core.py](https://app.codecov.io/gh/apache/superset/pull/35933?src=pr&el=tree&filepath=superset%2Futils%2Fcore.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvY29yZS5weQ==) | 0.00% | [3 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/superset/pull/35933?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/charts/schemas.py](https://app.codecov.io/gh/apache/superset/pull/35933?src=pr&el=tree&filepath=superset%2Fcharts%2Fschemas.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY2hhcnRzL3NjaGVtYXMucHk=) | 50.00% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/superset/pull/35933?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #35933 +/- ##
===========================================
+ Coverage 0 68.20% +68.20%
===========================================
Files 0 622 +622
Lines 0 45720 +45720
Branches 0 4975 +4975
===========================================
+ Hits 0 31183 +31183
- Misses 0 13288 +13288
- Partials 0 1249 +1249
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/35933/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/35933/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `44.20% <25.00%> (?)` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/35933/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `47.74% <50.00%> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/35933/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `68.18% <50.00%> (?)` | |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/35933/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `67.47% <50.00%> (?)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/35933/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ΓΈ> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/35933?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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(charts): support integer column/metric IDs in deck.gl charts [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35933:
URL: https://github.com/apache/superset/pull/35933#issuecomment-3597951312
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19831491156) for [7bf14c1](https://github.com/apache/superset/commit/7bf14c1)
β’ **Environment:** http://16.146.112.84:8080 (admin/admin)
β’ **Lifetime:** 48h auto-cleanup
β’ **Updates:** New commits create fresh environments automatically
--
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(charts): support integer column/metric IDs in deck.gl charts [superset]
Posted by "mystichronicle (via GitHub)" <gi...@apache.org>.
mystichronicle commented on PR #35933:
URL: https://github.com/apache/superset/pull/35933#issuecomment-3487888650
@geido could you please review?
--
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(charts): support integer column/metric IDs in deck.gl charts [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35933:
URL: https://github.com/apache/superset/pull/35933#issuecomment-3544136213
Looks like a small conflict mitigation / rebase is needed... but pinging @richardfogaca here who might want to take a look.
--
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(charts): support integer column/metric IDs in deck.gl charts [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35933:
URL: https://github.com/apache/superset/pull/35933#issuecomment-3474102113
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(charts): support integer column/metric IDs in deck.gl charts [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35933:
URL: https://github.com/apache/superset/pull/35933#issuecomment-3597874954
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19831491156) for [7bf14c1](https://github.com/apache/superset/commit/7bf14c1)
--
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(charts): support integer column/metric IDs in deck.gl charts [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #35933:
URL: https://github.com/apache/superset/pull/35933#discussion_r2491639441
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/transformUtils.ts:
##########
@@ -134,9 +134,10 @@ export function addPropertiesToFeature<T extends Record<string, unknown>>(
}
Review Comment:
The newly added `type` property in the metric object parameter lacks documentation. Consider adding a JSDoc comment to explain what values `type` can have (e.g., 'metric', 'fix') and when each type should be used, as this affects the function's behavior significantly.
```suggestion
/**
* Returns the metric label from the given metric object or string.
*
* @param metric - The metric to extract the label from. Can be:
* - a string (the metric name),
* - an object with optional `type` and `value` properties.
* - `type`: Can be 'metric' or 'fix'.
* - 'metric': Indicates a regular metric; label will be generated from `value`.
* - 'fix': Indicates a fixed value; label will not be generated (returns undefined).
* - `value`: The value of the metric (string or number).
* - undefined.
* @returns The metric label, or undefined if not applicable.
*/
```
--
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(charts): support integer column/metric IDs in deck.gl charts [superset]
Posted by "palaxi0 (via GitHub)" <gi...@apache.org>.
palaxi0 commented on PR #35933:
URL: https://github.com/apache/superset/pull/35933#issuecomment-3603327754
@mystichronicle While the actual root cause is not an issue now (Deduplication and Size point recognizes and render the plot) I am facing an issue while trying a simple point size based on a metric like this.
Error: Metric '[object Object]' does not exist
<img width="1622" height="928" alt="Captura de pantalla 2025-12-02 a la(s) 12 33 16β―p m" src="https://github.com/user-attachments/assets/44b67ec7-0703-43f1-a2d1-83ff9020f29c" />
Can you please take a look?
--
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(charts): support integer column/metric IDs in deck.gl charts [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #35933:
URL: https://github.com/apache/superset/pull/35933#issuecomment-3530298294
Superset uses Git pre-commit hooks courtesy of [pre-commit](https://pre-commit.com/). To install run the following:
```
pip3 install -r requirements/development.txt
pre-commit install
```
A series of checks will now run when you make a git commit.
Alternatively it is possible to run pre-commit by running pre-commit manually:
```
pre-commit run --all-files
```
--
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(charts): support integer column/metric IDs in deck.gl charts [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35933:
URL: https://github.com/apache/superset/pull/35933#issuecomment-3554934982
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19518717219) for [7bf14c1](https://github.com/apache/superset/commit/7bf14c1)
β’ **Environment:** http://54.200.140.8:8080 (admin/admin)
β’ **Lifetime:** 48h auto-cleanup
β’ **Updates:** New commits create fresh environments automatically
--
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(charts): support integer column/metric IDs in deck.gl charts [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #35933:
URL: https://github.com/apache/superset/pull/35933#discussion_r2624674487
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.ts:
##########
@@ -75,17 +77,23 @@ export default function buildQuery(formData: DeckScatterFormData) {
typeof col === 'string' ? col : col.label || col.sqlExpression || '',
);
const withJsColumns = addJsColumnsToColumns(columnStrings, js_columns);
+ const uniqueColumns = ensureColumnsUnique(withJsColumns);
- columns = withJsColumns as QueryFormColumn[];
+ columns = uniqueColumns as QueryFormColumn[];
columns = addTooltipColumnsToQuery(columns, tooltip_contents);
- const metrics = processMetricsArray([point_radius_fixed?.value]);
+ const isRadiusMetric =
+ point_radius_fixed?.type === 'metric' && point_radius_fixed?.value;
+ const metrics = isRadiusMetric
Review Comment:
Isn't this a type mismatch between metrics and orderby?
When point_radius_fixed.type === 'metric', the code:
- Passes String(point_radius_fixed.value) to processMetricsArray (stringifying the ID)
- But passes raw point_radius_fixed.value (integer) to orderby
This means for numeric metric IDs, the backend receives metrics: ["5"] but orderby: [[5, false]]. The metric lookup will fail because the string "5" doesn't match the integer 5.
Could probably add a quick unit test to verify this
--
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(charts): support integer column/metric IDs in deck.gl charts [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35933:
URL: https://github.com/apache/superset/pull/35933#discussion_r2482191956
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/spatialUtils.ts:
##########
@@ -109,11 +109,20 @@ export function addSpatialNullFilters(
if (!spatial) return filters;
const spatialColumns = getSpatialColumns(spatial);
- const nullFilters: QueryObjectFilterClause[] = spatialColumns.map(column => ({
- col: column,
- op: 'IS NOT NULL',
- val: null,
- }));
+ const uniqueSpatialColumns = [...new Set(spatialColumns)];
+ const existingFilterCols = new Set(
+ filters
+ .filter(filter => filter.op === 'IS NOT NULL')
+ .map(filter => filter.col),
+ );
Review Comment:
### Inefficient filter lookup with O(n) complexity <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code creates a new Set on every function call by filtering and mapping the entire filters array, which has O(n) time complexity where n is the number of filters.
###### Why this matters
This approach becomes inefficient when the filters array is large or when addSpatialNullFilters is called frequently, as it processes the entire array each time instead of using a more efficient lookup mechanism.
###### Suggested change β *Feature Preview*
Consider using a Map or optimized lookup structure if this function is called frequently with large filter arrays, or cache the existingFilterCols Set if the filters don't change between calls:
```typescript
const existingFilterCols = filters.reduce((acc, filter) => {
if (filter.op === 'IS NOT NULL') {
acc.add(filter.col);
}
return acc;
}, new Set<string>());
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0f9df90-a8da-4e07-9bb9-7fd9b96a961b/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0f9df90-a8da-4e07-9bb9-7fd9b96a961b?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0f9df90-a8da-4e07-9bb9-7fd9b96a961b?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0f9df90-a8da-4e07-9bb9-7fd9b96a961b?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0f9df90-a8da-4e07-9bb9-7fd9b96a961b)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b53caedd-f24d-47cf-9ec8-c3ce6790b62c -->
[](b53caedd-f24d-47cf-9ec8-c3ce6790b62c)
##########
superset/utils/core.py:
##########
@@ -1231,6 +1231,9 @@ def get_column_name(column: Column, verbose_map: dict[str, Any] | None = None) -
verbose_map = verbose_map or {}
return verbose_map.get(column, column)
+ if isinstance(column, int):
+ return str(column)
Review Comment:
### Integer column ID converted to string without name resolution <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The function converts integer column IDs directly to strings without any validation or lookup, which may not provide meaningful column names for users.
###### Why this matters
This approach bypasses the intended column resolution logic and may result in displaying raw numeric IDs instead of human-readable column names, potentially confusing users who expect descriptive labels.
###### Suggested change β *Feature Preview*
Consider implementing a lookup mechanism to resolve integer IDs to actual column names, or add documentation explaining when raw integer strings are acceptable. For example:
```python
if isinstance(column, int):
# TODO: Implement ID-to-name lookup when datasource context is available
# For now, return string representation as fallback
return str(column)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb66105a-d433-41fb-a132-af508da512ce/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb66105a-d433-41fb-a132-af508da512ce?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb66105a-d433-41fb-a132-af508da512ce?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb66105a-d433-41fb-a132-af508da512ce?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bb66105a-d433-41fb-a132-af508da512ce)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:adea077d-2eca-4a35-af43-841f103315ec -->
[](adea077d-2eca-4a35-af43-841f103315ec)
##########
superset/charts/schemas.py:
##########
@@ -1131,6 +1131,12 @@ class AnnotationLayerSchema(Schema):
)
+def validate_orderby_column(value: Any) -> bool:
+ if value is None or (isinstance(value, str) and len(value) == 0):
+ raise validate.ValidationError(_("orderby column must be populated"))
+ return True
Review Comment:
### Insufficient Type Validation in Order By Column <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The validate_orderby_column function is too permissive in its type checking. It only validates for None and empty strings, but accepts any other type without validation.
###### Why this matters
This could lead to runtime errors when the validation passes but the value is of an incompatible type (like a number or boolean) that can't be used as an orderby column.
###### Suggested change β *Feature Preview*
```python
def validate_orderby_column(value: Any) -> bool:
if not isinstance(value, str):
raise validate.ValidationError(_('orderby column must be a string'))
if len(value) == 0:
raise validate.ValidationError(_('orderby column must be populated'))
return True
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8189bed-2f84-42dc-9478-0a6b4d65dac5/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8189bed-2f84-42dc-9478-0a6b4d65dac5?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8189bed-2f84-42dc-9478-0a6b4d65dac5?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8189bed-2f84-42dc-9478-0a6b4d65dac5?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d8189bed-2f84-42dc-9478-0a6b4d65dac5)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:7f198d50-fc24-4331-b39a-72ada8e98b3e -->
[](7f198d50-fc24-4331-b39a-72ada8e98b3e)
--
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(charts): support integer column/metric IDs in deck.gl charts [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #35933:
URL: https://github.com/apache/superset/pull/35933#discussion_r2491961413
##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.ts:
##########
@@ -75,17 +77,23 @@ export default function buildQuery(formData: DeckScatterFormData) {
typeof col === 'string' ? col : col.label || col.sqlExpression || '',
);
const withJsColumns = addJsColumnsToColumns(columnStrings, js_columns);
+ const uniqueColumns = ensureColumnsUnique(withJsColumns);
- columns = withJsColumns as QueryFormColumn[];
+ columns = uniqueColumns as QueryFormColumn[];
columns = addTooltipColumnsToQuery(columns, tooltip_contents);
- const metrics = processMetricsArray([point_radius_fixed?.value]);
+ const isRadiusMetric =
+ point_radius_fixed?.type === 'metric' && point_radius_fixed?.value;
Review Comment:
The condition `point_radius_fixed?.value` will be falsy when value is 0, which is a valid numeric metric ID. This should explicitly check for `point_radius_fixed?.value !== undefined` to properly handle the value 0.
```suggestion
point_radius_fixed?.type === 'metric' && point_radius_fixed?.value !== undefined;
```
--
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(charts): support integer column/metric IDs in deck.gl charts [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35933:
URL: https://github.com/apache/superset/pull/35933#issuecomment-3554911314
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19518717219) for [7bf14c1](https://github.com/apache/superset/commit/7bf14c1)
--
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(charts): support integer column/metric IDs in deck.gl charts [superset]
Posted by "mystichronicle (via GitHub)" <gi...@apache.org>.
mystichronicle closed pull request #35933: fix(charts): support integer column/metric IDs in deck.gl charts
URL: https://github.com/apache/superset/pull/35933
--
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