- 
                Notifications
    
You must be signed in to change notification settings  - Fork 16.1k
 
fix(charts): support integer column/metric IDs in deck.gl charts #35933
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -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 | ||
| 
         
      Comment on lines
    
      +1134
     to 
      +1137
    
   
  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Insufficient Type Validation in Order By Column 
Tell me moreWhat 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 mattersThis 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 Previewdef 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.  | 
||
| 
     | 
||
| 
     | 
||
| class ChartDataDatasourceSchema(Schema): | ||
| description = "Chart datasource" | ||
| id = Union( | ||
| 
          
            
          
           | 
    @@ -1328,9 +1334,7 @@ class Meta: # pylint: disable=too-few-public-methods | |
| fields.Tuple( | ||
| ( | ||
| fields.Raw( | ||
| validate=[ | ||
| Length(min=1, error=_("orderby column must be populated")) | ||
| ], | ||
| validate=validate_orderby_column, | ||
| allow_none=False, | ||
| ), | ||
| fields.Boolean(), | ||
| 
          
            
          
           | 
    ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -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) | ||
| 
         
      Comment on lines
    
      +1234
     to 
      +1235
    
   
  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Integer column ID converted to string without name resolution 
Tell me moreWhat 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 mattersThis 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 PreviewConsider 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.  | 
||
| 
     | 
||
| raise ValueError("Missing label") | ||
| 
     | 
||
| 
     | 
||
| 
          
            
          
           | 
    @@ -1263,6 +1266,9 @@ def get_metric_name(metric: Metric, verbose_map: dict[str, Any] | None = None) - | |
| verbose_map = verbose_map or {} | ||
| return verbose_map.get(metric, metric) | ||
| 
     | 
||
| if isinstance(metric, int): | ||
| return str(metric) | ||
| 
     | 
||
| raise ValueError(__("Invalid metric object: %(metric)s", metric=str(metric))) | ||
| 
     | 
||
| 
     | 
||
| 
          
            
          
           | 
    ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.