Copilot commented on code in PR #50409:
URL: https://github.com/apache/arrow/pull/50409#discussion_r3538874349
##########
python/pyarrow/src/arrow/python/python_to_arrow.cc:
##########
@@ -1050,8 +1049,8 @@ class PyStructConverter : public
StructConverter<PyConverter, PyConverterTrait>
PyObject* unicode =
PyUnicode_FromStringAndSize(field_name.c_str(), field_name.size());
RETURN_IF_PYERROR();
- 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);
Review Comment:
`PyList_SetItem()` can fail (e.g., if the list size/index assumptions are
violated) and sets a Python exception, but the return value is currently
ignored. If it does fail, this function will keep going and return
`Status::OK()` with an exception still set, which can lead to confusing
downstream failures.
##########
python/pyarrow/src/arrow/python/python_to_arrow.cc:
##########
@@ -1266,7 +1265,7 @@ Status ConvertToSequenceAndInferSize(PyObject* obj,
PyObject** seq, int64_t* siz
RETURN_IF_PYERROR();
break;
}
- PyList_SET_ITEM(lst, i, item);
+ PyList_SetItem(lst, i, item);
Review Comment:
`PyList_SetItem()` returns `-1` on failure and sets an exception, but the
return value is ignored here. If it ever fails, the loop will continue with a
pending Python exception and ultimately return `Status::OK()`.
##########
python/pyarrow/src/arrow/python/datetime.cc:
##########
@@ -274,13 +274,14 @@ static inline Status PyDate_convert_int(int64_t val,
const DateUnit unit, int64_
}
PyObject* NewMonthDayNanoTupleType() {
- if (MonthDayNanoTupleType.tp_name == nullptr) {
- if (PyStructSequence_InitType2(&MonthDayNanoTupleType,
&MonthDayNanoTupleDesc) != 0) {
+ if (MonthDayNanoTupleType == nullptr) {
+ MonthDayNanoTupleType = PyStructSequence_NewType(&MonthDayNanoTupleDesc);
+ if (MonthDayNanoTupleType == nullptr) {
Py_FatalError("Could not initialize MonthDayNanoTuple");
}
}
- Py_INCREF(&MonthDayNanoTupleType);
- return (PyObject*)&MonthDayNanoTupleType;
+ Py_INCREF(MonthDayNanoTupleType);
+ return (PyObject*)MonthDayNanoTupleType;
}
Review Comment:
`PyStructSequence_NewType()` is used unconditionally for initializing the
`MonthDayNano` struct-sequence type. If this API isn’t available in all
supported CPython versions, this will break compilation; consider a
`PY_VERSION_HEX`-guarded fallback to `PyStructSequence_InitType2()` for older
versions.
##########
python/pyarrow/src/arrow/python/helpers.cc:
##########
@@ -121,12 +122,27 @@ std::string PyObject_StdStringRepr(PyObject* obj) {
if (!bytes_ref) {
PyErr_Clear();
std::stringstream ss;
- ss << "<object of type '" << Py_TYPE(obj)->tp_name << "' repr() failed>";
+ ss << "<object of type '" << PyObject_StdStringTypeName(obj) << "' repr()
failed>";
return ss.str();
}
return PyBytes_AsStdString(bytes_ref.obj());
}
+std::string PyObject_StdStringTypeName(PyObject* obj) {
+ OwnedRef name_ref(PyType_GetName(Py_TYPE(obj)));
+ if (!name_ref) {
+ PyErr_Clear();
+ return "?";
+ }
+ Py_ssize_t size;
+ const char* data = PyUnicode_AsUTF8AndSize(name_ref.obj(), &size);
+ if (data == nullptr) {
+ PyErr_Clear();
+ return "?";
+ }
+ return std::string(data, size);
+}
Review Comment:
`PyObject_StdStringTypeName()` unconditionally uses `PyType_GetName()`.
Since PyArrow still supports Python 3.10 (`python/pyproject.toml` has
`requires-python = ">=3.10"`), this should be guarded (or shimmed) so builds
against older CPython headers don’t fail. A fallback that reads `type.__name__`
keeps the implementation limited-API-friendly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
