aminghadersohi commented on PR #40342: URL: https://github.com/apache/superset/pull/40342#issuecomment-4565822445
A few schema issues worth addressing before merge: **`GetAnnotationLayerInfoRequest.id` should be `identifier`** Every other `get_*_info` tool in the MCP suite uses `identifier` as the field name (`GetChartInfoRequest`, `GetDashboardInfoRequest`, etc.). This is the only outlier. Suggest renaming for consistency. **`AnnotationFilter.col` should include `start_dttm` and `end_dttm`** Currently only `short_descr` is filterable. Annotations are inherently time-bounded events — time-range filtering is the primary use case (e.g. "show me annotations between date X and Y"). Recommend adding `start_dttm` and `end_dttm` to the `col: Literal[...]`. **`AnnotationList.layer_id: int = 0` sentinel is fragile** A default of `0` is a valid-looking but invalid layer ID. If the tool ever forgets to set it, the response silently has `"layer_id": 0`. Suggest `layer_id: int | None = None` and a `model_validator` that requires it to be set before serialization. **`AnnotationList.filters_applied` is typed as `List[ColumnOperator]` instead of `List[AnnotationFilter]`** All sibling `List*` schemas use the typed filter class (e.g. `List[QueryFilter]`, `List[TagFilter]`). This one uses the untyped base. Minor but inconsistent. -- 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]
