Skip to content

ARROW-17160: [C++] Create a base directory for PyArrow CPP header files #14275

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.

Sign up for GitHub

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

Jump to bottom
Merged
jorisvandenbossche merged 6 commits into from
Oct 10, 2022
Merged

ARROW-17160: [C++] Create a base directory for PyArrow CPP header files #14275

jorisvandenbossche merged 6 commits into from
Oct 10, 2022

Conversation

@AlenkaF
Copy link
Member

@AlenkaF AlenkaF commented Sep 29, 2022

This PR is a follow-up of #13311 where it was decided to change the base directory for PyArrow C++ headers to avoid top level inclusions. See #13311 (comment)

@github-actions
Copy link

github-actions bot commented Sep 29, 2022

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

kou
kou reviewed Sep 29, 2022
View reviewed changes
@AlenkaF AlenkaF mentioned this pull request Oct 4, 2022
Merged
@AlenkaF AlenkaF marked this pull request as ready for review October 6, 2022 09:09
jorisvandenbossche
jorisvandenbossche approved these changes Oct 6, 2022
View reviewed changes
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

There is still the question about moving the include paths from "arrow/python/.." to "pyarrow/.." (and how to provide some transition path for this), but assuming the current status of exposing those as "arrow/python/.." (which seems the safe assumption for 10.0), this seems like a good change to use those paths in the code and headers itself.

pitrou
pitrou reviewed Oct 6, 2022
View reviewed changes
@AlenkaF
Copy link
Member Author

AlenkaF commented Oct 7, 2022

There is still the question about moving the include paths from "arrow/python/.." to "pyarrow/.." (and how to provide some transition path for this), but assuming the current status of exposing those as "arrow/python/.." (which seems the safe assumption for 10.0), this seems like a good change to use those paths in the code and headers itself.

I suggest we try to move the include paths from "arrow/python/.." to "pyarrow/.." for a future release. I can create a separate JIRA issue for it (guessing with Joris we will want to change namespaces from arrow&python to pyarrow also?). Or do you want to tackle this as a part of ARROW-17838 @kou?

@kou
Copy link
Member

kou commented Oct 7, 2022

Could you create a separate JIRA issue for it?

@AlenkaF
Copy link
Member Author

AlenkaF commented Oct 7, 2022

Done: https://issues.apache.org/jira/browse/ARROW-17958

@kiszk
Copy link
Member

kiszk commented Oct 7, 2022 •
edited
Loading

I am sorry for rerunning this travis ci job at https://app.travis-ci.com/github/apache/arrow/builds/256419696 while the job was already succeeded.

@AlenkaF
Copy link
Member Author

AlenkaF commented Oct 7, 2022

I am sorry for rerunning this travis ci job at https://app.travis-ci.com/github/apache/arrow/builds/256419696 while the job was already succeeded.

No problem at all =)

@AlenkaF AlenkaF requested a review from pitrou October 7, 2022 08:11
pitrou
pitrou approved these changes Oct 7, 2022
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thank you very much @AlenkaF !

@pitrou
Copy link
Member

pitrou commented Oct 7, 2022

@kou Do you have any other concerns?

@kou
Copy link
Member

kou commented Oct 8, 2022

No.

@AlenkaF
Copy link
Member Author

AlenkaF commented Oct 10, 2022

I will merge later today, if there is no objection =)

@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Oct 10, 2022

The Python failure is one of the flaky dataset tests

@jorisvandenbossche jorisvandenbossche merged commit 21dbf4a into apache:master Oct 10, 2022
@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Oct 10, 2022

Thanks @AlenkaF !

@ursabot
Copy link

ursabot commented Oct 10, 2022

Benchmark runs are scheduled for baseline = d8f3946 and contender = 21dbf4a. 21dbf4a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.56% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.55% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.25% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 21dbf4ac ec2-t3-xlarge-us-east-2
[Failed] 21dbf4ac test-mac-arm
[Failed] 21dbf4ac ursa-i9-9960x
[Finished] 21dbf4ac ursa-thinkcentre-m75q
[Finished] d8f39460 ec2-t3-xlarge-us-east-2
[Failed] d8f39460 test-mac-arm
[Failed] d8f39460 ursa-i9-9960x
[Finished] d8f39460 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@AlenkaF AlenkaF deleted the ARROW-17160 branch October 11, 2022 03:43
AlenkaF added a commit that referenced this pull request Oct 11, 2022
@AlenkaF
ARROW-17831: [Python][Docs] PyArrow Architecture page outdated after …
3d7f2f2 
…moving pyarrow C++ code (#14311)

Keeping it as draft until #14275 is resolved and final structure is decided.

Authored-by: Alenka Frim <frim.alenka@gmail.com>
Signed-off-by: Alenka Frim <frim.alenka@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kou kou kou left review comments

@jorisvandenbossche jorisvandenbossche jorisvandenbossche approved these changes

@pitrou pitrou pitrou approved these changes

Assignees

No one assigned

Labels

Component: Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@AlenkaF @kou @kiszk @pitrou @jorisvandenbossche @ursabot