feat(rust): utf8 to temporal casting #10517

brayanjuls · 2023-08-15T22:57:23Z

Closes #9994 , closes #10478

The implementation I did in this PR support converting strings to date or datetime, it only support specific formats which are the following:

%Y-%m-%d
%Y-%m-%dT%H:%M:%S
%Y-%m-%dT%H:%M:%S.3f
%Y-%m-%dT%H:%M:%S%.f%:z

Additionally:

~~I found that timestamp casting for time-unit milliseconds and microseconds is not implemented in arrow2 library, so I opened this issue.~~ This is now implemented in nano-arrow crate inside polars.
Converting date times with the following format "2021-01-01 12:54:58.432" or "2021-01-01 12:54:58" is not supported by nano-arrow cast function, it only supports RFC3339(or a variation of it), should the other formats be supported as well ? if so I think we should open a different issue for that.
Converting a string number to date is not supported, ie: pl.DataFrame({"x1":["1234"]}).with_columns(**{ "x1-date": pl.col("x1").cast(pl.Date)}), this is why I removed pl.Utf8 from test_from_epoch unit test, should it be supported?

Example

import polars as pl

df = pl.DataFrame({
    "x1": ["2021-01-01"], 
    "x2": ["2021-01-01T12:54:58"],
    "x3": ["2021-01-01T12:54:58.432"],
    "x4": ["2021-01-01T12:54:58.432 +00:00"],
}).with_columns(**{
    "x1-date":pl.col("x1").cast(pl.Date),
    "x2-datetime":pl.col("x2").cast(pl.Datetime),
    "x3-datetime":pl.col("x3").cast(pl.Datetime),
    "x4-datetime-tz":pl.col("x3").cast(pl.Datetime("ms","America/Santiago")),
})

stinodego

Thanks for the PR! I have a few comments already - will look at implementation later.

stinodego · 2023-08-17T11:41:12Z

py-polars/tests/unit/test_lazy.py

@@ -1313,7 +1313,7 @@ def test_quadratic_behavior_4736() -> None:
    ldf.select(reduce(add, (pl.col(fld) for fld in ldf.columns)))


-@pytest.mark.parametrize("input_dtype", [pl.Utf8, pl.Int64, pl.Float64])


Why was pl.Utf8 removed here?

This pl.utf8 is to test that the epoch string behavior works, but I think it should not be supported as it is a string not following date/datetime format. ie: pl.DataFrame({"x1":["1234"]}).with_columns(**{ "x1-date": pl.col("x1").cast(pl.Date)})

Should we keep the support for epoch string?

I added it back.

Are we sure this is meant to work?

To be honest it feels a bit strange to have that pl.Series([<string>]).cast(pl.Date) could be interpreted as both a format string, or as the number of days since 1970-01-01

I find it hard to believe that anyone would store a timestamp as a string - if they're trying to cast a string to Date, it's almost certainly a date string (and if not, I think it's OK to let the user cast to Int32 first):

# this looks odd to me In [9]: print(pl.Series(['1234']).cast(pl.Date)) shape: (1,) Series: '' [date] [ 1973-05-19 ]

My suggestion would be to remove Utf8 from here, treat this as a bug fix, and not double-traverse the array with is_parsable_as_number - @stinodego thoughts?

I completely agree, that's why I removed it in the first place but got no feedback on my answer so I put it back, also there is an open issue where this behavior is reported as a bug, #10478

nice, thanks - ok maybe let's add that as a test case and check and closes #10478 to the description?

and could we wait for Stijn's response before marking this as resolved please? thanks 🙏

finally, sorry for having been slow on this PR, been on holiday recently

Apologies, haven't been able to get back to this one. Agree with what you guys are saying - casting "1234" to a Date should fail.

No worries, I understand that there is a lot of work here to be done and also life happens. Will make the changes. Thanks.

py-polars/tests/unit/test_queries.py

crates/polars-core/src/chunked_array/cast.rs

# Conflicts: # py-polars/tests/unit/test_queries.py

Support for different timeunits added in nano-arrow

…rom int to date.

# Conflicts: # crates/polars-arrow/src/compute/cast/utf8_to.rs # crates/polars-arrow/src/temporal_conversions.rs

…ressions (pola-rs#11768)

Co-authored-by: Weijie Guo <[email protected]>

…ame validation on unit tests

…a-rs#11784)

MarcoGorelli

Right, looks solid now, thanks for persevering with this! If you rebase onto the latest main, the tests should pass

I don't want to block this PR any further, but if I you wanted to do a follow-up (in a separate PR, after this one) to use time_unit instead of tu and time_zone instead of tz in these files, that would be welcome

MarcoGorelli · 2023-10-21T12:44:29Z

py-polars/tests/unit/test_lazy.py

@@ -1415,6 +1415,23 @@ def test_from_epoch(input_dtype: pl.PolarsDataType) -> None:
        _ = ldf.select(pl.from_epoch(ts_col, time_unit="s2"))  # type: ignore[call-overload]


+def test_from_epoch_str() -> None:


nice, good one for splitting this out into a separate test!

brayanjuls · 2023-10-21T14:42:53Z

Right, looks solid now, thanks for persevering with this! If you rebase onto the latest main, the tests should pass

I don't want to block this PR any further, but if I you wanted to do a follow-up (in a separate PR, after this one) to use time_unit instead of tu and time_zone instead of tz in these files, that would be welcome

Sure! I will do the rebase as well as fixing the naming.

Support for different timeunits added in nano-arrow

…rom int to date.

…ame validation on unit tests

…n matching on timestamp casting

… string to datetime.

brayanjuls · 2023-10-21T15:10:20Z

not sure what to make of the test failure
@alexander-beedie do you know?

I do! And a rebase will fix it, heh - not an issue with this code 😄 #11910 (comment)

Just for curiosity how can I run hypothesis test locally ? I tried using pytest to the specific file but they do not run.

…-to-temporal-cast # Conflicts: # crates/polars-arrow/src/compute/cast/mod.rs # crates/polars-arrow/src/compute/cast/utf8_to.rs # crates/polars-core/src/chunked_array/cast.rs

MarcoGorelli · 2023-10-21T18:39:41Z

you'll need to also pass -m 'not slow' on the command line

alexander-beedie · 2023-10-21T20:56:50Z

you'll need to also pass -m 'not slow' on the command line

Yup; I tend to run with a blanket -m "" to make sure I always run everything locally :)

MarcoGorelli

" Files changed 247"

🤔 something may have gone wrong when rebasing

brayanjuls · 2023-10-23T02:52:57Z

" Files changed 247"

🤔 something may have gone wrong when rebasing

Ohh not I did not saw that when pushed the changes. I am sorry. I am going to do a hard reset to the commit previous to the rebase, and I will try to make the rebase again after that.

…nto utf8-to-temporal-cast" This reverts commit 215990b, reversing changes made to c5459f1.

brayanjuls · 2023-10-24T04:42:50Z

I am still trying to make this merge work out, I did a revert but just realized that it keeps history so I cannot redo the merge from main because previous changes will go unnoticed. I want to avoid doing hard reset and then push because I don't want to hurt changes. Any Idea on what to do at this point?

I can do rever-reverted but that will only leave me with the rebase that went wrong.

MarcoGorelli · 2023-10-24T07:26:38Z

if I were you I'd open a new PR with just a single commit

brayanjuls · 2023-10-27T13:07:35Z

I just opened a new PR #12072, I will close this one.

feat(rust): utf8 to temporal casting

869fa1d

brayanjuls requested review from ritchie46, stinodego, alexander-beedie and orlp as code owners August 15, 2023 22:57

github-actions bot added enhancement New feature or an improvement of an existing feature rust Related to Rust Polars labels Aug 15, 2023

feat(rust): utf8 to temporal casting

bb73153

stinodego requested changes Aug 17, 2023

View reviewed changes

MarcoGorelli requested changes Aug 17, 2023

View reviewed changes

crates/polars-core/src/chunked_array/cast.rs Outdated Show resolved Hide resolved

cmdlineluser mentioned this pull request Sep 25, 2023

A DataFrame.astype method to cast columns #5758

Closed

cmdlineluser mentioned this pull request Oct 5, 2023

feat(python,rust,cli): add DATE function for SQL #11541

Merged

brayanjuls and others added 14 commits October 9, 2023 22:27

Merge branch 'main' into utf8-to-temporal-cast

ca7c105

# Conflicts: # py-polars/tests/unit/test_queries.py

feat: utf8 to timestamp/date casting support

34ba75a

Support for different timeunits added in nano-arrow

feat: added missing tests for failure scenarios, also fixed casting f…

8be15bd

…rom int to date.

Merge branch 'main' into utf8-to-temporal-cast

901ab3a

# Conflicts: # crates/polars-arrow/src/compute/cast/utf8_to.rs # crates/polars-arrow/src/temporal_conversions.rs

fix: fixed issue regarding arrow libraries import and code formatting

de17db6

fix(rust,python): only exclude final output names of group_by key exp…

aea80f4

…ressions (pola-rs#11768)

depr(python): Rename group_by_rolling to rolling (pola-rs#11761)

20a3991

refactor(rust): Make all emw function expr non-anonymous (pola-rs#11638)

7efc54e

Co-authored-by: Weijie Guo <[email protected]>

feat(python,rust,cli): add DATE function for SQL (pola-rs#11541)

8589836

chore: more granular polars-ops imports (pola-rs#11760)

6e886f9

feat(python): primitive kwargs in plugins (pola-rs#11268)

5d48cc8

fix: fixed validate_is_number import issue, also added missing datafr…

17e1402

…ame validation on unit tests

refactor(python): Rename IntegralType to IntegerType (pola-rs#11773)

befe308

fix: fixed linter issues.

084a7e1

brayanjuls requested review from stinodego and MarcoGorelli October 16, 2023 22:59

reswqa and others added 2 commits October 17, 2023 08:19

chore(rust): Move cum_agg to polars-ops (pola-rs#11770)

0cfce61

depr(python): Deprecate use_pyarrow param for Series.to_list (pol…

6f25831

…a-rs#11784)

MarcoGorelli approved these changes Oct 21, 2023

View reviewed changes

stinodego and others added 2 commits October 21, 2023 15:55

fix(python): Fix Array data type initialization (pola-rs#11907)

492a3c1

docs(python): Fix docstring for diff methods (pola-rs#11921)

04357ef

brayanjuls added 10 commits October 21, 2023 11:48

feat(rust): utf8 to temporal casting

3386abd

feat(rust): utf8 to temporal casting

bce21ec

feat: utf8 to timestamp/date casting support

4b08b6b

Support for different timeunits added in nano-arrow

feat: added missing tests for failure scenarios, also fixed casting f…

013916a

…rom int to date.

fix: fixed issue regarding arrow libraries import and code formatting

dab1451

fix: fixed validate_is_number import issue, also added missing datafr…

374d946

…ame validation on unit tests

fix: fixed linter issues.

68f9d69

fix: removing additional asserts from unit test, also improved patter…

8e810ff

…n matching on timestamp casting

fix: fixed the bug that incorrectly enabled the conversion from epoch…

5225444

… string to datetime.

fix: removed unused import

fa5a0e1

brayanjuls added 2 commits October 21, 2023 13:14

fix: fixed variable naming from tu to time_unit and from tz to time_zone

30dcdc3

Merge remote-tracking branch 'origin/utf8-to-temporal-cast' into utf8…

215990b

…-to-temporal-cast # Conflicts: # crates/polars-arrow/src/compute/cast/mod.rs # crates/polars-arrow/src/compute/cast/utf8_to.rs # crates/polars-core/src/chunked_array/cast.rs

brayanjuls requested review from c-peters and universalmind303 as code owners October 21, 2023 16:24

MarcoGorelli requested changes Oct 22, 2023

View reviewed changes

Revert "Merge remote-tracking branch 'origin/utf8-to-temporal-cast' i…

20f94d9

…nto utf8-to-temporal-cast" This reverts commit 215990b, reversing changes made to c5459f1.

cmdlineluser mentioned this pull request Oct 25, 2023

Additional info regarding the questions raised in your article. danielbeach/PolarsDateTimeManipulation#1

Open

brayanjuls mentioned this pull request Oct 27, 2023

feat(rust): casting utf8 to temporal #12072

Merged

brayanjuls closed this Oct 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rust): utf8 to temporal casting #10517

feat(rust): utf8 to temporal casting #10517

brayanjuls commented Aug 15, 2023 •

edited by stinodego

Loading

stinodego left a comment

stinodego Aug 17, 2023

brayanjuls Aug 17, 2023

brayanjuls Oct 16, 2023

MarcoGorelli Oct 20, 2023

brayanjuls Oct 20, 2023

MarcoGorelli Oct 20, 2023

stinodego Oct 20, 2023 •

edited

Loading

brayanjuls Oct 20, 2023

MarcoGorelli left a comment

MarcoGorelli Oct 21, 2023

brayanjuls commented Oct 21, 2023

brayanjuls commented Oct 21, 2023

MarcoGorelli commented Oct 21, 2023

alexander-beedie commented Oct 21, 2023 •

edited

Loading

MarcoGorelli left a comment

brayanjuls commented Oct 23, 2023

brayanjuls commented Oct 24, 2023

MarcoGorelli commented Oct 24, 2023

brayanjuls commented Oct 27, 2023

		@@ -1313,7 +1313,7 @@ def test_quadratic_behavior_4736() -> None:
		ldf.select(reduce(add, (pl.col(fld) for fld in ldf.columns)))


		@pytest.mark.parametrize("input_dtype", [pl.Utf8, pl.Int64, pl.Float64])

		@@ -1415,6 +1415,23 @@ def test_from_epoch(input_dtype: pl.PolarsDataType) -> None:
		_ = ldf.select(pl.from_epoch(ts_col, time_unit="s2")) # type: ignore[call-overload]


		def test_from_epoch_str() -> None:

feat(rust): utf8 to temporal casting #10517

feat(rust): utf8 to temporal casting #10517

Conversation

brayanjuls commented Aug 15, 2023 • edited by stinodego Loading

Example

stinodego left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stinodego Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoGorelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brayanjuls commented Oct 21, 2023

brayanjuls commented Oct 21, 2023

MarcoGorelli commented Oct 21, 2023

alexander-beedie commented Oct 21, 2023 • edited Loading

MarcoGorelli left a comment

Choose a reason for hiding this comment

brayanjuls commented Oct 23, 2023

brayanjuls commented Oct 24, 2023

MarcoGorelli commented Oct 24, 2023

brayanjuls commented Oct 27, 2023

brayanjuls commented Aug 15, 2023 •

edited by stinodego

Loading

stinodego Oct 20, 2023 •

edited

Loading

alexander-beedie commented Oct 21, 2023 •

edited

Loading