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!: rewrite pl$date_range() and add pl$datetime_range() #950

Merged
merged 12 commits into from
Mar 21, 2024
Merged

Conversation

eitsupi
Copy link
Collaborator

@eitsupi eitsupi commented Mar 20, 2024

No description provided.

@eitsupi eitsupi changed the title wip rewrite pl$date_range() [skip ci] wip rewrite pl$date_range() Mar 20, 2024
@eitsupi eitsupi changed the title wip rewrite pl$date_range() wip feat!: rewrite pl$date_range() and add pl$datetime_range() Mar 20, 2024
@eitsupi eitsupi changed the title wip feat!: rewrite pl$date_range() and add pl$datetime_range() feat!: rewrite pl$date_range() and add pl$datetime_range() Mar 20, 2024
@eitsupi eitsupi marked this pull request as ready for review March 20, 2024 14:36
@eitsupi
Copy link
Collaborator Author

eitsupi commented Mar 21, 2024

@etiennebacher Please review.

Copy link
Collaborator

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

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

Thanks, I have a question on whether passing datetimes in pl$date_range() should be deprecated (as is in this PR) or removed.

NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
R/functions__eager.R Outdated Show resolved Hide resolved
R/functions__eager.R Outdated Show resolved Hide resolved
R/functions__eager.R Outdated Show resolved Hide resolved
#' Only takes effect if the output column is of type [Datetime][DataType_Datetime].
#' @return A [Expr][Expr_class] of data type Date or [Datetime][DataType_Datetime]
#' @section Interval:
#' `interval` is created according to the following string language:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we inherit that from another part of the docs? I think there's the same list elsewhere

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can't we inherit that from another part of the docs? I think there's the same list elsewhere

Sorry, but I don't understand what you want to say. This section is inherited by pl_datetime_range and there are no duplicates.
I don't think refactoring other functions' documentation is in the scope of this PR. (I don't want to make this PR huge anymore)

Copy link
Collaborator

Choose a reason for hiding this comment

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

My point was that Expr_rolling has almost the same thing in @details (but the first sentence is different so we can't just inherit it here)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I will update the docs in #958

R/functions__eager.R Outdated Show resolved Hide resolved
R/expr__datetime.R Outdated Show resolved Hide resolved
@eitsupi
Copy link
Collaborator Author

eitsupi commented Mar 21, 2024

Thanks, I have a question on whether passing datetimes in pl$date_range() should be deprecated (as is in this PR) or removed.

My understanding is that passing a Datetime is not deprecated, but rather that a breaking change will be introduced in the future that will return a Date even if a Datetime is passed.

Python has built-in logic to issue a warning when a Datetime is returned, but I haven't implemented it at the moment because it's cumbersome to copy.
https://github.com/pola-rs/polars/blob/103bb664b59acc4a9c48771f65ecc8f25b3bd303/py-polars/polars/functions/range/date_range.py#L335-L356

@etiennebacher
Copy link
Collaborator

etiennebacher commented Mar 21, 2024

My understanding is that passing a Datetime is not deprecated, but rather that a breaking change will be introduced in the future that will return a Date even if a Datetime is passed.

Returning a Datetime in pl$date_range() has been deprecated since py-polars 0.19.3, I think we can safely assume it's gonna be removed in 1.0.0. This PR is a big refactor of pl$date_range() with several other breaks, so to me we should use this opportunity to implement this breaking change. Also, deprecating without warning doesn't help users, so if we end up deprecating this usage there should be a message.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Mar 21, 2024

We can't make this breaking change here because no changes have been introduced to the upstream Rust side.

@eitsupi eitsupi marked this pull request as draft March 21, 2024 12:35
@eitsupi eitsupi marked this pull request as ready for review March 21, 2024 13:16
@eitsupi
Copy link
Collaborator Author

eitsupi commented Mar 21, 2024

I have added warnings.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Mar 21, 2024

Although I wrote that breaking changes cannot be made now, the time_unit and time_zone arguments can be removed.

However, even if we do that, we can't delete the the warnings, so I think it's fine as is now.

Copy link
Collaborator

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

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

Thanks!

@eitsupi eitsupi merged commit e98d499 into main Mar 21, 2024
33 checks passed
@eitsupi eitsupi deleted the date-range branch March 21, 2024 13:38
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.

None yet

2 participants