pitrou commented on code in PR #50327:
URL: https://github.com/apache/arrow/pull/50327#discussion_r3552416254
##########
python/pyarrow/array.pxi:
##########
@@ -4229,6 +4357,30 @@ cdef class StructArray(Array):
Concrete class for Arrow arrays of a struct data type.
"""
+ cdef object _getitem_py(self, int64_t i):
+ if self.ap.IsNull(i):
+ return None
+ cdef int64_t k, num_fields = self.type.num_fields
+ if self._children_cache is None:
+ names = [self.type.field(k).name for k in range(num_fields)]
+ if len(set(names)) != len(names):
+ # StructScalar.as_py raises ValueError on duplicate field
+ # names; mark the cache so we take the Scalar path below.
+ self._children_cache = (None, None)
Review Comment:
Why not raise `ValueError` here instead of adding this weird fallback path?
##########
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)
Review Comment:
You may be able to call `GetView(i)` which will give you a
`std::string_view`.
##########
python/pyarrow/array.pxi:
##########
@@ -2458,6 +2476,34 @@ cdef class NumericArray(Array):
A base class for Arrow numeric arrays.
"""
+ cdef object _getitem_py(self, int64_t i):
+ cdef Type tid = self.ap.type_id()
+ if self.ap.IsNull(i):
+ return None
+ if tid == _Type_INT64:
+ return (<CInt64Array*> self.ap).Value(i)
+ elif tid == _Type_INT32:
+ return (<CInt32Array*> self.ap).Value(i)
Review Comment:
Why the peculiar ordering of types? I would rather have something more
regular for readability.
##########
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:
We do support 32-bit platforms actually. But obviously a string size
couldn't be larger than 4GiB on such a platform anyway, so the AI's comment is
moot.
##########
python/pyarrow/tests/test_array.py:
##########
@@ -465,6 +465,56 @@ def test_array_getitem_numpy_scalars():
assert arr[np.int32(idx)].as_py() == lst[idx]
+def test_to_pylist_bulk_paths():
+ # GH-50326: to_pylist converts through scalar-free _getitem_py
+ # specializations; 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([b"a\x00b", None, b"", b"\xff"], type=pa.binary()),
+ pa.array([b"a\x00b", None, b""], type=pa.large_binary()),
+ pa.array([[b"x", None, b"\x00y"], None, []],
+ type=pa.list_(pa.binary())),
+ pa.array([1, None, -(2**62), 2**62], type=pa.int64()),
+ pa.array([0, None, 2**63 + 7], type=pa.uint64()),
+ pa.array([-128, 127, None], type=pa.int8()),
+ pa.array([1.5, None, -0.5], type=pa.float64()),
+ pa.array([1.5, None], type=pa.float32()),
+ pa.array([True, None, False], type=pa.bool_()),
+ pa.array([{"a": 1, "b": "x"}, None, {"a": None, "b": None}],
+ type=pa.struct([("a", pa.int32()), ("b", pa.string())])),
+ pa.array([], type=pa.list_(pa.int32())),
+ pa.array([None, None], type=pa.list_(pa.string())),
+ ]
Review Comment:
Can we do `pa.binary_view` as well?
##########
python/pyarrow/tests/test_array.py:
##########
@@ -465,6 +465,56 @@ def test_array_getitem_numpy_scalars():
assert arr[np.int32(idx)].as_py() == lst[idx]
+def test_to_pylist_bulk_paths():
+ # GH-50326: to_pylist converts through scalar-free _getitem_py
+ # specializations; 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([b"a\x00b", None, b"", b"\xff"], type=pa.binary()),
+ pa.array([b"a\x00b", None, b""], type=pa.large_binary()),
+ pa.array([[b"x", None, b"\x00y"], None, []],
+ type=pa.list_(pa.binary())),
+ pa.array([1, None, -(2**62), 2**62], type=pa.int64()),
+ pa.array([0, None, 2**63 + 7], type=pa.uint64()),
+ pa.array([-128, 127, None], type=pa.int8()),
+ pa.array([1.5, None, -0.5], type=pa.float64()),
+ pa.array([1.5, None], type=pa.float32()),
+ pa.array([True, None, False], type=pa.bool_()),
+ pa.array([{"a": 1, "b": "x"}, None, {"a": None, "b": None}],
+ type=pa.struct([("a", pa.int32()), ("b", pa.string())])),
+ pa.array([], type=pa.list_(pa.int32())),
+ pa.array([None, None], type=pa.list_(pa.string())),
+ ]
+ for arr in arrays:
+ for view in (arr, arr.slice(1), arr.slice(0, 2), arr.slice(2)):
+ assert view.to_pylist() == [x.as_py() for x in view]
+
+ # Values inside numeric lists must stay Python ints/None, never floats
+ result = pa.array([[1, None, 3]], type=pa.list_(pa.int32())).to_pylist()
+ assert result == [[1, None, 3]]
+ assert [type(x) for x in result[0]] == [int, type(None), int]
+
+ # Duplicate struct field names raise like StructScalar.as_py does
+ dup = pa.StructArray.from_arrays(
+ [pa.array([1, 2]), pa.array(["a", "b"])], names=["x", "x"])
+ with pytest.raises(ValueError):
Review Comment:
Can you test (part of) the error message as well? Something like:
```suggestion
with pytest.raises(ValueError, match='some regex'):
```
##########
python/pyarrow/array.pxi:
##########
@@ -1864,7 +1864,19 @@ cdef class Array(_PandasConvertible):
lst : list
"""
self._assert_cpu()
- return [x.as_py(maps_as_pydicts=maps_as_pydicts) for x in self]
+ cdef int64_t i, n = self.length()
+ if maps_as_pydicts is not None:
+ # Converting maps to dicts has per-entry semantics (duplicate-key
+ # detection); use the Scalar-based conversion for exact behavior.
+ return [x.as_py(maps_as_pydicts=maps_as_pydicts) for x in self]
+ return [self._getitem_py(i) for i in range(n)]
+
+ cdef object _getitem_py(self, int64_t i):
Review Comment:
Not necessary for this PR, but as the AI hinted it might be better to
replace this with a `cdef list _getitem_range_py(self, int64_t offset, int64_t
length)`. This would cut down on function call and prologue overhead.
Perhaps add a TODO or open a separate issue?
--
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]
