Skip to content

GH-50326: [Python] Convert arrays to Python objects without per-element Scalars in to_pylist#50327

Open
viirya wants to merge 5 commits into
apache:mainfrom
viirya:GH-50326-python-bulk-to-pylist
Open

GH-50326: [Python] Convert arrays to Python objects without per-element Scalars in to_pylist#50327
viirya wants to merge 5 commits into
apache:mainfrom
viirya:GH-50326-python-bulk-to-pylist

Conversation

@viirya

@viirya viirya commented Jul 1, 2026

Copy link
Copy Markdown
Member

Rationale for this change

pa.Array.to_pylist() converts one element at a time through Array::GetScalar plus a Python Scalar wrapper; for list types each row additionally allocates a Python Array wrapper for the row's values slice and a fresh generator before recursing per element. A sample profile shows ~20% of runtime in CPython GC (triggered by the per-row GC-tracked allocations), ~25% in GetScalar, and only ~7% doing the useful work of creating the output objects — making to_pylist several times slower than converting via to_pandas() and back, and ~24x slower than ndarray.tolist() for plain int64. Details in #50326; this hit Apache Spark's Arrow-serialized Python UDFs (apache/spark#56940, apache/spark#56943).

What changes are included in this PR?

Following review feedback, this adds a general scalar-free conversion mechanism instead of per-type to_pylist overrides:

  • Array gains cdef object _getitem_py(self, int64_t 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 today (dates, times, timestamps, durations, decimals, dictionary, extension, unions, views, ...).
  • The baseline Array.to_pylist becomes a single loop over _getitem_py. maps_as_pydicts != None keeps the Scalar-based path, since map→dict conversion has per-entry duplicate-key semantics.
  • Specializations avoid all per-element Scalar and per-row Array-wrapper allocation:
    • integers and floats (a type_id switch on NumericArray; date/time/timestamp subclasses fall through to the exact base),
    • boolean,
    • string/binary and large variants (GetValue + PyUnicode_DecodeUTF8 / PyBytes_FromStringAndSize, matching str(buf, 'utf8') / to_pybytes() exactly),
    • list/large_list/fixed_size_list (each row's list is built from the child's _getitem_py over the offset range; the wrapped child is cached on the parent array),
    • map (association list of key/value tuples, matching MapScalar.as_py),
    • struct (one dict per row; duplicate field names fall back to the Scalar path so they raise ValueError like StructScalar.as_py).

Nested types compose without any per-row wrappers. ChunkedArray.to_pylist, Table.to_pylist and ListScalar.as_py delegate here and speed up automatically. Follow-up candidates: string/binary views, run-end-encoded, dictionary, a fast path for date32.

Benchmarks (macOS arm64, M4 Max):

benchmark before after speedup
flat int64 with nulls (4M) 0.39 s 0.028 s 14x (~7 ns/element, on par with ndarray.tolist)
flat string (4M) 0.83 s 0.06 s 14x
list<string> (2M rows) 1.93 s 0.46 s 4.2x
list<list<int32>> (1M rows) 2.10 s 0.40 s 5.2x
struct<int64,string> (1M rows) 0.91 s 0.07 s 13x
map<string,int64> (1M rows) 2.77 s 0.74 s 3.8x

Are these changes tested?

test_to_pylist_bulk_paths (added here) compares against the per-scalar conversion with exact element types for representative arrays including sliced views. Additionally verified with a randomized differential test against [x.as_py() for x in arr] with exact-type equality: all integer widths (incl. values beyond 2^62), floats (NaN/inf), boolean, string/binary (+large, multibyte), all list kinds, nested lists, struct (incl. empty struct, duplicate-field-name ValueError), map (incl. strict-mode duplicate-key KeyError), dictionary/null fallbacks, sliced/chunked arrays, and both maps_as_pydicts modes — no differences. pytest test_array.py test_scalars.py test_convert_builtin.py test_table.py test_types.py: 1295 passed.

Are there any user-facing changes?

No behavior changes, only performance.

This pull request and its description were written by Isaac.

…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
Copilot AI review requested due to automatic review settings July 1, 2026 23:30
@viirya viirya requested review from AlenkaF, raulcd and rok as code owners July 1, 2026 23:30
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #50326 has been automatically assigned in GitHub to PR creator.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves pyarrow.Array.to_pylist() performance for list-like arrays and (large) string arrays by adding specialized bulk conversion implementations that avoid per-element Scalar allocation and wrapper overhead, while keeping output semantics unchanged.

Changes:

  • Add bulk to_pylist() implementations for ListArray, LargeListArray, and FixedSizeListArray that convert child values once and slice per row using offsets.
  • Add fast to_pylist() implementations for StringArray and LargeStringArray that decode directly from the value buffer.
  • Add a new test validating bulk-path results against the scalar-based reference across nested, sliced, empty, and all-null inputs.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
python/pyarrow/array.pxi Adds type-specific to_pylist() fast paths for list-like and string arrays.
python/pyarrow/tests/test_array.py Adds a regression/differential test to ensure bulk paths match scalar-based conversion.

Comment thread python/pyarrow/array.pxi Outdated
Comment on lines +4178 to +4195
n = arr.length()
result = []
# Decode values straight from the data buffer instead of creating
# a C++ Scalar and a Python Scalar wrapper per value (see GH-28694).
if arr.null_count() == 0:
for i in range(n):
data = arr.GetValue(i, &length)
result.append(
cp.PyUnicode_DecodeUTF8(<const char*> data, length, NULL))
else:
for i in range(n):
if arr.IsNull(i):
result.append(None)
else:
data = arr.GetValue(i, &length)
result.append(
cp.PyUnicode_DecodeUTF8(<const char*> data, length, NULL))
return result

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null_count() is a one-time vectorized popcount over the validity bitmap (~n/8 bytes, well under a millisecond for 2M rows), computed and cached per ArrayData. In exchange, the no-null branch skips the per-element IsNull() check entirely. Branching on null_bitmap_data() == NULL instead would save that single scan but degrade the common case of a sliced/combined array that has a bitmap yet contains no nulls in range — that would take the per-element IsNull() path forever. So the current form should be the better trade-off in practice.

@gaogaotiantian

Copy link
Copy Markdown

I'm not an expert in Cython but curious about how result.append() works. I think as long as result is not defined as a Cython list, result.append still works as a pure Python object? In that case, each append would trigger a dynamic attribute search and the list would be reallocated a few times during appending.

It would be an interesting experiment to do a full-allocation for the list before assigning the data, as we already know the length of the list. An extra step forward is to declare the return value as a list in Cython so it can optimize setitem even more.

Something like

cdef list result = [None] * n
cdef Py_ssize_t i

for i in range(n):
    result[i] = ...

return result

For a long list this might push the performance even further.

@viirya

viirya commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

Good idea — I tried exactly that (cdef list result = [None] * n + indexed assignment, also letting null rows keep the prefilled None), but it benchmarks the same or slightly worse: list<string> 0.34 s → 0.35–0.37 s, list<list<int32>> 0.65 s → 0.69–0.70 s (best of 3, consistent across reruns).

Two reasons, I think: Cython already lowers result.append(x) on a local it can see is a list to a PyList_Append call, and CPython lists over-allocate geometrically, so the resize cost is amortized and tiny. Meanwhile the preallocated version pays an extra refcount round-trip per slot (every result[i] = x has to decref the prefilled None). The loops are dominated by creating the per-row slice objects / decoding UTF-8 either way, so I kept the simpler append form.

@gaogaotiantian

Copy link
Copy Markdown

Okay if the benchmark is similar this is good. Would defining result as a list help? Like a cdef list for it and do append. Just curious whether cython knows it's a list already - maybe it does and that's why it's fast.

@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jul 2, 2026
@viirya

viirya commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

Good question — Cython already knows. Its type inference marks result = [] as list, so the untyped version and an explicit cdef list result = [] compile to the identical generated C: both lower result.append(x) to Cython's inlined __Pyx_PyList_Append(), which appends in place via PyList_SET_ITEM while there's spare capacity and only falls back to PyList_Append (geometric resize) otherwise. I verified by cythonizing both variants side by side and diffing the generated C — the loop bodies are byte-identical. That's indeed why the append form is already fast, and why adding cdef list doesn't move the numbers.

@pitrou

pitrou commented Jul 8, 2026

Copy link
Copy Markdown
Member

The cause is the per-element conversion loop ([x.as_py() for x in self]): every 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.

I agree this is extremely wasteful, but the approach here seems very ad hoc and also solves the performance issue for a very limited set of types, while the performance problem is more general:

In [5]: a = np.arange(10_0000)

In [6]: %timeit a.tolist()
722 μs ± 43.8 μs per loop (mean ± std. dev. of 7 runs, 1,000 loops each)

In [7]: b = pa.array(a)

In [8]: %timeit b.to_pylist()
17.5 ms ± 147 μs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [9]: b.to_pylist() == a.tolist()
Out[9]: True

How about we do something like the following:

  1. Add an Array method that indexes and returns a Python value (e.g. an int) directly instead of a PyArrow scalar (e.g. an Int64Scalar)
  2. Use that new method in the baseline implementation of Array::to_pylist.

@viirya

viirya commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Thanks for the review, @pitrou! Agreed — the per-type to_pylist overrides don't generalize. I'll rework the PR along your suggestion, which I read as a scalar-free getitem:

  1. Add cdef object _getitem_py(self, int64_t i, ...) on Array, whose base implementation preserves today's semantics exactly (GetScalar(i) + as_py()), so any type without a specialization behaves identically to today.
  2. Make the baseline Array.to_pylist a single loop over _getitem_py.
  3. Specialize _getitem_py for the common types: integers, floats, boolean, string/binary (+ large variants), list-likes (build each row's list directly from the child's _getitem_py over the offset range), struct and map — nested types then compose without allocating any Scalar or Array wrapper per row, and maps_as_pydicts threads through as an argument.

Types with non-trivial as_py semantics (dates/timestamps/decimals/dictionary/...) would keep the exact fallback initially and can be specialized incrementally in follow-ups; your int64 example above lands in the specialized bucket and should get within striking distance of ndarray.tolist.

One question before I rework: do you prefer this mechanism in Cython as sketched, or as a C++ ToPyList visitor under python/pyarrow/src/arrow/python/ (in the spirit of MonthDayNanoIntervalArrayToPyList)? The Cython version keeps the unspecialized fallback trivially exact and the diff reviewable; the C++ visitor is likely faster still (and would also serve ChunkedArray/Table uniformly) but is a much bigger surface. Happy to go either way.

@pitrou

pitrou commented Jul 8, 2026

Copy link
Copy Markdown
Member

One question before I rework: do you prefer this mechanism in Cython as sketched, or as a C++ ToPyList visitor under python/pyarrow/src/arrow/python/ (in the spirit of MonthDayNanoIntervalArrayToPyList)? The Cython version keeps the unspecialized fallback trivially exact and the diff reviewable; the C++ visitor is likely faster still (and would also serve ChunkedArray/Table uniformly) but is a much bigger surface. Happy to go either way.

Doing this in Cython would probably be more maintainable, right? Wrangling CPython API calls from C++ is generally tedious and error-prone.

…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
Copilot AI review requested due to automatic review settings July 8, 2026 17:49
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 8, 2026
@viirya viirya changed the title GH-50326: [Python] Speed up to_pylist for list-like and string arrays GH-50326: [Python] Convert arrays to Python objects without per-element Scalars in to_pylist Jul 8, 2026
@viirya

viirya commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Reworked as suggested — the PR now adds a scalar-free cdef _getitem_py(self, int64_t i) on Array and the baseline to_pylist is a single loop over it. The base implementation is GetScalar + Scalar.as_py, so unspecialized types (dates/times/timestamps/decimals/dictionary/extension/...) behave exactly as before, and specializations exist for integers, floats, boolean, string/binary (+ large variants), list/large_list/fixed_size_list, map and struct. Nested types compose: a list row is built directly from the child's _getitem_py over its offset range, with the child wrapper cached on the parent array. maps_as_pydicts != None keeps the Scalar path since map→dict conversion has per-entry duplicate-key semantics.

Your int64 example: 4M int64 with nulls goes from ~0.39 s to 0.028 s (~7 ns/element — on par with ndarray.tolist). Other numbers (M4 Max): flat string 4M 0.83→0.06 s; list<string> 2M 1.93→0.46 s; list<list<int32>> 1M 2.10→0.40 s; struct<int64,string> 1M 0.91→0.07 s; map<string,int64> 1M 2.77→0.74 s.

Net diff is smaller than the previous approach (−104 lines) since the per-type to_pylist overrides are gone. Differential tests against the Scalar path (exact type equality, incl. slices, all-null, duplicate struct field names raising ValueError, strict-mode maps) pass, as does the pytest suite (1295 passed). PR description updated accordingly.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread python/pyarrow/lib.pxd Outdated
Comment on lines +282 to +286
cdef:
shared_ptr[CArray] sp_array
CArray* ap
# Lazily wrapped child array(s) reused by _getitem_py (see GH-50326)
object _children_cache

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point — moved the declaration after the pre-existing attributes in 728584f, so type/_name offsets stay stable and the new field follows the append-only convention that Cython's size check assumes.

Copilot AI review requested due to automatic review settings July 8, 2026 18:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment thread python/pyarrow/array.pxi Outdated
Comment on lines +4108 to +4115
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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PyArrow only supports 64-bit platforms, where Py_ssize_t and int64_t have the same width; and on a hypothetical 32-bit build a single value longer than PY_SSIZE_T_MAX could not exist in the first place, since the child data buffer itself is bounded by the process address space. So the implicit conversion cannot truncate in practice.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the correction — I stand corrected on 32-bit support; agreed it's moot either way since such a value can't exist in a 32-bit address space.

Comment on lines +472 to +489
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([], type=pa.list_(pa.int32())),
pa.array([None, None], type=pa.list_(pa.string())),
]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added binary/large_binary (including embedded NUL bytes), list, wide-range integers, floats, boolean and struct coverage in 728584f, plus an assertion that duplicate struct field names still raise ValueError.

… 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
Copilot AI review requested due to automatic review settings July 8, 2026 18:37
@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jul 8, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@viirya

viirya commented Jul 8, 2026

Copy link
Copy Markdown
Member Author

Note on the two red CI jobs — both are unrelated to this change:

  • AMD64 macOS 15-intel Python 3: the only failure is test_s3fs_limited_permissions_create_bucket, an S3/minio fixture issue (the mc admin subprocess dies with SIGPIPE before the filesystem is exercised). The same job passes 7616 tests, including all array/scalar/conversion tests. The same flake also hit the previous commit's run.
  • AMD64 Conda Python 3.11 Sphinx & Numpydoc: two doctest failures in code this PR doesn't touch — Table.join_asof (the example's output chunking varies with task scheduling: [[1,3],[2,3,3]] vs [[1,3,2,3,3]]) and default_memory_pool (environment-dependent). The same job passed on the first commit of this PR with the same doctests.

@dbtsai

dbtsai commented Jul 9, 2026

Copy link
Copy Markdown
Member

Follow-up idea: reuse decoded strings for dictionary-encoded arrays

The new StringArray._getitem_py calls PyUnicode_DecodeUTF8 per element, so a repeated string value produces a fresh str object every time (N allocations + N UTF-8 decodes for N elements). Two places where value reuse could help:

  1. DictionaryArray (highest value, no correctness risk): the dictionary already stores each distinct value once. A specialized path could decode the D dictionary entries once and then return interned references (decoded[index[i]] + Py_INCREF) — turning N decodes into D decodes + N pointer reuses. This PR currently doesn't specialize DictionaryArray, so it falls back to the Scalar path and this win is unclaimed. Since str is immutable, sharing references is semantically identical to copying.

  2. Plain StringArray with repeated values: an explicit dedup cache (or PyUnicode_InternInPlace) would cut allocations on low-cardinality data, but adds a hash/lookup per element and regresses high-cardinality (mostly-unique) data. Probably only worth it behind a cardinality heuristic / opt-in, not by default.

Not blocking for this PR — just flagging DictionaryArray as a natural, safe follow-up where value reuse is a large win.

@dbtsai

dbtsai commented Jul 9, 2026

Copy link
Copy Markdown
Member

Follow-up ideas: further performance opportunities

The structural win here (no per-element Scalar/Array allocation + child-array caching) is solid. A few places that still leave performance on the table, roughly by impact:

  1. Hoist the numeric type-ID dispatch out of the per-element loop (biggest). NumericArray._getitem_py recomputes type_id() and walks an if/elif ladder (up to 10 branches) on every element, even though the type is invariant across the array. For an int8 column that's ~6 failed comparisons per value. Resolving the type once and running a monomorphic loop over the buffer turns the "no Scalar" win into a "vectorized" win.

  2. Share the fast path with as_py() / __iter__. _getitem_py is private and only called from to_pylist (when maps_as_pydicts is None). arr[i].as_py(), iteration, and ChunkedArray element access still allocate Scalars. Wiring those to reuse _getitem_py would extend the win to the other common access patterns.

  3. Null fast path. Every accessor calls IsNull(i) per element even when null_count == 0. Checking null_count == 0 once and skipping the per-element null test helps the very common all-non-null column.

  4. Offset reads in list/map paths. value_offset(i) / value_offset(i+1) are two virtual calls per row; reading the offsets buffer pointer once and indexing avoids that on long arrays. Also worth confirming for j in range(start, end) compiles to a C loop rather than allocating a range per row.

  5. Missing specializations that still fall back to Scalars: temporal (date/timestamp/time/duration), decimal, DictionaryArray, and string_view/binary_view have no _getitem_py. Timestamps/dates and dictionaries are common enough to be worth a fast path (dictionary especially — see the separate value-reuse comment).

None of these are blocking — the PR is a clear improvement as-is. #1 and #2 are the two with real leverage.

@pitrou

pitrou commented Jul 9, 2026

Copy link
Copy Markdown
Member

@dbtsai For the record, are these comments AI-generated? Did you vet them for correctness?

Comment thread python/pyarrow/array.pxi Outdated
Comment on lines +2483 to +2486
if tid == _Type_INT64:
return (<CInt64Array*> self.ap).Value(i)
elif tid == _Type_INT32:
return (<CInt32Array*> self.ap).Value(i)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the peculiar ordering of types? I would rather have something more regular for readability.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reordered to the regular int8..uint64, float, double sequence in 3d303ce.

Comment thread python/pyarrow/array.pxi Outdated
Comment on lines +4108 to +4115
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread python/pyarrow/array.pxi Outdated
Comment on lines +4366 to +4369
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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not raise ValueError here instead of adding this weird fallback path?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — it now raises the same ValueError as StructScalar.as_py directly (the cache/fallback dance is gone), and the test asserts the message.

Comment thread python/pyarrow/array.pxi
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):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed GH-50448 for the per-range conversion (folding in the dispatch-hoisting and null-check ideas from this thread as well), and added a TODO pointing at it.

Comment thread python/pyarrow/array.pxi Outdated
Comment on lines +4109 to +4114
cdef:
int64_t length
const uint8_t* data
if self.ap.IsNull(i):
return None
data = (<CLargeStringArray*> self.ap).GetValue(i, &length)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may be able to call GetView(i) which will give you a std::string_view.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice — done. Switched string/binary (+ large variants) to GetView(i); it also made StringViewArray/BinaryViewArray specializations trivial, so those are included now too.

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())),
]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do pa.binary_view as well?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added binary_view and string_view cases — and since GetView covers the view types, they now take real fast paths instead of the Scalar fallback.

# 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):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you test (part of) the error message as well? Something like:

Suggested change
with pytest.raises(ValueError):
with pytest.raises(ValueError, match='some regex'):

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

* 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
Copilot AI review requested due to automatic review settings July 9, 2026 14:47
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 9, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jul 9, 2026
@dbtsai

dbtsai commented Jul 9, 2026

Copy link
Copy Markdown
Member

@dbtsai For the record, are these comments AI-generated? Did you vet them for correctness?

Those were AI-generated based on my prompts for specific areas where I had questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants