GH-42018: Add numpy.StringDType support #48391
Conversation
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
alippai
changed the title
|
|
|
I have no idea if this is the way to implement numpy compat in arrow. Also I’ll add a better test case for the na_value param. |
Did you review the code to ensure it was/looked correct? |
|
@pitrou yes! I don’t work with C++ code professionally, so I lack the knowledge to know if eg the numpy 2.0+ feature usage is actually good or ridiculous. This was my first attempt and experiment with using AI for OSS development. The issue turned out to be more complex than I initially expected. The PR is still small enough, so I’d appreciate the feedback if it’s good direction and I totally understand if you say this is simply not reviewable and would waste the time of the participants. |
|
I might try to take a look but I will first have to get acquainted with the StringDType. |
|
@jorisvandenbossche might have insight. |
|
cc @ngoldbaum |
| inline npy_string_allocator* ArrowNpyString_acquire_allocator( | ||
| const PyArray_StringDTypeObject* descr) { | ||
| using Func = npy_string_allocator* (*)(const PyArray_StringDTypeObject*); | ||
| auto func = reinterpret_cast<Func>(PyArray_API[316]); |
There was a problem hiding this comment.
Are these functions exposed or are they only accessible through the PyArray_API?
There was a problem hiding this comment.
This is a reimplementation of the public api as the numpy api version used in pyarrow was below <2.0 and these were “hidden”. This way the code compiles with both old and new numpy. I didn’t find a better example in pyarrow how to onboard new API. The alternative is dropping the numpy 1.26 support (which might be allowed if pyarrow follows SPEC 0)
There was a problem hiding this comment.
The idea is unfortunately right, I think, since you can't put this in a different compilation unit with NPY_TARGET_VERSION=NPY_2_0_API_VERSION and you can't switch NPY_TARGET_VERSION within the same compilation unit.
Maybe for these specific functions that was actually unnecessary (and if you/@ngoldbaum likes, we could make them always defined in 2.5, it would just segfault if you ever use it, but we/I missed that it is impossible for there to be something to use it on).
That way, it might only be available if build with NumPy 2.5, but that is probably OK in practice, since that is what official builds will use (except maybe for some old Python version).
That said NPY_ABI_VERSION >= 0x02000000 isn't good here. If anything it should be NPY_FEATURE_VERSION (which you can enforce via NPY_TARGET_VERSION to indicate a minimal version of NumPy you support at runtime).
While think it's correct to hack it in this vain, I would suggest to guard it in a way that uses the custom version only when necessary and makes it obvious how to clean it up in the future.
(Even just copy-paste the C definitions with a #ifndef ...!?)
If NumPy offers the required defines (which it will at least on newer versions or with NPY_TARGET_VERSION=NPY_2_0_API_VERSION it is better to stop using it, there is no guarantee it will remain correct for ever.
There was a problem hiding this comment.
Removed most of the guards. As I understand the feature and target versions could be lower, but correct me if I’m wrong, please
github-actions
bot
added
awaiting committer review
|
I can take a look at this but I'll keep in mind an AI wrote it... |
| Status NumPyConverter::AppendStringDTypeValues(Builder* builder) { | ||
| auto* descr = reinterpret_cast<PyArray_StringDTypeObject*>(dtype_); | ||
|
|
||
| npy_string_allocator* allocator = ArrowNpyString_acquire_allocator(descr); |
There was a problem hiding this comment.
FYI for other reviewers: this locks a mutex internally in NumPy.
|
@ngoldbaum thanks for the review. I’ll address them as soon as I figure out what to do with the numpy versions. Also I appreciate the open minded perspective for the AI, I did my best to only submit something what works and doesn’t have unnecessary code. |
6 participants
Rationale for this change
Implement #42018
What changes are included in this PR?
Conversion in numpy->arrow direction with multiple string types
Are these changes tested?
Two basic conversion tests added
Are there any user-facing changes?
Yes, adds support to numpy.StringDType as source
I'm not sure what the AI policy is for apache/arrow, this PR was created using OpenAI Codex.
cc @jorisvandenbossche as he opened the original issue