-
Notifications
You must be signed in to change notification settings - Fork 4.2k
GH-50326: [Python] Convert arrays to Python objects without per-element Scalars in to_pylist #50327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
12b921e
7a15077
985a2e3
728584f
3d303ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -465,6 +465,56 @@ def test_array_getitem_numpy_scalars(): | |||||
| assert arr[np.int32(idx)].as_py() == lst[idx] | ||||||
|
|
||||||
|
|
||||||
| def test_to_pylist_bulk_paths(): | ||||||
| # GH-50326: to_pylist converts through scalar-free _getitem_py | ||||||
| # specializations; the result must match the per-scalar conversion | ||||||
| # exactly. | ||||||
| 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([b"a\x00b", None, b"", b"\xff"], type=pa.binary()), | ||||||
| pa.array([b"a\x00b", None, b""], type=pa.large_binary()), | ||||||
| pa.array([[b"x", None, b"\x00y"], None, []], | ||||||
| type=pa.list_(pa.binary())), | ||||||
| pa.array([1, None, -(2**62), 2**62], type=pa.int64()), | ||||||
| pa.array([0, None, 2**63 + 7], type=pa.uint64()), | ||||||
| pa.array([-128, 127, None], type=pa.int8()), | ||||||
| pa.array([1.5, None, -0.5], type=pa.float64()), | ||||||
| pa.array([1.5, None], type=pa.float32()), | ||||||
| pa.array([True, None, False], type=pa.bool_()), | ||||||
| pa.array([{"a": 1, "b": "x"}, None, {"a": None, "b": None}], | ||||||
| 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())), | ||||||
| ] | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we do
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added |
||||||
| for arr in arrays: | ||||||
| for view in (arr, arr.slice(1), arr.slice(0, 2), arr.slice(2)): | ||||||
| assert view.to_pylist() == [x.as_py() for x in view] | ||||||
|
|
||||||
| # Values inside numeric lists must stay Python ints/None, never floats | ||||||
| result = pa.array([[1, None, 3]], type=pa.list_(pa.int32())).to_pylist() | ||||||
| assert result == [[1, None, 3]] | ||||||
| assert [type(x) for x in result[0]] == [int, type(None), int] | ||||||
|
|
||||||
| # 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): | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||||||
| dup.to_pylist() | ||||||
|
|
||||||
|
|
||||||
| def test_array_slice(): | ||||||
| arr = pa.array(range(10)) | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.