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, python)!: consistently return list of date/datetime from lazy date_range #8513

Merged
merged 3 commits into from
May 24, 2023

Conversation

MarcoGorelli
Copy link
Collaborator

@MarcoGorelli MarcoGorelli commented Apr 26, 2023

closes #7110

This simplifies the logic, but would be a breaking change, so if accepted, might be better to wait for 0.18 or 1.0?

It also simplifies the logic on the user side (I think) - in all the test cases I found where this is used, it removes an unnecessary .implode call

If reviewing, this is much easier if ignoring whitespace: https://github.com/pola-rs/polars/pull/8513/files?w=1

@github-actions github-actions bot added breaking Change that breaks backwards compatibility enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars labels Apr 26, 2023
@ritchie46
Copy link
Member

I think we should also take this breaking opportunity to change the default lazy=False to eager=False as this will have most merit in the expression API anyway.

Other than that I agree with these changes. Having a dynamic output type dependent on data size was clearly bad design.

@MarcoGorelli
Copy link
Collaborator Author

sounds good - as a separate PR, right?

will also need to address #8512 - preferably after this PR but before the release, as this PR reduces complexity whereas addressing #8512 would increase it

@ritchie46
Copy link
Member

sounds good - as a separate PR, right?

Yeap, that's best it seems.

@MarcoGorelli MarcoGorelli marked this pull request as draft May 23, 2023 12:07
@MarcoGorelli MarcoGorelli marked this pull request as ready for review May 23, 2023 12:50
@@ -727,6 +754,7 @@ def test_time_range_lit() -> None:
name="tm",
)
)
tm = tm.select(pl.col("tm").explode())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

here start is an expression, so it behaves like in the lazy manner

if (
not eager
or isinstance(start, (str, pl.Expr))
or isinstance(end, (str, pl.Expr))
):

@ritchie46
Copy link
Member

Thanks. Good that we have a consistent output type now.

1 similar comment
@ritchie46
Copy link
Member

Thanks. Good that we have a consistent output type now.

alexander-beedie pushed a commit to alexander-beedie/polars that referenced this pull request May 23, 2023
alexander-beedie pushed a commit to alexander-beedie/polars that referenced this pull request May 23, 2023
@ritchie46 ritchie46 merged commit 2c4dc54 into pola-rs:main May 24, 2023
c-peters pushed a commit to c-peters/polars that referenced this pull request Jul 14, 2023
c-peters pushed a commit to c-peters/polars that referenced this pull request Jul 14, 2023
c-peters pushed a commit to c-peters/polars that referenced this pull request Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Change that breaks backwards compatibility enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected behavior when creating a new Series column with pl.date_range
3 participants