sadpandajoe commented on code in PR #44254:
URL: https://github.com/apache/superset/pull/44254#discussion_r4041651493
##########
superset/utils/date_parser.py:
##########
@@ -492,17 +519,18 @@ def get_since_until( # pylint:
disable=too-many-arguments,too-many-locals,too-m
"""
separator = " : "
- _relative_start = relative_start if relative_start else "today"
_relative_end = relative_end if relative_end else "today"
if time_range == NO_TIME_RANGE or time_range == _(NO_TIME_RANGE):
return None, None
if time_range and time_range.startswith("Last") and separator not in
time_range:
- time_range = time_range + separator + _relative_end
+ _end = relative_end or get_default_bound_for_shorthand(time_range)
Review Comment:
Independently re-verified this myself against the exact reviewed HEAD in an
isolated environment: confirmed `get_default_bound_for_shorthand()` is
genuinely unreached on the `get_since_until_from_time_range()`-wrapped path
(since `relative_start`/`relative_end` are always explicitly "today" there),
and that `get_relative_base`'s explicit-override branch plus the `hour`-regex
fix alone make that path correct. It's still load-bearing on the direct-call
path (no explicit relative_start/relative_end — MCP's
`_resolves_to_bounded_range()`, `views/api.py`'s `/api/v1/time_range/`, and the
unit tests). Keeping the `or` as-is — no change needed here, thanks for
spelling out the exact mechanism.
##########
superset/utils/date_parser.py:
##########
@@ -492,17 +519,18 @@ def get_since_until( # pylint:
disable=too-many-arguments,too-many-locals,too-m
"""
separator = " : "
- _relative_start = relative_start if relative_start else "today"
_relative_end = relative_end if relative_end else "today"
if time_range == NO_TIME_RANGE or time_range == _(NO_TIME_RANGE):
return None, None
if time_range and time_range.startswith("Last") and separator not in
time_range:
- time_range = time_range + separator + _relative_end
+ _end = relative_end or get_default_bound_for_shorthand(time_range)
+ time_range = time_range + separator + _end
if time_range and time_range.startswith("Next") and separator not in
time_range:
- time_range = _relative_start + separator + time_range
+ _start = relative_start or get_default_bound_for_shorthand(time_range)
Review Comment:
Fixed in fe2237e64a. Verified the exact scenario (`Next hour` + explicit
`relative_start="today"`, matching `get_since_until_from_time_range()`'s real
defaults) against the real function — it does not raise (since
`relative_start="today"` short-circuits the new helper, and "Next" always adds
a positive delta, so since≤until holds), but you're right it had zero coverage.
Added `test_get_since_until_sub_day_next_with_explicit_relative_bounds`,
covering second/minute/hour/2-hours with both bounds pinned to "today" via
freezegun.
##########
tests/unit_tests/mcp_service/common/test_time_range_validation.py:
##########
@@ -103,10 +105,14 @@ def test_strips_surrounding_whitespace(self) -> None:
class TestValidateTimeRangeSubDayLast:
"""Sub-day ``Last ...`` values are rewritten to an explicit DATEADD range.
- get_since_until() pairs a sub-day since-expression (resolved against
- ``now``) with a default until of ``today`` (midnight), so since lands
- after until and it raises "From date cannot be larger than to date".
- Anchoring both ends on ``now`` fixes that.
+ get_since_until() itself now resolves a sub-day ``Last ...`` value
Review Comment:
Fixed in adcc71a8c9. Updated the three stale docstrings in
`superset/mcp_service/common/time_range_validation.py` (the
`_SUB_DAY_REMAINDER` comment, `_normalize_sub_day_last()`, and
`_resolves_to_bounded_range()`) to describe current behavior instead of the
pre-fix raise. Same commit also extracts a shared `_RELATIVE_UNIT_PATTERN`
constant for the two regexes in `date_parser.py` (bito's other suggestion) to
close the exact duplication that caused the original `hour`-missing defect.
--
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]
