Skip to content

ARROW-7913: [C++][Python][R] C++ implementation of C data interface - #6483

Closed
pitrou wants to merge 6 commits into
apache:masterfrom
pitrou:ARROW-7913-c-data-interface-impl
Closed

ARROW-7913: [C++][Python][R] C++ implementation of C data interface#6483
pitrou wants to merge 6 commits into
apache:masterfrom
pitrou:ARROW-7913-c-data-interface-impl

Conversation

@pitrou

@pitroupitrou commented Feb 24, 2020

Copy link
Copy Markdown
Member

No description provided.

@pitroupitrou mentioned this pull request Feb 24, 2020
Closed
1 task
@github-actions

github-actionsBot commented Feb 24, 2020

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-7913

@pitrou

pitrou commented Feb 24, 2020

Copy link
Copy Markdown
MemberAuthor

Should I add extension type support in this PR or open a JIRA to do it later?

@nealrichardson

nealrichardson commented Feb 25, 2020

Copy link
Copy Markdown
Member

For me, I'm fine with a followup. Think we'll also need a followup to handle ChunkedArray and Table anyway, IIRC.

@bkietz
bkietz self-requested a review February 25, 2020 14:36
bkietz
bkietz requested changes Feb 25, 2020
View reviewed changes

@bkietzbkietz 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.

Thanks for doing this!

Lots of comments:

Comment threadcpp/src/arrow/c/bridge.h Outdated
Comment threadci/docker/linux-apt-r.dockerfile Outdated

@bkietzbkietzFeb 25, 2020

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.

Should this be a call to update-alternatives or so?

@pitroupitrouFeb 26, 2020

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.

I have no idea. At least I don't see a /etc/alternatives/python on Ubuntu.

Comment threadci/docker/linux-apt-r.dockerfile Outdated
Comment threadcpp/src/arrow/array/builder_time.h Outdated
Comment threadcpp/src/arrow/c/helpers.h Outdated
Comment threadcpp/src/arrow/c/bridge.cc Outdated

@bkietzbkietzFeb 25, 2020

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.

Suggested change
Status ImportNullBitmap(int32_t buffer_id = 0) {
Status ImportNullBitmap() {

@pitroupitrouFeb 26, 2020

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.

I think it's fine as is.

Comment threadcpp/src/arrow/c/bridge.cc Outdated
Comment threadcpp/src/arrow/c/bridge.cc Outdated
Comment threadr/tests/testthat/test-python.R Outdated
Comment threadpython/pyarrow/cffi.py Outdated

@bkietzbkietzFeb 25, 2020

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.

Suggested change
# TODO use out-of-line mode for faster import and avoid C parsing
# TODO(pitrou) use out-of-line mode for faster import and avoid C parsing

bkietz
bkietz requested changes Feb 25, 2020
View reviewed changes

@bkietzbkietz 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.

Thanks for doing this!

Lots of comments:

@pitrou
pitrouforce-pushed the ARROW-7913-c-data-interface-impl branch 5 times, most recently from c2606d6 to c4e3551CompareMarch 2, 2020 14:08
@pitrou

pitrou commented Mar 2, 2020

Copy link
Copy Markdown
MemberAuthor

Rebasing.

@pitrou
pitrouforce-pushed the ARROW-7913-c-data-interface-impl branch from c4e3551 to d18ec1dCompareMarch 2, 2020 14:23
@nealrichardson

nealrichardson commented Mar 2, 2020

Copy link
Copy Markdown
Member

CI is green; I'd like to merge now. @bkietz do you consent?

bkietz
bkietz approved these changes Mar 2, 2020
View reviewed changes

@bkietzbkietz 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.

merging

@bkietzbkietz closed this in 24ce242Mar 2, 2020
@pitrou
pitrou deleted the ARROW-7913-c-data-interface-impl branch March 2, 2020 20:01
@asfimportasfimport mentioned this pull request Mar 2, 2020
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nealrichardsonnealrichardsonnealrichardson left review comments
@bkietzbkietzbkietz approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pitrou@nealrichardson@bkietz