pitrou commented on code in PR #50430:
URL: https://github.com/apache/arrow/pull/50430#discussion_r3684407599
##########
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:
Why not factor out this check?
##########
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:
This looks a bit tedious. I think you could reuse the "association list"
above, build a dict from it and check that the dict length is equal to the list
length.
##########
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.
+ nested_map = pa.map_(pa.string(), map_type)
+ poison = pa.array(
+ [[("k1", [("a", 1)]), ("k1", [("d", 1), ("d", 2)])]], type=nested_map)
+ with pytest.raises(KeyError, match="duplicate key was 'k1'"):
+ poison.to_pylist(maps_as_pydicts="strict")
+ with pytest.warns(UserWarning) as caught:
+ assert poison.to_pylist(maps_as_pydicts="lossy") == [{"k1": {"d": 2}}]
+ assert [str(warning.message) for warning in caught] == [
+ "Encountered key 'k1' which was already encountered.",
+ "Encountered key 'd' which was already encountered.",
+ ]
+
+ # Preserve StructScalar.as_py's translation of nested KeyErrors.
+ nested_duplicate = pa.array(
+ [{"m": [("k", 1), ("k", 2)]}],
+ type=pa.struct([("m", map_type)]))
+ with pytest.raises(ValueError, match="duplicate field names"):
+ nested_duplicate.to_pylist(maps_as_pydicts="strict")
+
+ null_map = pa.array([None], type=map_type)
+ with pytest.raises(ValueError, match="Invalid value for
'maps_as_pydicts'"):
+ null_map.to_pylist(maps_as_pydicts="bogus")
+ # Invalid values are only rejected when a map value is converted.
+ assert pa.array([1, 2]).to_pylist(maps_as_pydicts="bogus") == [1, 2]
Review Comment:
Why test for this?
##########
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:
Why are we testing for this? I don't think this is part of the contract.
--
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]
