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.

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

Enforce boolean ascending for dask-cudf sort_values #9814

Merged

Conversation

charlesbluca
Copy link
Member

It is possible to pass a list of ascending booleans to dask-cudf's sort_values, which is not yet supported by quantile_divisions (which computes divisions for all sort-by columns sorted in ascending order), and can cause undefined behavior.

This small PR adds a check that ascending is a boolean before computing quantile divisions; note that this check happens after the single-partition case is handled, as cuDF can handle lists of ascending booleans.

@charlesbluca charlesbluca added bug Something isn't working 3 - Ready for Review Ready for review by team dask-cudf non-breaking Non-breaking change labels Dec 1, 2021
@charlesbluca charlesbluca requested a review from a team as a code owner December 1, 2021 18:20
@github-actions github-actions bot added the Python Affects Python cuDF API. label Dec 1, 2021
@codecov
Copy link

codecov bot commented Dec 1, 2021

Codecov Report

Merging #9814 (951c3dc) into branch-22.02 (967a333) will decrease coverage by 0.35%.
The diff coverage is 9.91%.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.02    #9814      +/-   ##
================================================
- Coverage         10.49%   10.13%   -0.36%     
================================================
  Files               119      119              
  Lines             20305    21557    +1252     
================================================
+ Hits               2130     2185      +55     
- Misses            18175    19372    +1197     
Impacted Files Coverage Δ
python/cudf/cudf/__init__.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/column.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/string.py 0.00% <ø> (ø)
python/cudf/cudf/core/frame.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/groupby/groupby.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/index.py 0.00% <ø> (ø)
python/cudf/cudf/core/indexed_frame.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/multiindex.py 0.00% <0.00%> (ø)
python/cudf/cudf/utils/utils.py 0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/sorting.py 92.30% <50.00%> (-0.61%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11c3dfe...951c3dc. Read the comment docs.

@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Dec 7, 2021
@galipremsagar
Copy link
Contributor

rerun tests

@galipremsagar
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit a72f19e into rapidsai:branch-22.02 Dec 7, 2021
@charlesbluca charlesbluca deleted the dask-prevent-list-asc branch July 19, 2022 14:26
@vyasr vyasr added dask Dask issue and removed dask-cudf labels Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working dask Dask issue non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants