Copilot commented on code in PR #50409:
URL: https://github.com/apache/arrow/pull/50409#discussion_r3547474573
##########
python/pyarrow/src/arrow/python/datetime.cc:
##########
@@ -596,7 +597,7 @@ Result<std::string> TzinfoToString(PyObject* tzinfo) {
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;
Review Comment:
MonthDayNanoTupleType can be nullptr here unless NewMonthDayNanoTupleType()
was called earlier. PyStructSequence_New(nullptr) is undefined behavior and can
segfault; previously passing &MonthDayNanoTupleType at least avoided a null
pointer. Ensure the cached type is initialized before use (and balance the
refcount from NewMonthDayNanoTupleType).
##########
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 does not steal references on failure. The
current code ignores the return value (and doesn't check PyErr), which can
leave a pending exception and leak the created objects if an unexpected failure
occurs.
##########
python/pyarrow/src/arrow/python/arrow_to_pandas.cc:
##########
@@ -1016,8 +1016,8 @@ Status ConvertMap(PandasOptions options, const
ChunkedArray& data,
return CheckPyError();
},
[&list_item](int64_t idx, OwnedRef& key_value, OwnedRef& item_value) {
- PyList_SET_ITEM(list_item.obj(), idx,
- PyTuple_Pack(2, key_value.obj(), item_value.obj()));
+ PyList_SetItem(list_item.obj(), idx,
+ PyTuple_Pack(2, key_value.obj(), item_value.obj()));
return CheckPyError();
Review Comment:
PyList_SetItem() can fail and only steals the tuple reference on success. As
written, a SetItem failure would leak the tuple from PyTuple_Pack and rely on
CheckPyError() to notice the error later. Consider creating the tuple first,
checking errors, and detaching only after a successful SetItem.
##########
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 does not steal a reference in
that case. Ignoring the return value can leak `item` and leave a Python
exception set, causing later C API calls to fail unpredictably.
##########
python/pyarrow/src/arrow/python/helpers.cc:
##########
@@ -121,12 +122,29 @@ 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:
PyObject_StdStringTypeName() currently returns type.__name__ only, which
drops the module-qualified name previously provided by tp_name (e.g.,
"pkg.Class" -> "Class"). This can make new/updated error messages less
informative. Consider returning "{__module__}.{__qualname__}" (dropping the
module for builtins) to preserve prior behavior while avoiding direct struct
access.
--
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]
