-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: Handle DST transitions in group_by_dynamic (#25410) #25459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
e2c2d32 to
ff5d1e9
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25459 +/- ##
==========================================
+ Coverage 79.58% 79.70% +0.11%
==========================================
Files 1743 1743
Lines 240439 240779 +340
Branches 3038 3038
==========================================
+ Hits 191347 191903 +556
+ Misses 48310 48094 -216
Partials 782 782 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
95938d5 to
c18ab6e
Compare
|
I feel this is a bit dangerous. We don't check if the error is indeed a DST transition, and even if true I'm not sure if skipping a full hour is always the correct behavior. |
cde2b19 to
cf387fe
Compare
7584916 to
ca8042b
Compare
|
@orlp Applied your feedback. The fix now uses a fallback helper that catches non-existent datetime errors during window boundary calculations and skips forward past the DST gap when a boundary falls on it. |
30edecb to
0e6556e
Compare
eb34d02 to
253a382
Compare
Co-authored-by: Claude <[email protected]>
Fixes #25410.
group_by_dynamicpanicked when window boundaries fell on non-existent datetimes during DST transitions, even when the actual data didn't contain those times.Reproduction
Cause
The
BoundsIterinpolars-timecalls.unwrap()on duration addition when computing window boundaries. With a 17-day period starting from Feb 7, the window boundaries include March 10, 2024 2:00 AM—a non-existent time in America/New_York when DST springs forward to 3:00 AM. This triggerstry_localize_datetimeto raise an error withNonExistent::Raise.Fix
Added fallback helper functions that catch non-existent datetime errors during window boundary calculations. When a boundary falls on a DST gap, the code skips forward by 1 hour and retries. This maintains performance in the common case (just a Result check) while gracefully handling DST transitions.