Skip to content

GH-36682: [C++] Add missing ARROW_ACERO dependency to ARROW_PYTHON #36681

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

Closed
kou wants to merge 1 commit into from
Closed

GH-36682: [C++] Add missing ARROW_ACERO dependency to ARROW_PYTHON #36681

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cpp/cmake_modules/DefineOptions.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,7 @@ takes precedence over ccache if a storage backend is configured" ON)
(This is a deprecated option. Use CMake presets instead.)"
OFF
DEPENDS
ARROW_ACERO
Copy link
Member

@raulcd raulcd Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I am not entirely sure I understand why are we doing this change. Should all ARROW_PYTHON dependencies be added? what about S3 or GCS?

Copy link
Member

@jorisvandenbossche jorisvandenbossche Jul 14, 2023

Choose a reason for hiding this comment

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

Also, shouldn't we in theory not rely on this in our own CI, given that it is a deprecated option?

If the minimal python build needs this, then we should just enable ARROW_ACERO directly in that build? (or update it to not require acero)

Copy link
Member Author

@kou kou Jul 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry. This is needless.
I thought that this is the cause of #36680 but it was wrong: #36683

I thought that this is still needed because #34711 removes ARROW_COMPUTE from ARROW_PYTHON instead of replacing ARROW_COMPUTE with ARROW_ACERO.
But it's correct because ARROW_PYTHON depends on ARROW_ACERO implicitly: ARROW_PYTHON -> ARROW_DATASET -> ARROW_ACERO

ARROW_CSV
ARROW_DATASET
ARROW_FILESYSTEM
Expand Down