Skip to content

Support for non-UTC time zones in datetime conversion#131

Merged
ankane merged 1 commit intoankane:masterfrom
wtn:tz
Apr 25, 2026
Merged

Support for non-UTC time zones in datetime conversion#131
ankane merged 1 commit intoankane:masterfrom
wtn:tz

Conversation

@wtn
Copy link
Copy Markdown
Contributor

@wtn wtn commented Apr 24, 2026

Fixes #130.

_to_ruby_datetime raised Todo for non-UTC zones, and datetime_to_rb_object .unwrap()'d the funcall, so the Ruby exception became a thread panic.

Moves datetime conversion into Rust, mirroring py-polars (crates/polars-python/src/conversion/datetime.rs): chrono-tz for named zones, FixedOffset for offsets. any_value_into_rb_object now returns RbResult<Value> so tz parse errors raise instead of panic. Ruby-side _to_ruby_datetime is removed.

@ankane
Copy link
Copy Markdown
Owner

ankane commented Apr 24, 2026

Hi @wtn, thanks for the PR. Looks good overall, but a few comments:

  1. Please mirror the code exactly when possible, including the style (there are minor differences in chunked_array.rs and larger differences in datetime.rs)
  2. It looks like Python still calls to_py_datetime when year >= 2100, so I'm not sure _to_ruby_datetime should be removed
  3. Please use Assisted-by: instead of Co-authored-by: for Claude, like Linux

@wtn
Copy link
Copy Markdown
Contributor Author

wtn commented Apr 24, 2026

Thank you for feedback.

I've updated the commit message trailer, revised chunked_array.rs and datetime.rs, and restored _to_ruby_datetime with the addition of TZInfo for year >= 2100.

Copy link
Copy Markdown
Owner

@ankane ankane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, added some comments inline.

Comment thread lib/polars/utils/convert.rb Outdated
Comment thread ext/polars/src/conversion/datetime.rs Outdated
Comment thread ext/polars/src/conversion/chunked_array.rs Outdated
@wtn wtn force-pushed the tz branch 2 times, most recently from c5389ad to b423c80 Compare April 25, 2026 20:25
@ankane ankane merged commit 0936c9d into ankane:master Apr 25, 2026
@ankane
Copy link
Copy Markdown
Owner

ankane commented Apr 25, 2026

Looks great, thanks!

ankane added a commit that referenced this pull request Apr 25, 2026
@ankane
Copy link
Copy Markdown
Owner

ankane commented Apr 25, 2026

Made tzinfo optional in 6e775f1.

@wtn wtn deleted the tz branch April 25, 2026 21:55
@wtn
Copy link
Copy Markdown
Contributor Author

wtn commented Apr 25, 2026

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DataFrame#describe panics on tz-aware datetime columns

2 participants