aminghadersohi commented on PR #42283:
URL: https://github.com/apache/superset/pull/42283#issuecomment-5145717864
Thanks for catching this, Rafael — verified against the actual behavior and
you're right on all counts. `get_since_until(time_range="Last minute")` does
rewrite to `"Last minute : today"` and raises `From date cannot be larger than
to date` (since resolves against `now`, until against midnight). Confirmed the
same for `Last second`, `Last 5 minutes`, and `Last 2 hours`.
One small correction on the mechanism for `Last hour` specifically: it's not
a `TimeRangeParseFailError` from the missing unit — `hour` falling out of the
`(second|minute|day|week|month|quarter|year)` regex means it falls through to
the literal-string fallback (`DATETIME('Last hour')`), and that fallback parser
(`parse_human_datetime`) actually succeeds at fuzzily interpreting "Last hour"
as a single timestamp. That timestamp still ends up later than the "today"
until, so it hits the *same* `From date cannot be larger than to date`
ValueError as the others — just via a different code path. Net effect for the
caller is identical (an unhandled crash on a value the validator claimed was
fine), so it doesn't change the fix, just the diagnosis.
Went with your option (a): normalized bare sub-day `Last <n>?
<second|minute|hour>` values to the same `DATEADD(DATETIME('now'), -n, UNIT) :
DATETIME('now')` form already used for the `[second]`/`[minute]`/`[hour]`
bracket shorthands, rather than rejecting them. Reasoning: the PR already
treats those bracket tokens as "this is a real request for a rolling sub-day
window, just resolve it correctly" — rejecting the spelled-out `Last hour` form
while accepting `[hour]` would be an inconsistency in the accepted grammar
that's likely to surface as its own follow-up bug report. `Next
<second|minute|hour>` doesn't need the same treatment — it pairs with a `today`
(midnight) *since*, which is always ≤ the `now`-based until, so it already
resolves correctly.
Also tightened the rejection message's `'Last <unit>'` example to include a
sub-day case (`'Last hour'`), since that's now genuinely accepted rather than a
value that would slip through only to crash downstream.
Added `TestValidateTimeRangeSubDayLast` covering exactly the five values you
named (`Last second`, `Last minute`, `Last hour`, `Last 5 minutes`, `Last 2
hours`), each asserting the bare form previously raised in `get_since_until()`,
that `validate_time_range()` now normalizes it, and that the normalized form
round-trips to a bounded `since < until` range. Also added cases for the
lowercase `last hour` (still rejected, matching `get_since_until()`'s
case-sensitive check) and `Next hour` (unaffected, passes through unchanged).
Ran `ruff format`/`ruff check`/`mypy` on the changed files and the full
`date_parser` + `time_range_validation` suites — all green.
--
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]
