viirya commented on code in PR #50430:
URL: https://github.com/apache/arrow/pull/50430#discussion_r3685160268
##########
python/pyarrow/array.pxi:
##########
@@ -3618,18 +3611,48 @@ cdef class MapArray(ListArray):
Concrete class for Arrow arrays of a map data type.
"""
- cdef object _getitem_py(self, int64_t i):
+ cdef object _getitem_py(self, int64_t i, object maps_as_pydicts):
cdef CListArray* arr = <CListArray*> self.ap
+ if maps_as_pydicts not in (None, "lossy", "strict"):
+ # Matches MapScalar.as_py, which validates before the null check.
+ raise ValueError(
+ "Invalid value for 'maps_as_pydicts': "
+ + "valid values are 'lossy', 'strict' or `None` (default). "
+ + f"Received {maps_as_pydicts!r}."
+ )
Review Comment:
Done in 93b80439a8 — factored into `_check_maps_as_pydicts()`, now shared by
`MapScalar.as_py` and `MapArray._getitem_py`.
##########
python/pyarrow/array.pxi:
##########
@@ -3618,18 +3611,48 @@ cdef class MapArray(ListArray):
Concrete class for Arrow arrays of a map data type.
"""
- cdef object _getitem_py(self, int64_t i):
+ cdef object _getitem_py(self, int64_t i, object maps_as_pydicts):
cdef CListArray* arr = <CListArray*> self.ap
+ if maps_as_pydicts not in (None, "lossy", "strict"):
+ # Matches MapScalar.as_py, which validates before the null check.
+ raise ValueError(
+ "Invalid value for 'maps_as_pydicts': "
+ + "valid values are 'lossy', 'strict' or `None` (default). "
+ + f"Received {maps_as_pydicts!r}."
+ )
if arr.IsNull(i):
return None
if self._children_cache is None:
self._children_cache = (self.keys, self.items)
cdef Array keys = <Array> (<tuple> self._children_cache)[0]
cdef Array items = <Array> (<tuple> self._children_cache)[1]
cdef int64_t j, start = arr.value_offset(i), end = arr.value_offset(i
+ 1)
- # Matches MapScalar.as_py with the default maps_as_pydicts=None:
- # an association list of (key, value) tuples.
- return [(keys._getitem_py(j), items._getitem_py(j)) for j in
range(start, end)]
+ if maps_as_pydicts is None:
+ # Matches MapScalar.as_py with the default maps_as_pydicts=None:
+ # an association list of (key, value) tuples.
+ return [
+ (keys._getitem_py(j, None), items._getitem_py(j,
maps_as_pydicts))
+ for j in range(start, end)
+ ]
+ # MapScalar.as_py converts every key before processing values, then
+ # checks each key immediately before converting its corresponding
value.
Review Comment:
I simplified it to a single interleaved loop in 93b80439a8 (no more separate
keys pass), which I think addresses the tedium. I didn't go through the
association list though: its per-entry 2-tuples are exactly what the direct
dict build avoids, and it measures accordingly on 1M rows of 2-entry maps —
0.12 s for the direct dict vs 0.48 s for the association list (≈0.56 s with a
dict built on top). The duplicate handling (warn/raise) still needs the
explicit loop either way.
##########
python/pyarrow/tests/test_array.py:
##########
@@ -523,6 +523,79 @@ def test_to_pylist_bulk_paths():
dup.to_pylist()
+def test_to_pylist_maps_as_pydicts():
+ # GH-50429: maps_as_pydicts converts through the scalar-free path; the
+ # semantics must match MapScalar.as_py exactly.
+ map_type = pa.map_(pa.string(), pa.int32())
+ flat = pa.array(
+ [None, [("k1", 1), ("k2", None)], []], type=map_type)
+ # Expected values are written out literally so the reference stays
+ # independent of Array.to_pylist (ListScalar.as_py delegates to it).
+ cases = [
+ (flat, [None, {"k1": 1, "k2": None}, {}]),
+ (flat.slice(1), [{"k1": 1, "k2": None}, {}]),
+ (pa.array([[[('k', 1)], None], None], type=pa.list_(map_type)),
+ [[{"k": 1}, None], None]),
+ (pa.array([[[('k', 1)], None], None], type=pa.large_list(map_type)),
+ [[{"k": 1}, None], None]),
+ (pa.array([[[('k', 1)], None], None], type=pa.list_(map_type, 2)),
+ [[{"k": 1}, None], None]),
+ (pa.array([[("o", [("i", 5)])]],
+ type=pa.map_(pa.string(), map_type)),
+ [{"o": {"i": 5}}]),
+ (pa.array([{"m": [("k", 1)]}, None],
+ type=pa.struct([("m", map_type)])),
+ [{"m": {"k": 1}}, None]),
+ ]
+ for arr, expected in cases:
+ assert arr.to_pylist(maps_as_pydicts="strict") == expected
+
+ dup = pa.array([[("k", 1), ("k", 2)]], type=map_type)
+ with pytest.warns(UserWarning, match="already encountered"):
+ assert dup.to_pylist(maps_as_pydicts="lossy") == [{"k": 2}]
+ with pytest.raises(KeyError, match="strict mode"):
+ dup.to_pylist(maps_as_pydicts="strict")
+
+ # Duplicate keys must be detected before converting values: with a
+ # poison value *after* the duplicate, strict mode raises the outer
+ # duplicate-key error, and lossy mode warns before converting that value.
Review Comment:
It pins that duplicate detection happens before converting that entry's
value, matching `MapScalar.as_py` — the first implementation got this wrong
(values converted before duplicate detection) and review caught it, so the test
guards the regression. The exact-sequence form was requested in an earlier
review pass; if you don't consider the ordering part of the contract I'm happy
to reduce it to a plain `pytest.warns`.
--
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]
