aminghadersohi commented on PR #42144:
URL: https://github.com/apache/superset/pull/42144#issuecomment-5037809658
I checked this branch out and exercised the tools over a live MCP session
against a local Superset (Postgres metadata DB, `examples` data) rather than
through the unit tests. The normalization itself works: all eight bracket
tokens, plus casing and surrounding whitespace, come out as expected, and
`applied_filters` in the response confirms the rewritten value reaches the
query context.
Three things came out of it that I think are worth addressing.
### 1. `'this week'` in the new description has the same failure mode this
PR fixes
`superset/mcp_service/dataset/schemas.py:866` now instructs the model:
> Use Superset relative shorthands like 'Last 7 days', 'Last month', 'Last
year', 'Last quarter', **'this week'**, 'previous calendar year'
`this week` does not parse. Live call against the branch:
```
query_dataset(dataset_id=28, metrics=["count"], time_range="this week")
→ 2823 rows (the entire table), warnings: []
```
Every other example in that sentence resolves correctly — I checked each
one. `previous calendar week` is the working equivalent if a current-period
example is wanted.
This seems worth fixing before merge: an LLM guesses `[year]` occasionally,
but it will follow the field description consistently.
Related, `Last week` parses and `last week` does not — `get_since_until`
does a case-sensitive `time_range.startswith("Last")`.
### 2. The stated root cause doesn't match the observed behavior
The description and the test docstrings (`test_query_dataset.py:1044-1047`,
`1110-1112`) say `[year]` "triggered `TimeRangeParseFailError`". I could not
reproduce that through this code path. `parse_human_datetime("[year]")` does
raise, but `query_dataset` never reaches it:
```python
get_since_until(time_range="[year]") → (None, datetime(2026, 7, 21)) # no
exception
get_since_until(since="[year]") → TimeRangeParseFailError
```
The mechanism is at `superset/utils/date_parser.py:581`, where all parsing
is gated on the `" : "` separator:
```python
if time_range and separator in time_range:
...parse both sides...
else:
since = since or "" # the `since` parameter — time_range is never
read here
_since = parse_human_datetime(since) if since else None
_until = parse_human_datetime(until) if until else
parse_human_datetime(_relative_end)
```
A separator-less value survives only if an earlier prefix rewrite (`Last…`,
`Next…`, `previous calendar …`, `Current …`, `first … of …`) converts it into
something containing the separator. Everything else falls into the `else`,
where `time_range` is discarded outright and the result is `(None, today)` —
unbounded start, no filtering, no error.
| value | result |
| --- | --- |
| `Last week`, `Last 7 days`, `previous calendar year`, ISO ranges | parse
correctly |
| `this week`, `this month`, `last week`, `yesterday`, `banana`, `[decade]`,
`[year]` | silently discarded → full table |
| `banana : split` | `TimeRangeParseFailError` |
So the bug being fixed is a silent wrong answer, not a raised error — which
is the more dangerous of the two and worth describing accurately, especially in
the docstrings, since those persist in the tree.
### 3. `get_table` has the same unguarded input
Pre-existing on master, not a regression from this PR, but the same class of
problem: `superset/mcp_service/semantic_layer/tool/get_table.py:230` appends
the raw string as a `TEMPORAL_RANGE` filter with no validation.
```
get_table(dataset_id=28, metrics=["count"], time_range="[year]") → 2823
(full table)
get_table(dataset_id=28, metrics=["count"], time_range="banana") → 2823
(full table), success: true, warnings: []
get_table(dataset_id=28, metrics=["count"], time_range="Last year") → 0
(correct)
get_table(dataset_id=28, metrics=["count"], time_range="2003-01-01 :
2004-01-01") → 1000 (correct)
```
`manage_native_filters` likewise accepts `default_time_range: "[year]"` and
persists it into the dashboard's filter config.
### Suggestion
Since the eight-token map covers eight members of an open class —
`[decade]`, `this week`, `yesterday`, and arbitrary garbage all still return
unfiltered data with `success: true` — a shared validator used by both
`QueryDatasetRequest` and `GetTableRequest` that rejects separator-less values
not matching the recognized prefix grammar would close the class instead.
Returning a `ValidationError` listing the accepted forms also turns a silent
wrong answer into something the model can self-correct from, which the current
behavior can't.
That predicate needs care — the `first … of …` patterns are handled before
the separator check — so the existing `date_parser` tests should be the guard.
Reasonable as a follow-up; only item 1 seems merge-blocking.
### Minor
`SC-113648` appears in two test docstrings (`test_query_dataset.py:1046`,
`1112`). That's an external tracker ID that won't resolve for anyone reading
this repo.
---
Environment notes for anyone reproducing: this branch needs
`sqlalchemy-continuum==1.6.0` installed and `superset db upgrade` run (master
added `tables.deleted_at`, and `list_datasets` fails against an unmigrated
metadata DB). Both are from the master merge, not from this change.
--
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]
