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

Remove default UNKNOWN_NULL_COUNT from cudf::column member functions #13341

Merged
merged 18 commits into from
May 18, 2023
Merged

Remove default UNKNOWN_NULL_COUNT from cudf::column member functions #13341

merged 18 commits into from
May 18, 2023

Conversation

davidwendt
Copy link
Contributor

@davidwendt davidwendt commented May 11, 2023 •
edited
Loading

Description

Remove the default parameters for null-mask and null-count from cudf::column constructors and set_null_mask member functions.

Reference #13311

Checklist

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

@davidwendt davidwendt added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 11, 2023
@davidwendt davidwendt self-assigned this May 11, 2023
@vyasr vyasr mentioned this pull request May 11, 2023
Merged
3 tasks
@github-actions github-actions bot added the Python Affects Python cuDF API. label May 12, 2023
@github-actions github-actions bot added the Java Affects Java cuDF API. label May 12, 2023
@davidwendt davidwendt marked this pull request as ready for review May 16, 2023 18:35
@davidwendt davidwendt requested review from a team as code owners May 16, 2023 18:35
vyasr
vyasr requested changes May 17, 2023
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Mostly looks great to me! However IIUC about 50% of the changes are in the concatenate_tests.cpp file, and they don't seem relevant. That ratio is too high for me, so unless I'm misunderstanding why that's here I would like those changes split into a separate PR.

cpp/src/interop/from_arrow.cu Outdated Show resolved Hide resolved
cpp/tests/copying/concatenate_tests.cpp Outdated Show resolved Hide resolved
cpp/tests/copying/concatenate_tests.cpp Outdated Show resolved Hide resolved
@davidwendt davidwendt requested a review from vyasr May 17, 2023 18:18
@vyasr vyasr mentioned this pull request May 17, 2023
Merged
3 tasks
vyasr
vyasr approved these changes May 17, 2023
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels May 17, 2023
mythrocks
mythrocks approved these changes May 18, 2023
View reviewed changes
Copy link
Contributor

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

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

LGTM!

@davidwendt
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit c59c2fa into rapidsai:branch-23.06 May 18, 2023
@davidwendt davidwendt deleted the column-null-count branch May 18, 2023 21:00
@ChuckHastings
Copy link
Contributor

Shouldn't this have been marked BREAKING? It broke cugraph.

The constructor for cudf::column now requires all 4 parameters to be passed, so any code that relied on the default for the fourth parameter is now broken.

@ChuckHastings ChuckHastings mentioned this pull request May 19, 2023
Merged
rapids-bot bot pushed a commit to rapidsai/cugraph that referenced this pull request May 19, 2023
@ChuckHastings
update cudf column constructor calls (#3592)
6eb4f0a 
rapidsai/cudf#13341 changed the constructors for `cudf::column`.

This PR updates the constructor calls to work with the latest cudf changes.

Authors:
  - Chuck Hastings (https://github.com/ChuckHastings)

Approvers:
  - Seunghwa Kang (https://github.com/seunghwak)

URL: #3592
@vyasr
Copy link
Contributor

vyasr commented May 19, 2023

Yes, this should have been marked as breaking. Apologies for the oversight. @raydouglass will updating the labels now have any effect, or will we need to modify the changelog etc manually to account for this now?

@raydouglass
Copy link
Member

will updating the labels now have any effect

Yes, the changelog is generated from scratch each time, so if the labels are updated the next PR that merges will update the entire changelog.

@ChuckHastings
Copy link
Contributor

I'm not pressed about retroactively worrying about this, personally. The breaking label helps me by alerting me in the slack channel that changes might come up that might affect my development. In this case it was pretty easy to find what the culprit was (the error message clearly indicated a problem in this constructor... not too many PRs merged recently caused a change in the constructor.

I don't know how the labels propagate into release documentation. If the labels have an effect on that, I suppose it would be helpful to get that tagged correctly so that it's represented in the release documentation. If that's all still manual then it's probably less important.

@vyasr vyasr added breaking Breaking change and removed non-breaking Non-breaking change labels May 19, 2023
@vyasr
Copy link
Contributor

vyasr commented May 19, 2023

Sounds good, updated the labels.

trxcllnt added a commit to trxcllnt/cuspatial that referenced this pull request May 22, 2023
@trxcllnt
update cudf calls after rapidsai/cudf#13341
995f81e
trxcllnt added a commit to trxcllnt/cuspatial that referenced this pull request May 23, 2023
@trxcllnt
update cudf calls after rapidsai/cudf#13341
1f635b7
@trxcllnt trxcllnt mentioned this pull request May 23, 2023
Merged
3 tasks
rapids-bot bot pushed a commit that referenced this pull request May 24, 2023
@vyasr
Remove UNKNOWN_NULL_COUNT (#13372)
56150d9 
This is the final PR for removing `UNKNOWN_NULL_COUNT` and the implicit kernel launch in the `null_count` methods of `column` and `column_view`. 

Depends on #13355 and #13341.

Closes #11968

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - MithunR (https://github.com/mythrocks)
  - Nghia Truong (https://github.com/ttnghia)
  - Karthikeyan (https://github.com/karthikeyann)

URL: #13372
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request May 26, 2023
@trxcllnt
Fix cudf::column constructor args (#1151)
5162e39 
Update `cudf::column` constructor args to match the changes in rapidsai/cudf#13341.

Also corrects a minor issue in the docs, closes #1154.

Authors:
  - Paul Taylor (https://github.com/trxcllnt)
  - Taurean Dyer (https://github.com/taureandyernv)
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Michael Wang (https://github.com/isVoid)
  - Mark Harris (https://github.com/harrism)

URL: #1151
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlowe jlowe jlowe approved these changes

@vyasr vyasr vyasr approved these changes

@mythrocks mythrocks mythrocks approved these changes

@galipremsagar galipremsagar galipremsagar 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

@bdice bdice Awaiting requested review from bdice bdice is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

Assignees

@davidwendt davidwendt

Labels
3 - Ready for Review Ready for review by team breaking Breaking change improvement Improvement / enhancement to an existing function Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@davidwendt @ChuckHastings @vyasr @raydouglass @jlowe @mythrocks @galipremsagar