-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Conversation
|
@github-actions crossbow submit example-python-minimal-* |
|
|
This comment was marked as outdated.
This comment was marked as outdated.
|
@github-actions crossbow submit example-python-minimal-* |
This comment was marked as outdated.
This comment was marked as outdated.
34f72d9 to
fe14c3f
Compare
|
I've opened #36683 for the |
| (This is a deprecated option. Use CMake presets instead.)" | ||
| OFF | ||
| DEPENDS | ||
| ARROW_ACERO |
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.
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?
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.
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)
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.
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
|
I withdraw this because this is needless. |
|
|
Rationale for this change
PyArrow wants Acero.
What changes are included in this PR?
Add
ARROW_ACEROtoARROW_PYTHONdependency.Are these changes tested?
Yes.
Are there any user-facing changes?
Yes.