Copilot commented on code in PR #50409:
URL: https://github.com/apache/arrow/pull/50409#discussion_r3547554117
##########
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 and set a Python exception; the return value is
currently ignored, so this method could return Status::OK() while a Python
error is pending. Check the return value (or call RETURN_IF_PYERROR()) after
each SetItem to reliably propagate 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() return value is ignored here; if it fails, a Python
exception will be left pending and the loop will continue, potentially
returning Status::OK(). Handle the error and DECREF the partially-built list to
avoid leaking it.
##########
python/pyarrow/src/arrow/python/helpers.cc:
##########
@@ -121,12 +122,30 @@ 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) {
+ // Once Python 3.10 is dropped, this can use PyType_GetName(Py_TYPE(obj)) (
+ // added in 3.11).
+ OwnedRef name_ref(
+ PyObject_GetAttrString(reinterpret_cast<PyObject*>(Py_TYPE(obj)),
"__name__"));
+ 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:
PR description says PyObject_StdStringTypeName is built on PyType_GetName,
but the implementation currently does a __name__ attribute lookup. If the
intent is to use the limited API helper when available (3.11+), use
PyType_GetName under a version guard and fall back to __name__ for older
Pythons.
--
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]
