Copilot commented on code in PR #50327:
URL: https://github.com/apache/arrow/pull/50327#discussion_r3546240817
##########
python/pyarrow/array.pxi:
##########
@@ -4006,6 +4105,15 @@ cdef class LargeStringArray(Array):
Concrete class for Arrow arrays of large string (or utf8) data type.
"""
+ cdef object _getitem_py(self, int64_t i):
+ cdef:
+ int64_t length
+ const uint8_t* data
+ if self.ap.IsNull(i):
+ return None
+ data = (<CLargeStringArray*> self.ap).GetValue(i, &length)
+ return cp.PyUnicode_DecodeUTF8(<const char*> data, length, NULL)
Review Comment:
`PyUnicode_DecodeUTF8` expects a `Py_ssize_t` length; passing the `int64_t`
length from `LargeStringArray::GetValue` can overflow/truncate on 32-bit builds
or for very large values, potentially turning the length negative and causing
crashes or incorrect decoding. Please cast via `Py_ssize_t` with an explicit
overflow check (and apply the same pattern to `LargeBinaryArray._getitem_py`,
which has the same issue).
##########
python/pyarrow/tests/test_array.py:
##########
@@ -465,6 +465,38 @@ def test_array_getitem_numpy_scalars():
assert arr[np.int32(idx)].as_py() == lst[idx]
+def test_to_pylist_bulk_paths():
+ # GH-50326: list-like and string arrays convert to Python objects in
+ # bulk instead of going through one Scalar per element; the result must
+ # match the per-scalar conversion exactly.
+ arrays = [
+ pa.array([[1, None, 3], None, [], [4]], type=pa.list_(pa.int32())),
+ pa.array([["a", None], None, [], ["bcd", ""]],
+ type=pa.list_(pa.string())),
+ pa.array([["a", None], None, [], ["bcd", ""]],
+ type=pa.large_list(pa.large_string())),
+ pa.array([[1, None], None, [3, 4]], type=pa.list_(pa.int32(), 2)),
+ pa.array([[[1], [2, None]], None, [None, [3]]],
+ type=pa.list_(pa.list_(pa.int32()))),
+ pa.array([[("k1", 1), ("k2", None)], None, []],
+ type=pa.map_(pa.string(), pa.int32())),
+ pa.array(["a", None, "", "\N{GRINNING FACE} \N{SNOWMAN}"],
+ type=pa.string()),
+ pa.array(["a", None, "", "\N{GRINNING FACE} \N{SNOWMAN}"],
+ type=pa.large_string()),
+ pa.array([], type=pa.list_(pa.int32())),
+ pa.array([None, None], type=pa.list_(pa.string())),
+ ]
Review Comment:
`BinaryArray`/`LargeBinaryArray` now have scalar-free `_getitem_py` fast
paths, but this test only exercises string and list/string cases. Adding binary
and list<binary> cases here would help ensure the bytes fast path matches
scalar conversion (including embedded NUL bytes) and stays covered across
slices.
--
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]
