GH-50398 [Python] Use more CPython Limited APIs in python/pyarrow - #50409
GH-50398 [Python] Use more CPython Limited APIs in python/pyarrow#50409mroeschke wants to merge 18 commits into
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: |
mroeschke
changed the title
|
|
There was a problem hiding this comment.
Pull request overview
This PR refactors PyArrow’s C/Cython extension code to reduce reliance on CPython internal struct-field access and macro APIs, moving toward wider use of CPython Limited API-compatible functions (as part of the long-term goal of supporting abi3 wheels).
Changes:
- Introduces
internal::PyObject_StdStringTypeName()(based onPyType_GetName) and switches multiple error paths to use it instead of readingPy_TYPE(obj)->tp_name. - Replaces several CPython macros / struct accesses with function-based APIs (e.g.,
PyFloat_AsDouble,PyTuple_GetItem,PyList_SetItem,PyBytes_Size/AsString,PyType_GetSlot,PyType_GetFlags). - Switches the
MonthDayNanostruct-sequence type initialization to a cachedPyStructSequence_NewType()pattern.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| python/pyarrow/src/arrow/python/udf.cc | Use limited-API helper for Python type names in error messages. |
| python/pyarrow/src/arrow/python/python_to_arrow.cc | Replace macros with function forms for float/tuple/list APIs. |
| python/pyarrow/src/arrow/python/pyarrow.cc | Use limited-API helper for type names in unwrap error messages. |
| python/pyarrow/src/arrow/python/numpy_to_arrow.cc | Replace bytes/tuple macros with function APIs. |
| python/pyarrow/src/arrow/python/io.cc | Use limited-API helper for type names in IO error messages. |
| python/pyarrow/src/arrow/python/inference.cc | Use limited-API helper for dict-key type name error messages. |
| python/pyarrow/src/arrow/python/helpers.h | Declare the new PyObject_StdStringTypeName() helper. |
| python/pyarrow/src/arrow/python/helpers.cc | Implement type-name helper; switch to PyType_GetSlot / PyType_GetFlags. |
| python/pyarrow/src/arrow/python/extension_type.cc | Use limited-API helper for extension instance type name in ToString(). |
| python/pyarrow/src/arrow/python/decimal.cc | Use limited-API helper for type names in decimal conversion errors. |
| python/pyarrow/src/arrow/python/datetime.cc | Switch MonthDayNano struct-sequence init to cached PyStructSequence_NewType. |
| python/pyarrow/src/arrow/python/common.h | Include helpers and replace bytes macro access; improve type-name errors. |
| python/pyarrow/src/arrow/python/benchmark.cc | Replace list access macro with function form. |
| python/pyarrow/src/arrow/python/arrow_to_pandas.cc | Replace list-set macro with PyList_SetItem. |
| python/pyarrow/io.pxi | Replace PyBytes_AS_STRING usage with PyBytes_AsString in Cython. |
|
There are CI failures due to an unexpected deprecation warning, see python/cpython#124182 (comment) for a potential solution. |
| } | ||
|
|
||
| std::string PyObject_StdStringTypeName(PyObject* obj) { | ||
| // Once Python 3.10 is dropped, this can use PyType_GetName(Py_TYPE(obj)) (added in 3.11). |
There was a problem hiding this comment.
To be fair we are just about to release 25.0.0, Release Candidate already created and being voted, which is the last release which will support Python 3.10. We could just drop 3.10 from main any time now.
github-actions
Bot
added
awaiting changes
There was a problem hiding this comment.
We should probably remove this file instead. Nobody ever runs.
There was a problem hiding this comment.
Sounds good. Removed.
There was a problem hiding this comment.
It's not yet removed from the PR apparently :)
There was a problem hiding this comment.
Why not replace these as well? Or are they part of the Limited API?
There was a problem hiding this comment.
Thanks, yeah these are not part of the Limited API. Changed in f308f1c
| bytes = PyBytes_AsString(obj); | ||
| size = PyBytes_Size(obj); |
There was a problem hiding this comment.
Need to check for errors here, at least as a debug assertion (since those functions shouldn't fail on a bytes object).
There was a problem hiding this comment.
For all these error checking, an agent picked up on using RETURN_IF_PYERROR in f308f1c. Let me know if I should use something else
There was a problem hiding this comment.
Well, there's no reason for errors here so we can just have:
if (PyBytes_Check(obj)) {
bytes = PyBytes_AsString(obj);
size = PyBytes_Size(obj);
DCHECK(!PyErr_Occurred());There was a problem hiding this comment.
Ah gotcha. Followed this pattern in bfa0d50
| } | ||
|
|
||
| const int32_t length = static_cast<int32_t>(PyBytes_GET_SIZE(utf8_obj.obj())); | ||
| const int32_t length = static_cast<int32_t>(PyBytes_Size(utf8_obj.obj())); |
There was a problem hiding this comment.
As an alternative, we can add PyBytes_AsStdStringView that would return a std::string_view of a PyBytes object, and use it here.
There was a problem hiding this comment.
Sure in b1b1e72 I defined PyBytes_AsStdStringView in helpers.cc and used it here
| } | ||
| PyArray_Descr* sub_dtype = | ||
| reinterpret_cast<PyArray_Descr*>(PyTuple_GET_ITEM(tup, 0)); | ||
| reinterpret_cast<PyArray_Descr*>(PyTuple_GetItem(tup, 0)); |
| PyList_SET_ITEM(bytes_field_names_.obj(), i, bytes); | ||
| PyList_SET_ITEM(unicode_field_names_.obj(), i, unicode); | ||
| PyList_SetItem(bytes_field_names_.obj(), i, bytes); | ||
| PyList_SetItem(unicode_field_names_.obj(), i, unicode); |
github-actions
Bot
added
awaiting change review
| PyObject* MonthDayNanoIntervalToNamedTuple( | ||
| const MonthDayNanoIntervalType::MonthDayNanos& interval) { | ||
| OwnedRef tuple(PyStructSequence_New(&MonthDayNanoTupleType)); | ||
| OwnedRef tuple(PyStructSequence_New(MonthDayNanoTupleType)); | ||
| if (ARROW_PREDICT_FALSE(tuple.obj() == nullptr)) { | ||
| return nullptr; |
| PyObject* MonthDayNanoIntervalToNamedTuple( | ||
| const MonthDayNanoIntervalType::MonthDayNanos& interval) { | ||
| OwnedRef tuple(PyStructSequence_New(&MonthDayNanoTupleType)); | ||
| OwnedRef tuple(PyStructSequence_New(MonthDayNanoTupleType)); | ||
| if (ARROW_PREDICT_FALSE(tuple.obj() == nullptr)) { |
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Here are some additional comments, but I notice some of the existing comments haven't been addressed.
| std::string PyBytes_AsStdString(PyObject* obj) { | ||
| ARROW_DCHECK(PyBytes_Check(obj)); | ||
| return std::string(PyBytes_AS_STRING(obj), PyBytes_GET_SIZE(obj)); | ||
| return std::string(PyBytes_AsString(obj), PyBytes_Size(obj)); |
There was a problem hiding this comment.
Can use PyBytes_AsStringAndSize as below.
There was a problem hiding this comment.
| return "?"; | ||
| } | ||
| Py_ssize_t size; | ||
| const char* data = PyUnicode_AsUTF8AndSize(name_ref.obj(), &size); |
There was a problem hiding this comment.
Why not use PyUnicode_AsStdString?
There was a problem hiding this comment.
There was a problem hiding this comment.
It's not yet removed from the PR apparently :)
| } | ||
|
|
||
| const int32_t length = static_cast<int32_t>(PyBytes_GET_SIZE(utf8_obj.obj())); | ||
| const int32_t length = static_cast<int32_t>(PyBytes_Size(utf8_obj.obj())); |
There was a problem hiding this comment.
As an alternative, we can add PyBytes_AsStdStringView that would return a std::string_view of a PyBytes object, and use it here.
mroeschke
changed the title
mroeschke
changed the title
There was a problem hiding this comment.
⚠️ Not ready to approve
There are confirmed error-handling issues (unchecked PyList_SetItem / PyBytes_AsStringAndSize) and a likely nullptr crash risk in MonthDayNanoIntervalToNamedTuple, plus user-visible changes not reflected in the PR description.
Review details
Comments suppressed due to low confidence (4)
python/pyarrow/src/arrow/python/python_to_arrow.cc:1053
- PyList_SetItem() can fail (and sets an exception); the return value is currently ignored, so failures won’t be propagated and the loop continues with a pending Python error. Please check the return value and use RETURN_IF_PYERROR() to surface the failure as a Status.
PyUnicode_FromStringAndSize(field_name.c_str(), field_name.size());
RETURN_IF_PYERROR();
PyList_SetItem(bytes_field_names_.obj(), i, bytes);
PyList_SetItem(unicode_field_names_.obj(), i, unicode);
python/pyarrow/src/arrow/python/python_to_arrow.cc:1270
- PyList_SetItem() return value is ignored while building the list from an iterator. If SetItem fails, a Python exception will be set but this function will continue and return Status::OK(), leaving a pending error.
break;
}
PyList_SetItem(lst, i, item);
}
python/pyarrow/src/arrow/python/helpers.cc:112
- PyBytes_AsStringAndSize() return value is ignored. If it fails, this returns a std::string_view over a null buffer (undefined behavior on later use).
ARROW_DCHECK(PyBytes_Check(obj));
char* buffer = nullptr;
Py_ssize_t length = 0;
PyBytes_AsStringAndSize(obj, &buffer, &length);
return std::string_view(buffer, length);
python/pyarrow/src/arrow/python/datetime.cc:606
- MonthDayNanoIntervalToNamedTuple() calls PyStructSequence_New(MonthDayNanoTupleType) without ensuring MonthDayNanoTupleType has been initialized; after this change it is a nullptr by default, so this can crash if NewMonthDayNanoTupleType() hasn’t been called yet in the process.
PyObject* MonthDayNanoIntervalToNamedTuple(
const MonthDayNanoIntervalType::MonthDayNanos& interval) {
OwnedRef tuple(PyStructSequence_New(MonthDayNanoTupleType));
if (ARROW_PREDICT_FALSE(tuple.obj() == nullptr)) {
- Files reviewed: 23/23 changed files
- Comments generated: 3
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
| # IPC / Messaging | ||
| include "ipc.pxi" | ||
|
|
||
| # Micro-benchmark routines | ||
| include "benchmark.pxi" | ||
|
|
||
| # Public API | ||
| include "public-api.pxi" |
There was a problem hiding this comment.
⚠️ Not ready to approve
MonthDayNano interval conversion can now segfault due to using an uninitialized cached PyTypeObject* in MonthDayNanoIntervalToNamedTuple().
Review details
Comments suppressed due to low confidence (2)
python/pyarrow/src/arrow/python/datetime.cc:606
- MonthDayNanoIntervalToNamedTuple() now calls PyStructSequence_New(MonthDayNanoTupleType) without ensuring MonthDayNanoTupleType has been initialized. Since MonthDayNanoTupleType starts as nullptr, calling this before NewMonthDayNanoTupleType() will segfault (e.g., MonthDayNanoIntervalScalar.as_py() can be used without ever running type inference).
PyObject* MonthDayNanoIntervalToNamedTuple(
const MonthDayNanoIntervalType::MonthDayNanos& interval) {
OwnedRef tuple(PyStructSequence_New(MonthDayNanoTupleType));
if (ARROW_PREDICT_FALSE(tuple.obj() == nullptr)) {
python/pyarrow/lib.pyx:243
- The PR removes the pyarrow micro-benchmark surface (benchmark.pxi include, plus related sources/modules elsewhere). That is a user-visible API change (imports like
pyarrow.benchmark) and also affects ASV benchmark coverage, but the PR description says there are no user-facing changes and doesn't mention the removal. If this is intentional, it should be called out (and ideally deprecated first); otherwise, keep the benchmark module and update it to use limited-API-safe calls instead of deleting it.
# IPC / Messaging
include "ipc.pxi"
# Public API
include "public-api.pxi"
- Files reviewed: 23/23 changed files
- Comments generated: 0 new
- Review effort level: Low
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
| bytes = PyBytes_AsString(ref.obj()); | ||
| RETURN_IF_PYERROR(); | ||
| size = PyBytes_Size(ref.obj()); | ||
| RETURN_IF_PYERROR(); |
There was a problem hiding this comment.
Similarly as above, this can be reduced to a single ARROW_DCHECK as these calls shouldn't normally fail after PyBytes_Check returned true.
There was a problem hiding this comment.
🟡 Not ready to approve
A conversion path now calls PyFloat_AsDouble() without checking for Python exceptions, and the PR also removes packaged benchmark entry points despite stating there are no user-facing changes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Comments suppressed due to low confidence (3)
python/pyarrow/src/arrow/python/python_to_arrow.cc:1053
PyList_SetItem()returns -1 and sets an exception on failure; unlike the macro form, it isn't infallible. These calls ignore the return value, so a failure would leave a Python exception pending and potentially corrupt later error handling. Consider checking the return value (pattern used elsewhere, e.g. python/pyarrow/src/arrow/python/numpy_convert.cc:400-402).
PyList_SetItem(bytes_field_names_.obj(), i, bytes);
PyList_SetItem(unicode_field_names_.obj(), i, unicode);
python/pyarrow/lib.pyx:243
- This removes the packaged
pyarrow.benchmark/benchmark_PandasObjectIsNullentry points (and the corresponding C++ sources). That is a user-visible API change and also potentially impacts ASV/perf tracking; the PR description currently states “No user-facing changes”. If the intent is to keep benchmarks, they likely need to be updated to use the limited API rather than removed; if the intent is to drop them, please explicitly call that out (and consider a deprecation path or release-note entry).
# IPC / Messaging
include "ipc.pxi"
# Public API
include "public-api.pxi"
python/pyarrow/src/arrow/python/python_to_arrow.cc:259
PyFloat_AsDouble()can set a Python exception (e.g., for float subclasses) and returns -1.0 on error. This branch doesn't checkPyErr_Occurred(), so conversion could silently succeed with an invalid value while an exception is pending.
if (PyFloat_Check(obj)) {
value = PyFloat_AsDouble(obj);
} else if (internal::PyFloatScalar_Check(obj)) {
- Files reviewed: 23/23 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
It introduces a crash risk in MonthDayNanoIntervalToNamedTuple due to uninitialized cached type usage, and it removes pyarrow.benchmark despite the PR stating no user-facing changes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
python/pyarrow/lib.pyx:243
- This PR removes the pyarrow.benchmark bindings (benchmark.pxi / benchmark.py, the exported C++ symbol, and the ASV microbenchmark). That is a user-visible API / repository tooling change, but the PR description says there are no user-facing changes. Either keep the module (possibly behind a deprecation path) or update the PR description / release notes to reflect the removal.
# File IO
include "io.pxi"
# IPC / Messaging
include "ipc.pxi"
# Public API
include "public-api.pxi"
python/pyarrow/src/arrow/python/datetime.cc:606
- MonthDayNanoTupleType is now a lazily-initialized pointer, but MonthDayNanoIntervalToNamedTuple() calls PyStructSequence_New(MonthDayNanoTupleType) without ensuring the type has been created. If NewMonthDayNanoTupleType() was never called (e.g., code paths not going through ImportPresentIntervalTypes), this will dereference nullptr and crash.
PyObject* MonthDayNanoIntervalToNamedTuple(
const MonthDayNanoIntervalType::MonthDayNanos& interval) {
OwnedRef tuple(PyStructSequence_New(MonthDayNanoTupleType));
if (ARROW_PREDICT_FALSE(tuple.obj() == nullptr)) {
python/pyarrow/src/arrow/python/common.h:31
- common.h now includes arrow/python/helpers.h, which pulls in arrow/python/numpy_interop.h (and NumPy headers). common.h is included broadly across the Arrow Python C++ sources, so this can unintentionally make non-NumPy-dependent translation units require NumPy headers and increases compile-time coupling. Prefer forward-declaring PyObject_StdStringTypeName() here instead of including helpers.h.
#include "arrow/buffer.h"
#include "arrow/python/helpers.h"
#include "arrow/python/pyarrow.h"
#include "arrow/python/visibility.h"
#include "arrow/result.h"
#include "arrow/util/logging.h"
#include "arrow/util/macros.h"
- Files reviewed: 23/23 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Rationale for this change
What changes are included in this PR?
This is an agent generated PR that swaps implementations to use CPython Limited APIs and required less reworks.
Py_TYPE(obj)->tp_namestruct-field reads with a new limited-API helper (PyObject_StdStringTypeName, built onPyType_GetName)tp_as_number->nb_intslot access withPyType_GetSlot(..., Py_nb_int)PyType_HasFeaturemacro withPyType_GetFlags(...)bit testsPyTypeObject + PyStructSequence_InitType2pattern with a cachedPyStructSequence_NewTypeAre these changes tested?
Yes
Are there any user-facing changes?
No