fix(date_parser): add check for time range timeshifts #36039
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Recursive call loses function parameters ▹ view | |
 |
Potential infinite recursion in time_shift processing ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset/utils/date_parser.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #e4353a
Actionable Suggestions - 1
-
superset/utils/date_parser.py - 1
- Incorrect relative date handling in time_shift range parsing · Line 480-482
Review Details
-
Files reviewed - 2 · Commit Range:
3a06eb7..3241c6b- superset/utils/date_parser.py
- tests/unit_tests/utils/date_parser_tests.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
|
Redundant separator variable assignment ▹ view | |
|
Time shift overrides instead of modifying existing time range ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset/utils/date_parser.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| if " : " in time_shift: | ||
| # Date range format: parse as a new time range | ||
| separator = " : " | ||
| since_part, until_part = time_shift.split(separator, 1) |
There was a problem hiding this comment.
Redundant separator variable assignment 
Tell me more
What is the issue?
The separator string " : " is defined as a local variable inside the conditional block, but the same separator is already defined earlier in the function scope.
Why this matters
This creates unnecessary string object allocation and variable assignment when the same separator value is already available in the function scope, leading to minor memory overhead and redundant operations.
Suggested change ∙ Feature Preview
Remove the local separator = " : " assignment and reuse the existing separator variable that was defined earlier in the function:
if separator in time_shift:
# Date range format: parse as a new time range
since_part, until_part = time_shift.split(separator, 1)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| if " : " in time_shift: | ||
| # Date range format: parse as a new time range | ||
| separator = " : " | ||
| since_part, until_part = time_shift.split(separator, 1) | ||
| _since = datetime_eval(f"DATETIME('{since_part.strip()}')") | ||
| _until = datetime_eval(f"DATETIME('{until_part.strip()}')") |
There was a problem hiding this comment.
Time shift overrides instead of modifying existing time range 
Tell me more
What is the issue?
The time_shift functionality completely overrides the existing _since and _until values when a date range format is detected, ignoring the original time range parameters.
Why this matters
This breaks the expected behavior where time_shift should modify existing time ranges rather than replace them entirely. Users expecting time_shift to adjust their current time range will get unexpected results as their original range is completely discarded.
Suggested change ∙ Feature Preview
The time_shift with date range format should either be treated as an error condition or should modify the existing _since and _until values rather than replacing them. Consider adding validation to prevent this conflicting usage:
if time_shift:
if " : " in time_shift:
# This conflicts with existing time range - should be an error
raise ValueError("time_shift cannot contain date range format when time range is already specified")
else:
time_delta_since = parse_past_timedelta(time_shift, _since)
time_delta_until = parse_past_timedelta(time_shift, _until)
_since = _since if _since is None else (_since - time_delta_since)
_until = _until if _until is None else (_until - time_delta_until)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for using date range format (with " : " separator) in the time_shift parameter of the get_since_until() function. Previously, time_shift only accepted time delta formats like "1 year ago". Now it can also accept a full date range like "yesterday : today", which will replace the original time range entirely rather than shifting it.
Key Changes:
- Modified
time_shiftlogic to detect and handle date range format (containing " : ") - When detected, the time_shift value is parsed as a new time range instead of being applied as a delta
- Added test case to verify the new behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| superset/utils/date_parser.py | Added conditional logic to parse time_shift as date range when it contains " : " separator |
| tests/unit_tests/utils/date_parser_tests.py | Added test case for time_shift with date range format |
| _until = _until if _until is None else (_until - time_delta_until) | ||
| if " : " in time_shift: | ||
| # Date range format: parse as a new time range | ||
| separator = " : " |
There was a problem hiding this comment.
The separator variable is redundant since it's already defined at line 347 with the same value. This line can be removed as the separator is already in scope.
| separator = " : " |
| _since = datetime_eval(f"DATETIME('{since_part.strip()}')") | ||
| _until = datetime_eval(f"DATETIME('{until_part.strip()}')") |
There was a problem hiding this comment.
The time_shift date range parsing bypasses the sophisticated time_range_lookup logic (lines 419-465) that handles special patterns like 'start of next month', 'last 5 days', etc. This creates inconsistent behavior where 'today : tomorrow' works in time_range but patterns like 'last week : today' won't work in time_shift. Consider reusing the same parsing logic by extracting it into a reusable function.
| _since = _since if _since is None else (_since - time_delta_since) | ||
| _until = _until if _until is None else (_until - time_delta_until) | ||
| if " : " in time_shift: | ||
| # Date range format: parse as a new time range |
There was a problem hiding this comment.
Should we just define the seperator variable at line 478 and then use it at line 480 and not redefine it at line 482
| # Date range format: parse as a new time range | ||
| separator = " : " | ||
| since_part, until_part = time_shift.split(separator, 1) | ||
| _since = datetime_eval(f"DATETIME('{since_part.strip()}')") |
There was a problem hiding this comment.
Can we use parse_human_datetime() for _since , as it will work fine for this case without using f string
parts = time_shift.split(separator, 1)
if len(parts) != 2:
raise ValueError(f"Invalid time_shift format: {time_shift}")
since_part, until_part = (part.strip() for part in parts)
_since = parse_human_datetime(since_part)
_until = parse_human_datetime(until_part)
Something like this i guess
2 participants
SUMMARY
Check for timerange timeshifts when parsing the date to handle timeshift values like '2004-01-01 : 2005'.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
API call for adding a timeshift should work with a format like '2004-01-01 : 2005' when the
DATE_RANGE_TIMESHIFTS_ENABLEDflag is enabled.ADDITIONAL INFORMATION