aminghadersohi commented on PR #42283:
URL: https://github.com/apache/superset/pull/42283#issuecomment-5145748989
Thanks @rebenitez1802 — confirmed and fixed in 391d9b0.
Reproduced the mismatch first. Every sub-day `Last …` form raises, not just
the ones called out:
```
Last second / Last minute / Last hour -> ValueError: From date cannot
be larger than to date
Last 5 minutes / Last 2 hours / Last 30 seconds -> same
```
One correction to the diagnosis: `Last hour` fails through the same
since/until path as the others (`"Last hour : today"`, since anchored on `now`,
until on midnight), not via `TimeRangeParseFailError`. Same outcome, same fix.
**Fix** — took the first of your two options and reused the `DATEADD`
normalization, so sub-day values resolve to the range the caller asked for
instead of erroring:
```
Last hour -> DATEADD(DATETIME('now'), -1, HOUR) : DATETIME('now')
Last 5 minutes -> DATEADD(DATETIME('now'), -5, MINUTE) : DATETIME('now')
Last 30 seconds -> DATEADD(DATETIME('now'), -30, SECOND) : DATETIME('now')
```
The bracket map now holds plain `Last <unit>` strings and routes through
that same single rewrite, so `[hour]` and a bare `Last hour` provably converge
on one canonical range (asserted in a test) rather than the expressions being
spelled out in two places. Unit casing is ignored, since `Last Hour` fails
downstream identically; the `Last` prefix itself stays case-sensitive to mirror
`get_since_until()`.
**Why `startswith("Last")` stays broad** rather than becoming a unit
whitelist: `get_since_until()` hands the remainder to a freeform parser, so
these all resolve correctly today and a whitelist would regress them:
```
Last Monday -> 2026-07-27 .. 2026-07-31
Last January -> 2026-01-01 .. 2026-07-31
Last year to date -> 2025-01-01 .. 2026-07-31
Last 3 days ago -> 2026-07-28 .. 2026-07-31
```
An unparseable unit like `Last decade` still raises a loud
`TimeRangeParseFailError` downstream — noisy, but not the silent full-table
match this validator exists to prevent, so I left it out of scope. That
reasoning is now in the `_has_recognized_bare_prefix` docstring so the next
reader doesn't have to re-derive it.
Also took your point about the message steering callers wrong — it now lists
`'Last hour'` and `'Last 5 minutes'` among the valid examples.
**Tests:** each sub-day case asserts the premise (raw value raises) before
asserting the rewrite; plus guards that day-and-coarser values and the freeform
tails above pass through untouched, that lowercase `last hour` is still
rejected, and that `Next <sub-day>` needs no treatment (it pairs with a
midnight `since`, so `since <= until` always holds). Tool-level coverage added
in `test_query_dataset.py`. 168 tests pass across the four affected suites.
--
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]
