-
Notifications
You must be signed in to change notification settings - Fork 124
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: pyarrow automated inference of format %Y%m%d%H%M%S
#1326
feat: pyarrow automated inference of format %Y%m%d%H%M%S
#1326
Conversation
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.
Thank you for this! I just have a minor comment below, but it looks already great
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.
Thanks @raisadz !
I didn't know that polars does not detect this format automatically. Since that's the case, I feel a bit hesitant to add it honestly.
Maybe let's sleep a bit on this and wait for Marco weighting on it
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.
thanks for your PR (and @EdAbati + @FBruzzesi for reviews)
just got one really minor comment
regarding Polars, the datetime inference there is just based on trying out a bunch of hardcoded formats, and is probably ripe for being rewritten anyway
so i think we might as well go ahead with this
narwhals/_arrow/utils.py
Outdated
HMS_RE = r"^(?P<hms>\d{2}:\d{2}:\d{2})$" | ||
HM_RE = r"^(?P<hm>\d{2}:\d{2})$" | ||
HMS_RE_NO_SEP = r"^(?P<hmsns>\d{6})$" |
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.
"ns" reads to me as "nanoseconds" here - maybe we can just write hms_no_sep
?
What type of PR is this? (check all applicable)
Related issues
Checklist
If you have comments or can explain your changes, please do so below.