bito-code-review[bot] commented on PR #43572:
URL: https://github.com/apache/superset/pull/43572#issuecomment-5438900298

   <!-- Bito Reply -->
   The review comment is correct. The `BubbleChartConfig` validator currently 
only checks `sql_expression` and `saved_metric` for dimensions, allowing 
`aggregate` to be passed, which is then silently ignored by the mapper. To 
resolve this, you should update the `reject_metric_style_dimensions` validator 
in `superset/mcp_service/chart/schemas.py` to also reject the `aggregate` field 
on `entity` and `series` dimensions.
   
   Here is the corrected validator implementation:
   
   ```python
       @model_validator(mode="after")
       def reject_metric_style_dimensions(self) -> "BubbleChartConfig":
           """entity and series are dimensions, not metrics."""
           dims = [(self.entity, "entity")]
           if self.series is not None:
               dims.append((self.series, "series"))
           for col, name in dims:
               _reject_sql_expression_on_dimension(col, name)
               if col.saved_metric:
                   raise ValueError(
                       f"{name} cannot use saved_metric=True; "
                       "saved metrics belong in the 'x', 'y', or 'size' fields"
                   )
               if col.aggregate:
                   raise ValueError(f"{name} cannot use aggregate; dimensions 
do not support aggregation")
           return self
   ```
   
   Would you like me to fetch all other comments on this PR to validate and 
implement fixes for them as well?
   
   **superset/mcp_service/chart/schemas.py**
   ```
   @model_validator(mode="after")
       def reject_metric_style_dimensions(self) -> "BubbleChartConfig":
           """entity and series are dimensions, not metrics."""
           dims = [(self.entity, "entity")]
           if self.series is not None:
               dims.append((self.series, "series"))
           for col, name in dims:
               _reject_sql_expression_on_dimension(col, name)
               if col.saved_metric:
                   raise ValueError(
                       f"{name} cannot use saved_metric=True; "
                       "saved metrics belong in the 'x', 'y', or 'size' fields"
                   )
               if col.aggregate:
                   raise ValueError(f"{name} cannot use aggregate; dimensions 
do not support aggregation")
           return self
   ```


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

Reply via email to