aminghadersohi commented on code in PR #42144:
URL: https://github.com/apache/superset/pull/42144#discussion_r3624891753
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -838,9 +861,12 @@ class QueryDatasetRequest(QueryCacheControl):
time_range: str | None = Field(
default=None,
description=(
- "Time range filter (e.g. 'Last 7 days', 'Last month', "
- "'2024-01-01 : 2024-12-31'). Requires a temporal column "
- "on the dataset."
+ "Time range filter. Use Superset relative shorthands like "
+ "'Last 7 days', 'Last month', 'Last year', 'Last quarter', "
+ "'this week', 'previous calendar year', or an ISO-8601 range "
+ "like '2024-01-01 : 2024-12-31'. Requires a temporal column "
+ "on the dataset. Do NOT use bracket shorthands like '[year]' "
+ "or '[quarter]' — use 'Last year' / 'Last quarter' instead."
Review Comment:
This is intentional, not a mismatch: the description states the preferred
format ("use \x27Last year\x27"), while normalize_time_range defensively
tolerates bracket shorthands as a fallback for LLMs that pass them anyway
despite the guidance -- that fallback is the entire point of this PR.
Recommending the canonical form while still accepting and correcting the
non-canonical one is deliberate, not contradictory. Leaving as-is.
##########
superset/mcp_service/dataset/schemas.py:
##########
@@ -838,9 +861,12 @@ class QueryDatasetRequest(QueryCacheControl):
time_range: str | None = Field(
default=None,
description=(
- "Time range filter (e.g. 'Last 7 days', 'Last month', "
- "'2024-01-01 : 2024-12-31'). Requires a temporal column "
- "on the dataset."
+ "Time range filter. Use Superset relative shorthands like "
+ "'Last 7 days', 'Last month', 'Last year', 'Last quarter', "
+ "'this week', 'previous calendar year', or an ISO-8601 range "
+ "like '2024-01-01 : 2024-12-31'. Requires a temporal column "
+ "on the dataset. Do NOT use bracket shorthands like '[year]' "
+ "or '[quarter]' — use 'Last year' / 'Last quarter' instead."
Review Comment:
This is intentional, not a mismatch: the description states the preferred
format ("use 'Last year'"), while normalize_time_range defensively tolerates
bracket shorthands as a fallback for LLMs that pass them anyway despite the
guidance — that fallback is the entire point of this PR. Recommending the
canonical form while still accepting and correcting the non-canonical one is
deliberate, not contradictory. Leaving as-is.
--
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]
