-
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
Enh: adding automated inferencing of format %Y-%m-%dT%H:%M
in pyarrow
#1292
Conversation
awesome! @FBruzzesi fancy taking a look if you have time? (not urgent of course) |
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 a ton @raisadz ππΌ
It already looks good, yet I left a couple of opinionated comments π
Let me know what you think
narwhals/_arrow/utils.py
Outdated
format = "" | ||
matches = pc.extract_regex(arr, pattern=HMS_RE) | ||
if pc.all(matches.is_valid()).as_py(): | ||
format = "%H:%M:%S" | ||
else: | ||
matches = pc.extract_regex(arr, pattern=HM_RE) | ||
if pc.all(matches.is_valid()).as_py(): | ||
format = "%H:%M" | ||
return format |
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.
Opinionated take is to follow a similar pattern (pun unintended) as per date regex, i.e. having a format mapping:
TIME_FORMATS = (
(HMS_RE, "%H:%M:%S"),
(HM_RE, "%H:%M"),
)
and here loop through that and have a early return for the first that fully matches, otherwise return empty 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.
Thank you for the suggestion @FBruzzesi! I added a time formatting mapping similar to how it is done for dates, it looks much nicer now π
Co-authored-by: Francesco Bruzzesi <[email protected]>
β¦ow-date-parsing
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.
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.