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: [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]
