-
-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
String dtype: map builtin str alias to StringDtype #59685
base: main
Are you sure you want to change the base?
Changes from 12 commits
f444105
630d41c
d127770
38d011a
c4ed9d3
cad7d8f
189e26d
1089eb3
8f7e968
51900f1
15f45d2
4464fb1
650f694
49297f0
9164dbb
cf9f855
bd79fc9
4c775d1
3b0b779
b0276b2
d413fc6
d634da2
e791330
db8900c
e6aad17
4e6cf04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,7 +184,7 @@ def test_construct_from_string_fill_value_raises(string): | |
[ | ||
(SparseDtype(int, 0), float, SparseDtype(float, 0.0)), | ||
(SparseDtype(int, 1), float, SparseDtype(float, 1.0)), | ||
(SparseDtype(int, 1), str, SparseDtype(object, "1")), | ||
(SparseDtype(int, 1), np.str_, SparseDtype(object, "1")), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we even need this test case? I'm not sure what expectations we have around the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, can also leave out this test case entirely. I am not entirely sure yet what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd be fine to remove; I feel like this opens up a pandora's box of issues that aren't worth tracking down at this point in time |
||
(SparseDtype(float, 1.5), int, SparseDtype(int, 1)), | ||
], | ||
) | ||
|
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 is a bit ugly, but essentially this is avoiding that
astype(str)
for ArrowExtensionArray (if that happens to have a datetimelike dtype) is called here, because that will useensure_string_array
for that implementation, causing a recursion error.An alternative would be a better check than
arr.dtype.kind in "mM"
that restricts itself toDatetimeLikeArrayMixin
(essentially I need aisinstance(arr, DatetimeLikeArrayMixin)
, I think). Could potentially do that with some ABC check.Another alternative is to handle
astype(str)
specifically in ArrowExtensionArray (currentlyastype
is not implemented there, and it falls back entirely on the base class implementation)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 think eventually it would be good to implement
ArrowExtensionArray.astype
so I would support eventually handingstr
there. Could you add aTODO
comment here describing when this could be removed whenastype
is implemented?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.
Added a TODO comment