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

Remove null mask and null count from column_view constructors #13311

Merged
merged 7 commits into from
May 16, 2023

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented May 9, 2023

Description

This is a breaking change that removes default values. Removing the default null count value of UNKNOWN_NULL_COUNT is necessary since we are removing UNKNOWN_NULL_COUNT altogether. A potential alternative was setting the default to 0, which would correspond to a default null mask of nullptr (the current default). However, that change is potentially more dangerous if callers were previously setting the null mask to something other than nullptr without setting the null count and relying on the behavior of UNKNOWN_NULL_COUNT. Removing the default parameter altogether is therefore the safer option here.

Contributes to #11968.

Checklist

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

@vyasr vyasr added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function breaking Breaking change labels May 9, 2023
@vyasr vyasr added this to the Enable streams milestone May 9, 2023
@vyasr vyasr self-assigned this May 9, 2023
@vyasr vyasr requested a review from a team as a code owner May 9, 2023 00:13
@vyasr vyasr requested review from robertmaynard and vuule May 9, 2023 00:13
@vyasr vyasr changed the title Remove default null count value from column_view constructors Remove default UNKNOWN_NULL_COUNT value from column_view constructors May 9, 2023
@GregoryKimball
Copy link
Contributor

@etseidl I'd like to make sure you are aware of this work. Would you please let me know if you are using mutable_column_view in your application? If so, I recommend checking the null mask and null count handling to make sure the data structures stay consistent.

@vyasr
Copy link
Contributor Author

vyasr commented May 11, 2023

To be clear, @etseidl this particular PR won't affect anything major regarding the mutable column view. The problematic case will be in a subsequent PR (related to this overall project). The relevant change will affect the following current behavior:

  1. Create and populate a column
  2. Create a mutable view of that column
  3. Modify the null mask via the mutable view in place

Currently, the creation of the mutable view (step 2) unsets the null count in the column, so the next time that the column's null count is queried it is recomputed based on the mask. In the future, that lazy computation of the count will be removed, so the user will be required to set the null count as part of step 3 (setting the mask alone will be insufficient).

@vyasr vyasr requested a review from a team as a code owner May 11, 2023 23:00
@github-actions github-actions bot added the Java Affects Java cuDF API. label May 11, 2023
@vyasr vyasr requested a review from ttnghia May 11, 2023 23:06
cpp/src/binaryop/compiled/binary_ops.cu Outdated Show resolved Hide resolved
cpp/src/dictionary/detail/concatenate.cu Outdated Show resolved Hide resolved
@vyasr vyasr changed the title Remove default UNKNOWN_NULL_COUNT value from column_view constructors Remove null mask and null count from column_view constructors May 12, 2023
@vyasr vyasr requested a review from vuule May 16, 2023 00:02
@vyasr
Copy link
Contributor Author

vyasr commented May 16, 2023

/merge

@rapids-bot rapids-bot bot merged commit 9619439 into rapidsai:branch-23.06 May 16, 2023
51 checks passed
@vyasr vyasr deleted the feat/column_view_null_count branch May 16, 2023 16:24
rapids-bot bot pushed a commit that referenced this pull request May 18, 2023
…13341)

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

Reference #13311

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

Approvers:
  - Jason Lowe (https://github.com/jlowe)
  - GALI PREM SAGAR (https://github.com/galipremsagar)
  - Vyas Ramasubramani (https://github.com/vyasr)
  - MithunR (https://github.com/mythrocks)

URL: #13341
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants