Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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): utf8 to temporal casting #10517
feat(rust): utf8 to temporal casting #10517
Changes from 12 commits
869fa1d
bb73153
ca7c105
34ba75a
8be15bd
901ab3a
de17db6
aea80f4
20a3991
7efc54e
8589836
6e886f9
5d48cc8
17e1402
befe308
084a7e1
0cfce61
6f25831
2c6c9bd
6b929f2
dcec1e8
ef503c3
4fb3f07
d00a432
7983724
4476fbd
8463def
89ffd88
a507d67
003ca4d
32e7a24
f63014e
d85c452
8d29d3c
00082c5
45009eb
d24c508
f5f3fa9
d6ef2e4
c3e1c1e
9ea46ef
46e7009
34d42c6
89cc1e2
28a99f6
a42185f
a05b298
d39c360
f41e8f4
dfbc5f4
cd0288e
21e2c0c
65659b9
754067c
2fea820
570dca7
6cf037b
eac03a2
c8cfdee
e21c1a7
c2562d8
a7fdbee
0b8be40
d1af5f9
7b9f10e
27a4fe2
d31c30d
106dce8
f509de9
d9f2f5f
1a0c174
6dae550
cb92cb8
0d9f865
d9c6316
fada98b
5425f6a
c69722d
eb469b4
fe04f4a
d847f69
c5459f1
6a1731e
a75eaca
96b465e
6155e7f
ff358ca
6e8ce9c
3251703
8af94b0
5e96abd
492a3c1
04357ef
3386abd
bce21ec
4b08b6b
013916a
dab1451
374d946
68f9d69
8e810ff
5225444
fa5a0e1
30dcdc3
215990b
20f94d9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was
pl.Utf8
removed here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-01I 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):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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, I understand that there is a lot of work here to be done and also life happens. Will make the changes. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, good one for splitting this out into a separate test!