fix(charts): support integer column/metric IDs in deck.gl charts #35933
Conversation
Code Review Agent Run #643580Actionable Suggestions - 0Additional Suggestions - 10
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
dosubot
bot
added
change:backend
![korbit-ai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Inefficient filter lookup with O(n) complexity ▹ view | |
 |
Insufficient Type Validation in Order By Column ▹ view | |
 |
Integer column ID converted to string without name resolution ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Scatter/buildQuery.ts | ✅ |
| superset/superset_typing.py | ✅ |
| superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/spatialUtils.ts | ✅ |
| superset/common/query_object.py | ✅ |
| superset/charts/schemas.py | ✅ |
| superset/utils/core.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| const existingFilterCols = new Set( | ||
| filters | ||
| .filter(filter => filter.op === 'IS NOT NULL') | ||
| .map(filter => filter.col), | ||
| ); |
There was a problem hiding this comment.
Inefficient filter lookup with O(n) complexity
Tell me more
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:
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
| 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 |
There was a problem hiding this comment.
Insufficient Type Validation in Order By Column
Tell me more
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
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 TrueProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| if isinstance(column, int): | ||
| return str(column) |
There was a problem hiding this comment.
Integer column ID converted to string without name resolution
Tell me more
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:
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
… types and improve handling of undefined values
1 participant
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:
ColumnandMetrictype definitions to includeintin addition to strings and adhoc objectsvalidate_orderby_column()validator to accept integers, replacing Marshmallow'sLengthvalidator_set_metrics()inQueryObjectto handle integer metric IDsget_metric_name()andget_column_name()utility functionsFrontend:
buildQueryto prevent duplicate columns when the same column is used for both latitude and longitudetype === 'metric', not whentype === 'fix'addSpatialNullFilters()for spatial columnsTests:
get_metric_name()andget_column_name()with integer IDsBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: Creating a deck.gl scatterplot chart resulted in error:
After: deck.gl scatterplot charts create successfully with integer column/metric IDs.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION