fix(semantic layers): start/end ranges#41590
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41590 +/- ##
==========================================
- Coverage 64.45% 64.42% -0.04%
==========================================
Files 2670 2668 -2
Lines 147340 147252 -88
Branches 33988 33960 -28
==========================================
- Hits 94966 94864 -102
- Misses 50647 50659 +12
- Partials 1727 1729 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment was marked as low quality.
This comment was marked as low quality.
Code Review Agent Run #9c15bcActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
rebenitez1802
left a comment
There was a problem hiding this comment.
Needs rebase, LGTM
|
The suggestion to add explicit type annotations to the temporal boundary variables is correct, as it improves code clarity and adheres to strict typing standards. You can resolve this by explicitly annotating the Here is the corrected implementation for start: datetime | None
end: datetime | None
start, end = get_since_until_from_time_range(time_range=value)There are no other comments on this PR to address. Would you like me to review any other parts of the code? superset/semantic_layers/mapper.py |
mikebridge
left a comment
There was a problem hiding this comment.
LGTM. Correct, well-tested fix — named time ranges ("Last week", etc.) no longer raise ValueError, and delegating to the canonical get_since_until_from_time_range parser keeps this path aligned with the rest of Superset's time handling. Nicely covered by the parametrized regression test. Approving.
Optional: a one-line docstring on get_since_until_from_time_range would be worth adding now that it's a shared entry point.
This review was generated by Claude (AI) on behalf of @mikebridge.
mikebridge
left a comment
There was a problem hiding this comment.
Approving — LGTM. Any notes in my earlier review are non-blocking.
This review was generated by Claude (AI) on behalf of @mikebridge.
SUMMARY
Use
get_since_until_from_time_rangeto split start/end from a time range when exploring a semantic view.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION