GH-50429: [Python] Convert maps to dicts without per-element Scalars in to_pylist#50430
GH-50429: [Python] Convert maps to dicts without per-element Scalars in to_pylist#50430viirya wants to merge 7 commits into
Conversation
…arrays Array.to_pylist() converts one element at a time: each row allocates a C++ Scalar (Array::GetScalar), a Python Scalar wrapper and, for list types, a Python Array wrapper for the row's values slice plus a fresh generator, before recursing per element. On top of the allocation cost itself, these GC-tracked wrappers repeatedly trigger collections that traverse the growing result list (~20% of runtime). This makes to_pylist on list-typed arrays several times slower than the bulk to_pandas conversion path. Add bulk to_pylist overrides: * ListArray / LargeListArray / FixedSizeListArray convert the referenced range of child values with a single recursive to_pylist call, then slice the resulting Python list per row using the raw offsets and the validity bitmap. MapArray keeps the generic scalar-based path (association-tuple / maps_as_pydicts duplicate-key semantics), as do the list-view types (overlapping views must not share sublist objects). * StringArray / LargeStringArray decode values directly from the data buffer (GetValue + PyUnicode_DecodeUTF8), matching StringScalar.as_py (= str(buf, 'utf8')) exactly. Semantics are unchanged; values inside numeric lists stay Python ints/None. Benchmarks (M4 Max, 2M rows of 2-element lists / 1M rows nested): list<string> 1.93s -> 0.34s, list<list<int32>> 2.10s -> 0.65s, flat string (4M) 0.83s -> 0.05s. Co-authored-by: Isaac
…to_pylist Per review feedback, replace the per-type to_pylist overrides with a general mechanism: * Array gains a cdef _getitem_py(i) returning self[i] as a Python object. The base implementation is GetScalar + Scalar.as_py, so any type without a specialization behaves exactly as before. * The baseline Array.to_pylist becomes a single loop over _getitem_py. maps_as_pydicts != None keeps the Scalar-based path (map->dict conversion has per-entry duplicate-key semantics). * Specializations avoid all per-element Scalar and per-row Array wrapper allocation: integers/floats (type_id switch on NumericArray; dates/times/timestamps fall through to the exact base), boolean, string/binary (+ large variants), list/large_list/fixed_size_list (per-row list built from the child's _getitem_py over the offset range, child wrapper cached on the array), map (list of key/value tuples), struct (dict per row; duplicate field names fall back so they raise ValueError like StructScalar.as_py). Benchmarks (M4 Max): flat int64 with nulls 4M ~0.39s -> 0.028s (~7ns per element, on par with ndarray.tolist); flat string 4M 0.83s -> 0.06s; list<string> 2M 1.93s -> 0.46s; list<list<int32>> 1M 2.10s -> 0.40s; struct<int64,string> 1M 0.91s -> 0.07s; map<string,int64> 1M 2.77s -> 0.74s. Co-authored-by: Isaac
Co-authored-by: Isaac
… tests Move the _children_cache declaration after the pre-existing attributes so their offsets stay stable for extensions compiled against an older pyarrow, and extend test_to_pylist_bulk_paths with binary/large_binary (including embedded NUL bytes), list<binary>, wide-range integers, floats, boolean and struct coverage plus a duplicate-field-name ValueError assertion. Co-authored-by: Isaac
|
|
There was a problem hiding this comment.
Pull request overview
This PR extends the scalar-free Array.to_pylist() conversion path to support maps_as_pydicts for MapArray, enabling direct Map→dict materialization without per-element Scalar allocation while preserving existing MapScalar.as_py semantics.
Changes:
- Add a
cdef _getitem_py(i, maps_as_pydicts)mechanism and updateArray.to_pylist()to use it for scalar-free conversions. - Implement
_getitem_pyspecializations for common leaf and nested array types (numeric, boolean, string/binary, list, struct, map), with caching of wrapped child arrays. - Add tests comparing
to_pylist()results against per-scalar conversion, includingmaps_as_pydictsbehaviors and nested compositions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| python/pyarrow/array.pxi | Implements _getitem_py and adds scalar-free to_pylist specializations, including MapArray dict conversion for maps_as_pydicts. |
| python/pyarrow/lib.pxd | Declares new Array Cython API surface (_children_cache, _getitem_py). |
| python/pyarrow/tests/test_array.py | Adds regression/differential tests for scalar-free to_pylist and maps_as_pydicts semantics. |
| cdef dict result = {} | ||
| for j in range(start, end): | ||
| result[keys._getitem_py(j, None)] = items._getitem_py(j, maps_as_pydicts) | ||
| if len(result) == end - start: | ||
| return result | ||
| # Duplicate keys: redo the row with the per-key loop so the 'lossy' | ||
| # warnings and 'strict' KeyError match MapScalar.as_py exactly. | ||
| result = {} | ||
| for j in range(start, end): | ||
| key = keys._getitem_py(j, None) | ||
| 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(j, maps_as_pydicts) | ||
| return result |
* Order the numeric type-id dispatch regularly (int8..uint64, float, double) instead of by expected hotness. * Use GetView(i) (std::string_view) instead of GetValue with an out parameter for string/binary values, and add StringViewArray / BinaryViewArray specializations on top of it. * Raise the duplicate-field-names ValueError directly in StructArray._getitem_py instead of falling back to the Scalar path, and assert the message in the test. * Add binary_view/string_view test coverage. * Add a TODO pointing at apacheGH-50448 (per-range conversion follow-up). Co-authored-by: Isaac
…alars in to_pylist Thread maps_as_pydicts through the scalar-free _getitem_py mechanism instead of routing it to the Scalar-based path. MapArray builds the dict directly from the flattened keys/items children; when the dict size reveals duplicate keys, the row is redone with the per-key loop so the 'lossy' warnings and 'strict' KeyError match MapScalar.as_py exactly. Invalid values still raise the same ValueError when a map value is converted (including null rows), non-map arrays still ignore the option, and the option still propagates through nested types. Unspecialized types keep the exact Scalar fallback, which now receives the option. Benchmark (M4 Max, 1M rows of 2-entry maps, 10% nulls): to_pylist(maps_as_pydicts='lossy') 2.20s -> 0.10s (~21x). The dict form is also faster than the default association-list form (0.78s), which allocates a 2-tuple per entry. Co-authored-by: Isaac
bca5c9a to
0a1fee8
Compare
| cdef dict result = {} | ||
| for j in range(start, end): | ||
| result[keys._getitem_py(j, None)] = items._getitem_py(j, maps_as_pydicts) | ||
| if len(result) == end - start: | ||
| return result | ||
| # Duplicate keys: redo the row with the per-key loop so the 'lossy' | ||
| # warnings and 'strict' KeyError match MapScalar.as_py exactly. | ||
| result = {} | ||
| for j in range(start, end): | ||
| key = keys._getitem_py(j, None) | ||
| 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(j, maps_as_pydicts) | ||
| return result |
There was a problem hiding this comment.
Good catch — this was a real ordering difference. Fixed in 8626b97: all keys are now converted first (matching MapScalar.as_py, which converts them via keys()), and duplicates are detected before any value conversion, so the 'strict' KeyError and the 'lossy' warning are emitted at the same point as the Scalar path even when a later value conversion raises. Added tests with a raising value placed after the duplicate key in both modes, and verified the exception/warning sequences match the Scalar path exactly (including nested-map warnings order).
Convert all keys first (as MapScalar.as_py does via keys()) and check for duplicates before converting any value, so that the 'lossy' warning and the 'strict' KeyError are emitted at the same point as in MapScalar.as_py even when a later value conversion raises. Add tests with a raising value placed after the duplicate key in both modes. lossy 1M rows: 0.16s (vs 2.20s Scalar-based; the extra keys pass costs ~0.06s vs the previous single-pass form). Co-authored-by: Isaac
Rationale for this change
Follow-up of #50326; stacked on #50327 — only the last commit is new, and this will be rebased once #50327 merges.
GH-50327 converts arrays to Python objects without per-element Scalars, but the
maps_as_pydictsoption still routes to the Scalar-based path: every map row allocates aMapScalar, converts its keys via per-elementas_py, and builds the dict in Python. Map→dict is the natural consumption pattern for engines whose map values are Python dicts (e.g. Spark's Arrow-serialized Python UDFs currently receive association lists and rebuild a dict per row in pure Python — one of the dominant remaining costs in that path).What changes are included in this PR?
Thread
maps_as_pydictsthrough the scalar-free_getitem_pymechanism:MapArray._getitem_pybuilds the dict directly from the flattened keys/items children. When the resulting dict size reveals duplicate keys, the row is redone with the careful per-key loop, so the'lossy'warnings and'strict'KeyErrormatchMapScalar.as_pyexactly (including messages and warning-per-duplicate behavior).ValueErrorwhen a map value is converted — including null map rows, matching the Scalar path — while non-map arrays keep ignoring the option.Benchmark (macOS arm64, M4 Max; 1M rows of 2-entry
map<string,int64>, 10% nulls):to_pylist(maps_as_pydicts='lossy')Notably the dict form is now also faster than the default association-list form (0.78 s), which allocates a 2-tuple per entry.
Are these changes tested?
New
test_to_pylist_maps_as_pydictscompares against the per-scalar conversion for flat maps,list<map>,map<string, map<...>>andstruct<map>(plain and sliced) in both modes, and asserts the duplicate-key semantics ('lossy'warns and keeps the last value;'strict'raisesKeyError), the invalid-valueValueError, and that non-map arrays ignore the option. Randomized differential tests against the Scalar path (exact type equality) andpytest test_array.py test_scalars.py test_convert_builtin.py test_table.py(1210 passed) also pass.Are there any user-facing changes?
No behavior changes, only performance.
This pull request and its description were written by Isaac.