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

fix(rust, python): fix result dtype in date_range(..., eager=True) if duration contains "1s1d" #9670

Merged
merged 2 commits into from
Jul 4, 2023

Conversation

MarcoGorelli
Copy link
Collaborator

@MarcoGorelli MarcoGorelli commented Jul 1, 2023

On the way towards #9019

closes #9665

ideally, to be rebased onto #8591

Sorry for all the tests, my hope is that extensive tests now will mean fewer regression tests in the future 🤞


Summary:

Instead of doing

if not eager:
    <use date_range_lazy>
else:
    <use date_range_eager>

I'm doing

result = <use date_range_lazy>
if not eager:
    return result
return F.select(result).explode().set_sorted()

In order for that to work, I've had to align the lazy and eager paths a bit more. For example, in the eager mode, currently, a date range of dates with sub-daily precision results in a date_range of datetimes:

if (
start_is_date
and end_is_date
and not _interval_granularity(interval).endswith(("h", "m", "s"))
):
dt_range = dt_range.cast(Date)

That logic was not present in the lazy date range, so I've added it.

The extra .explode really makes the issue raised in #9019 glaringly obvious - but this is a first step towards resolving it!
Also - this started off as an attempt to refactor, the fact that it picked up (and ended up solving) #9665 was just a fortunate find along the way 😄

@github-actions github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars labels Jul 1, 2023
@MarcoGorelli MarcoGorelli marked this pull request as ready for review July 2, 2023 11:50
@MarcoGorelli MarcoGorelli force-pushed the can-we-simplify-date-range branch from a97fd59 to e91bd65 Compare July 2, 2023 12:21
@ritchie46
Copy link
Member

Also - this started off as an attempt to refactor, the fact that it picked up (and ended up solving) #9665 was just a fortunate find along the way smile

Finders luck! ^^

@ritchie46 ritchie46 merged commit 1d06aa7 into pola-rs:main Jul 4, 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
fix Bug fix python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

date_range(..., eager=True) output dtype depends on order of inputs in duration string
2 participants