-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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: Support writing Time
type in json
#21454
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21454 +/- ##
==========================================
+ Coverage 79.96% 79.99% +0.02%
==========================================
Files 1597 1598 +1
Lines 229115 229228 +113
Branches 2618 2620 +2
==========================================
+ Hits 183222 183381 +159
+ Misses 45294 45248 -46
Partials 599 599 ☔ View full report in Codecov by Sentry. |
Time
type in jsonTime
type in json
@@ -514,6 +537,33 @@ pub(crate) fn new_serializer<'a>( | |||
take, | |||
) | |||
}, | |||
ArrowDataType::Time32(tu) => { |
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.
We only use one of time32
/ time64
in Polars. (I don't know on top of head which one, but we only should implement and compile one)(
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, I wasn't sure of this since the Duration
writer above has branches for time units that we don't use. I'll remove time32.
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, I wasn't sure of this since the Duration writer above has branches for time units that we don't use.
Ah, then we can remove those as well. :) Can you do that?
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.
Yup. Drafting because I'm at work so no local CI so I'm relying on GitHub to help me lint/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.
Ok yeah this will have to wait until tonight.
406d90c
to
e8821c6
Compare
@ritchie46 I noticed that we have a few other functions in |
No, that's what's leftover from arrow2. We don't need it in Polars, so it can be removed. |
Closes #21452.
I also removed two small branches that aren't relevant to Polars:
ArrowDataType::Timestamp(s)
- this is ourDatetime
dtype, which only usesms
,us
, andns
, we have nos
implementation.ArrowDataType::Duration(s)
- same asDatetime
units described above.