pitrou commented on code in PR #43540:
URL: https://github.com/apache/arrow/pull/43540#discussion_r1705525621
##########
python/pyarrow/src/arrow/python/python_to_arrow.cc:
##########
@@ -1107,11 +1107,19 @@ class PyStructConverter : public
StructConverter<PyConverter, PyConverterTrait>
Status AppendDict(PyObject* dict, PyObject* field_names) {
// NOTE we're ignoring any extraneous dict items
for (int i = 0; i < num_fields_; i++) {
- PyObject* name = PyList_GET_ITEM(field_names, i); // borrowed
- PyObject* value = PyDict_GetItem(dict, name); // borrowed
- if (value == NULL) {
- RETURN_IF_PYERROR();
- }
+ PyObject *name, *value;
+#ifdef Py_GIL_DISABLED
+ name = PyList_GetItemRef(field_names, i);
+ RETURN_IF_PYERROR();
+ PyDict_GetItemRef(dict, name, &value);
+ RETURN_IF_PYERROR();
+ OwnedRef nameref(name);
Review Comment:
You must move this line before the `PyDict_GetItemRef` call, otherwise you
can get a reference leak.
##########
python/pyarrow/src/arrow/python/deserialize.cc:
##########
@@ -88,8 +88,19 @@ Status DeserializeDict(PyObject* context, const Array&
array, int64_t start_idx,
// The latter two steal references whereas PyDict_SetItem does not. So we
need
// to make sure the reference count is decremented by letting the OwnedRef
// go out of scope at the end.
- int ret = PyDict_SetItem(result.obj(), PyList_GET_ITEM(keys.obj(), i -
start_idx),
- PyList_GET_ITEM(vals.obj(), i - start_idx));
+ PyObject *key, *val;
Review Comment:
What is the status of the deserialization code already? I see
`DeserializeObject` is not being called anywhere, should it be entirely removed
from the codebase? @jorisvandenbossche
##########
python/pyarrow/src/arrow/python/python_to_arrow.cc:
##########
@@ -1107,11 +1107,19 @@ class PyStructConverter : public
StructConverter<PyConverter, PyConverterTrait>
Status AppendDict(PyObject* dict, PyObject* field_names) {
// NOTE we're ignoring any extraneous dict items
for (int i = 0; i < num_fields_; i++) {
- PyObject* name = PyList_GET_ITEM(field_names, i); // borrowed
- PyObject* value = PyDict_GetItem(dict, name); // borrowed
- if (value == NULL) {
- RETURN_IF_PYERROR();
- }
+ PyObject *name, *value;
+#ifdef Py_GIL_DISABLED
+ name = PyList_GetItemRef(field_names, i);
+ RETURN_IF_PYERROR();
+ PyDict_GetItemRef(dict, name, &value);
+ RETURN_IF_PYERROR();
+ OwnedRef nameref(name);
+ OwnedRef valuered(value);
+#else
Review Comment:
Again, no need to keep the older version IMHO.
##########
python/pyarrow/src/arrow/python/python_to_arrow.cc:
##########
@@ -1107,11 +1107,19 @@ class PyStructConverter : public
StructConverter<PyConverter, PyConverterTrait>
Status AppendDict(PyObject* dict, PyObject* field_names) {
// NOTE we're ignoring any extraneous dict items
for (int i = 0; i < num_fields_; i++) {
- PyObject* name = PyList_GET_ITEM(field_names, i); // borrowed
- PyObject* value = PyDict_GetItem(dict, name); // borrowed
- if (value == NULL) {
- RETURN_IF_PYERROR();
- }
+ PyObject *name, *value;
+#ifdef Py_GIL_DISABLED
+ name = PyList_GetItemRef(field_names, i);
+ RETURN_IF_PYERROR();
+ PyDict_GetItemRef(dict, name, &value);
+ RETURN_IF_PYERROR();
+ OwnedRef nameref(name);
+ OwnedRef valuered(value);
Review Comment:
```suggestion
OwnedRef valueref(value);
```
##########
python/pyarrow/src/arrow/python/numpy_to_arrow.cc:
##########
@@ -757,8 +757,14 @@ Status NumPyConverter::Visit(const StructType& type) {
}
for (auto field : type.fields()) {
- PyObject* tup =
- PyDict_GetItemString(PyDataType_FIELDS(dtype_),
field->name().c_str());
+ PyObject* tup;
+#ifdef Py_GIL_DISABLED
+ PyDict_GetItemStringRef(PyDataType_FIELDS(dtype_),
field->name().c_str(), &tup);
+ RETURN_IF_PYERROR();
+ OwnedRef tupref(tup);
+#else
+ tup = PyDict_GetItemString(PyDataType_FIELDS(dtype_),
field->name().c_str());
+#endif
Review Comment:
No need to keep this older variant IMHO. We should just vendor
https://github.com/python/pythoncapi-compat to make sure we can use the newer
API.
##########
python/pyarrow/src/arrow/python/python_to_arrow.cc:
##########
@@ -1141,7 +1149,14 @@ class PyStructConverter : public
StructConverter<PyConverter, PyConverterTrait>
ARROW_ASSIGN_OR_RAISE(auto pair, GetKeyValuePair(items, i));
// validate that the key and the field name are equal
- PyObject* name = PyList_GET_ITEM(field_names, i);
+ PyObject *name;
+#ifdef Py_GIL_DISABLED
+ name = PyList_GetItemRef(field_names, i);
+ RETURN_IF_PYERROR();
+ OwnedRef nameref(name);
+#else
Review Comment:
Again here.
##########
python/pyarrow/src/arrow/python/benchmark.cc:
##########
@@ -29,7 +30,17 @@ void Benchmark_PandasObjectIsNull(PyObject* list) {
}
Py_ssize_t i, n = PyList_GET_SIZE(list);
for (i = 0; i < n; i++) {
- internal::PandasObjectIsNull(PyList_GET_ITEM(list, i));
+ PyObject *item;
+#ifdef Py_GIL_DISABLED
Review Comment:
These changes shouldn't be necessary as the list is private to the benchmark.
--
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]
