Skip to content

ARROW-5531: [Python] Implement Array.from_buffers for varbinary and nested types, add DataType.num_buffers property - #4537

Closed
wesm wants to merge 2 commits into
apache:masterfrom
wesm:ARROW-5531
Closed

ARROW-5531: [Python] Implement Array.from_buffers for varbinary and nested types, add DataType.num_buffers property#4537
wesm wants to merge 2 commits into
apache:masterfrom
wesm:ARROW-5531

Conversation

@wesm

@wesmwesm commented Jun 12, 2019

Copy link
Copy Markdown
Member

Thanks to Antoine's recent work on Array::View this method can be made more robust and safe by checking for the correct number of buffers.

@wesm
wesm requested review from pitrou and xhochyJune 12, 2019 22:57

@pitroupitrou left a comment

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.

LGTM on the principle. Just a couple details.

Comment threadpython/pyarrow/array.pxi Outdated
Comment threadpython/pyarrow/tests/test_array.py Outdated

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.

Ah, I hadn't noticed that StringArray had a different from_buffers method.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Frankly I question whether it is useful, I am inclined to remove it

@xhochy

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Leaving it here for now.

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.

Remove this, it is probably only used by fletcher and pyarrow.jvm.

Comment threadpython/pyarrow/types.pxi Outdated
@wesm

wesm commented Jun 13, 2019

Copy link
Copy Markdown
MemberAuthor

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@wesm@xhochy@pitrou