rok commented on code in PR #50430:
URL: https://github.com/apache/arrow/pull/50430#discussion_r3638160908
##########
python/pyarrow/array.pxi:
##########
@@ -3618,18 +3611,65 @@ 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
+ cdef bint as_dicts = maps_as_pydicts is not None
+ if as_dicts and maps_as_pydicts != "lossy" and maps_as_pydicts !=
"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 not as_dicts:
+ # 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)
+ ]
+ # Convert all keys first (as MapScalar.as_py does via keys()) and
+ # detect duplicates before converting any value, so that the 'lossy'
+ # warnings and the 'strict' KeyError are emitted at the same point as
+ # in MapScalar.as_py even when a later value conversion raises.
+ cdef int64_t count = end - start
+ cdef list keys_py = [keys._getitem_py(j, None) for j in range(start,
end)]
+ cdef dict result = {}
+ cdef int64_t k
+ cdef bint no_dups
+ try:
+ no_dups = len(set(keys_py)) == count
+ except TypeError:
+ # Unhashable keys (e.g. struct or list keys): the per-key loop
+ # below reproduces MapScalar.as_py exactly, raising TypeError at
+ # the same membership test as the Scalar path does.
+ no_dups = False
+ if no_dups:
+ for k in range(count):
+ result[keys_py[k]] = items._getitem_py(start + k,
maps_as_pydicts)
+ return result
+ # Duplicate or unhashable keys: per-key loop matching MapScalar.as_py
+ # exactly.
+ for k in range(count):
+ key = keys_py[k]
+ if key in result:
+ if maps_as_pydicts == "strict":
+ raise KeyError(
+ "Converting to Python dictionary is not supported in
strict mode "
+ f"when duplicate keys are present (duplicate key was
'{key}')."
+ )
+ else:
+ warnings.warn(
+ f"Encountered key '{key}' which was already
encountered.")
+ result[key] = items._getitem_py(start + k, maps_as_pydicts)
+ return result
Review Comment:
The `set(keys_py)` fast path can be simplified to always use the per-key
loop. Both paths hash unique keys roughly twice, but this avoids allocating a
temporary set and an extra traversal; duplicates also avoid a fallback pass.
```suggestion
# MapScalar.as_py converts every key before processing values, then
# checks each key immediately before converting its corresponding
value.
cdef int64_t count = end - start
cdef list keys_py = [keys._getitem_py(j, None)
for j in range(start, end)]
cdef dict result = {}
cdef int64_t k
for k in range(count):
key = keys_py[k]
if key in result:
if maps_as_pydicts == "strict":
raise KeyError(
"Converting to Python dictionary is not supported in
strict mode "
f"when duplicate keys are present (duplicate key was
'{key}')."
)
warnings.warn(
f"Encountered key '{key}' which was already
encountered.")
result[key] = items._getitem_py(start + k, maps_as_pydicts)
return result
```
--
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]
