-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
API: ArrowDtype.type #51307
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
API: ArrowDtype.type #51307
Conversation
pandas/core/arrays/arrow/dtype.py
Outdated
@@ -90,7 +100,36 @@ def type(self): | |||
""" | |||
Returns pyarrow.DataType. |
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 docstring will need updating
So this is breaking the
In the relevant case item is np.nan. This PR makes it so that the |
if pa_type.unit == "ns": | ||
return Timestamp | ||
else: | ||
return datetime |
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.
(not for this PR, just noting from seeing the diff)
FWIW, now Timestamp supports multiple resolutions, we should make it return a Timestamp for all cases, I think (it's something we should also do on the pyarrow side, but on the short term it will be easier done on the pandas side to ensure consistent behaviour across pyarrow versions)
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.
LGTM
Thanks @jbrockmendel |
cc @mroeschke we discussed this one of the pyarrow testing PRs.
Still has some failing
pandas/tests/extension/test_arrow.py::TestBaseInterface::test_contains
tests where eyeballs would be welcome.ATM this returns Timestamp/Timedelta when the pyarrow unit is "ns" and pydatetime/pydatetime otherwise. We could reasonably update that to be always-Timestamp/Timedelta.