Skip to content
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

Add partition-wise Select support to cuDF-Polars #17495

Merged
merged 17 commits into from
Dec 18, 2024
Merged

Add partition-wise Select support to cuDF-Polars #17495

merged 17 commits into from
Dec 18, 2024

Conversation

rjzamora
Copy link
Member

@rjzamora rjzamora commented Dec 3, 2024

Description

Adds multi-partition (partition-wise) Select support following the same design as #17441

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@rjzamora rjzamora added feature request New feature or request 2 - In Progress Currently a work in progress non-breaking Non-breaking change cudf.polars Issues specific to cudf.polars labels Dec 3, 2024
@rjzamora rjzamora self-assigned this Dec 3, 2024
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Dec 3, 2024

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@github-actions github-actions bot added the Python Affects Python cuDF API. label Dec 3, 2024
@rjzamora rjzamora changed the title Add partition-wise Select support to cuDF-Polars Add partition-wise Select support to cuDF-Polars Dec 3, 2024
@rjzamora
Copy link
Member Author

rjzamora commented Dec 4, 2024

/ok to test

@rjzamora rjzamora marked this pull request as ready for review December 6, 2024 04:23
@rjzamora rjzamora requested a review from a team as a code owner December 6, 2024 04:23
@rjzamora
Copy link
Member Author

Thanks for the suggestions @wence- - I think this is ready for a final review.

wence-
wence- approved these changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@wence- wence- left a comment

Choose a reason for hiding this comment

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

One tiny fix required, but I like how this didn't need too much boilerplate in the end.

python/cudf_polars/cudf_polars/dsl/expressions/unary.py Outdated Show resolved Hide resolved
python/cudf_polars/tests/experimental/test_select.py
Comment on lines +44 to +48
with pytest.raises(
pl.exceptions.ComputeError,
match="NotImplementedError",
):
assert_gpu_result_equal(query, engine=engine)
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer assert_ir_translation_raises for these kind of things, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Parallel execution is currently independent of IR translation. When we raise an error, it's because we ran into a non-"pointwise" Select operation (with multiple partitions) after the IR was already translated successfully.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah, thanks.

I think for now this is fine, perhaps in the parallel execution environment we don't want "early/eager" fallback. But it might be worthwhile thinking about.

We can think of this lowering as another step in the "IR translation" phase.

@rjzamora rjzamora added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 2 - In Progress Currently a work in progress labels Dec 18, 2024
@rjzamora
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit 0ba1eb9 into rapidsai:branch-25.02 Dec 18, 2024
107 checks passed
@rjzamora rjzamora deleted the cudf-polars-multi-select branch December 18, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wence- wence- wence- approved these changes

@brandon-b-miller brandon-b-miller Awaiting requested review from brandon-b-miller brandon-b-miller is a code owner automatically assigned from rapidsai/cudf-python-codeowners

Assignees

@rjzamora rjzamora

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge cudf.polars Issues specific to cudf.polars feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
cuDF Python
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rjzamora @wence-