Skip to content
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

perf(python): Use PyO3 to convert between Python and Rust datetimes #20660

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

bschoenmaeckers
Copy link
Contributor

fixes #16199

Waiting for pyo3 v0.23.4 to be released. PyO3/pyo3#4835

Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 89.36170% with 5 lines in your changes missing coverage. Please review.

Project coverage is 79.02%. Comparing base (a0d96f2) to head (0a3fa09).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
crates/polars-python/src/conversion/datetime.rs 72.22% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #20660      +/-   ##
==========================================
- Coverage   79.03%   79.02%   -0.01%     
==========================================
  Files        1559     1559              
  Lines      221238   221235       -3     
  Branches     2529     2529              
==========================================
- Hits       174851   174834      -17     
- Misses      45806    45819      +13     
- Partials      581      582       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

delta.num_microseconds().unwrap()
};

Ok(AnyValue::Datetime(timestamp, TimeUnit::Microseconds, None))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this return the timezone info instead of None in the cases where there is one?

Copy link
Member

Choose a reason for hiding this comment

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

Currently we don't. I think eventually we have to, but then we also need to infer which timezone to set on the dtype.

@alexander-beedie alexander-beedie changed the title perf(python): use rust to convert to/from python datetimes perf(python): Use PyO3 to convert between Python and Rust datetimes Jan 13, 2025
@github-actions github-actions bot added performance Performance issues or improvements python Related to Python Polars and removed title needs formatting labels Jan 13, 2025
@bschoenmaeckers bschoenmaeckers force-pushed the datetime-convert branch 2 times, most recently from 7d9b9e6 to 54507d2 Compare January 13, 2025 09:20
@bschoenmaeckers bschoenmaeckers marked this pull request as ready for review January 13, 2025 09:21
Copy link

codspeed-hq bot commented Jan 13, 2025

CodSpeed Performance Report

Merging #20660 will not alter performance

Comparing bschoenmaeckers:datetime-convert (0a3fa09) with main (a0d96f2)

Summary

✅ 41 untouched benchmarks

@ritchie46
Copy link
Member

Thank you @bschoenmaeckers!

@ritchie46 ritchie46 merged commit e346f82 into pola-rs:main Jan 13, 2025
22 checks passed
@bschoenmaeckers bschoenmaeckers deleted the datetime-convert branch January 13, 2025 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance issues or improvements python Related to Python Polars
Projects
None yet
2 participants