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

feat(rust): Support duration + date #11190

Merged
merged 3 commits into from
Sep 21, 2023

Conversation

billylanchantin
Copy link
Contributor

Description

Closes: #11034

This PR adds support for duration + date. Supporting that operation will bring the Rust API more in line with the Python API, which already supports it.

Changes

  • Support duration + date
  • Tests

@github-actions github-actions bot added enhancement New feature or an improvement of an existing feature rust Related to Rust Polars labels Sep 19, 2023
@ritchie46
Copy link
Member

Python API, which already supports it.

I don't understand. How can the python API already support it in this case? Shouldn't we go to both code branches for both apis?

@billylanchantin
Copy link
Contributor Author

@alexander-beedie posted an example in the issue:

#11034 (comment)

TBH I was surprised that the Python side already supported it. I think what's happening is that PySeries ends up calling try_add instead of just add. And try_add seems to coerce the arguments.

I think the relevant code is here (but I had a little trouble confirming it):

https://github.com/pola-rs/polars/blob/main/crates/polars-core/src/series/arithmetic/borrowed.rs#L340-L347

@alexander-beedie
Copy link
Collaborator

alexander-beedie commented Sep 19, 2023

Yup; the underlying PySeries add is called from the Python side (via _arithmetic), and that then kicks out to try_add.

On the Rust side we have precedence from chrono not liking "duration + date", but on the Python side the standard library is completely fine with "timedelta + date", and we seem to have (possibly inadvertently) allowed it via the Python Series for some time so it might be a bit harsh to take it away!

I'm not going to declare which one is "best", but it would be good to align both sides of the API, and the weight of things appears to support allowing it, so... ;)

@billylanchantin
Copy link
Contributor Author

Also, there was already support for duration + datetime on the Rust side despite chrono not being a fan.

@ritchie46
Copy link
Member

Alright, thanks for the clarification. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or an improvement of an existing feature rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

date + duration is supported while duration + date is not
3 participants