-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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, python): add dt.combine
for combining date and time components
#6121
feat(rust, python): add dt.combine
for combining date and time components
#6121
Conversation
8370ab7
to
cc179c2
Compare
What if we just make |
Truthfully I don't like that; it has a large conceptual overhang with "datetime + duration", but it really isn't the same. For instance, it assumes that "date + time" is equivalent to "datetime at 00:00:00 + time.cast(duration)", but a "date + (duration of less than 1 day)" is actually a no-op (because that's how date type duration offsets work - the minimum change has to be +/- 1 day), eg: >>> df = pl.DataFrame( {"x":[date(2020,1,2)]} )
>>> df.select( pl.col("x") + timedelta(hours=23) ).item()
date( 2020,1,2 ) # << no change So it's a syntax that looks simple at first glance but it opens you up to ambiguity and "well *this* behaves one way, but *that* behaves another way, but they both sort of look the same?" scenarios. Having the functionality sit behind a dedicated / discoverable method isolates you from all of that (including any potential for extra contextual spaghetti lower down in the codebase). Plus... it's explicit rather than being a bit "magic" :) (Also the "datetime.combine(time)" behaviour whereby the new time part replaces the existing time part wouldn't be possible with a "+" syntax; there may also be some scope for additional params later). Not sure how |
Alright. Shall I setup the skeleton for the rust expression so we can move it down? |
Sounds like a plan :) |
Had another crack at it after a night's sleep; feels like I'm about 90% the way there, with (I think) only one last issue blocking me from actually testing it. Specifically, when registering the function in #[derive(Clone, PartialEq, Debug, Eq, Hash)]
pub enum TemporalFunction {
....
Combine(Series, TimeUnit),
...
} ...given the following function signature: pub(super) fn combine(s: &Series, tm: &Series, tu: TimeUnit) -> PolarsResult<Series> { (The new I've got all the rest of the plumbing in place, and an implementation, but it doesn't like What do you think might be the best way to handle this? (Assuming I didn't already jump the shark by having |
I just ran into a case where I'd need this. Hope this will be ready soon-ish 😸 |
I think it's close, if I can understand how best to handle the macro expansion issue with Series (the python version works, but really it should be moved down). @ritchie46, I think I need a few gems of your Rust wisdom here! 😅 |
Ok, I will take a look at this one tomorrow (if the bugs allow me). |
Bugs come first! Whenever you've actually got a spare moment is fine... ;) |
Exactly, I wasn't trying to rush you! I am just using the underlying logic implemented by @alexander-beedie in this PR and it works fine. I just thought it was interesting that I bumped into this while there's a PR open 😄 |
89015fd
to
ec548c9
Compare
dt.combine
for combining date and time components dt.combine
for combining date and time components
Background:
Saw two references to this not being straightforward recently, once in the shiny new Modern Polars guide...
...and again in the Discord:
Naming:
Follows the equivalent method associated with python datetimes:
Example:
If the underlying expression is a
Datetime
then itsTime
component is replaced, and if it is aDate
then a newDatetime
is created by combining the two components as-is.Next:
This is only exposed via Python at the moment, but I'd definitely like to move it down into Rust; I had a quick look at doing so, but there was rather more indirection than I recall, so I thought I'd get it working here first and then move it down.
@ritchie46 - if you can suggest looking at an existing function that's at least vaguely similar in approach to this one, I'm sure I can work it out...🤣