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

Handle sliced structs properly in pack/contiguous_split. #8739

Merged
merged 12 commits into from
Jul 19, 2021

Conversation

nvdbaranec
Copy link
Contributor

@nvdbaranec nvdbaranec commented Jul 13, 2021

Pre-sliced struct columns (those with a positive offset) were not being handled correctly.

Dependent PR has been merged.

@nvdbaranec nvdbaranec added bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels Jul 13, 2021
@nvdbaranec nvdbaranec requested a review from a team as a code owner July 13, 2021 22:33
@nvdbaranec nvdbaranec requested review from jrhemstad and vuule July 13, 2021 22:33
@codecov
Copy link

codecov bot commented Jul 14, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@2a8d202). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.08    #8739   +/-   ##
===============================================
  Coverage                ?   10.53%           
===============================================
  Files                   ?      116           
  Lines                   ?    18916           
  Branches                ?        0           
===============================================
  Hits                    ?     1993           
  Misses                  ?    16923           
  Partials                ?        0           

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 2a8d202...542490b. Read the comment docs.

@github-actions github-actions bot added the CMake CMake build issue label Jul 14, 2021
cpp/include/cudf/detail/null_mask.cuh Outdated Show resolved Hide resolved
cpp/include/cudf/detail/null_mask.cuh Outdated Show resolved Hide resolved
@nvdbaranec nvdbaranec requested a review from a team as a code owner July 15, 2021 16:05
@nvdbaranec nvdbaranec requested a review from ttnghia July 15, 2021 16:05
Comment on lines 156 to 164
* @param[in] bitmask The bitmask whose non-zero bits outside the range in the
* boundary words will be counted.
* @param[in] num_ranges The number of ranges
* @param[in] first_bit_indices The indices (inclusive) of the first bit in each
* range
* @param[in] last_bit_indices The indices (exclusive) of the last bit in each
* range
* @param[in,out] null_counts The number of non-zero bits in each range to be
* updated
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry that I'm a bit OCD about the style. This is just a suggestion (align multiple-line param doxygen with the parameter name), you can ignore it if you don't like 😃

Suggested change
* @param[in] bitmask The bitmask whose non-zero bits outside the range in the
* boundary words will be counted.
* @param[in] num_ranges The number of ranges
* @param[in] first_bit_indices The indices (inclusive) of the first bit in each
* range
* @param[in] last_bit_indices The indices (exclusive) of the last bit in each
* range
* @param[in,out] null_counts The number of non-zero bits in each range to be
* updated
* @param[in] bitmask The bitmask whose non-zero bits outside the range in the
* boundary words will be counted.
* @param[in] num_ranges The number of ranges
* @param[in] first_bit_indices The indices (inclusive) of the first bit in each
* range
* @param[in] last_bit_indices The indices (exclusive) of the last bit in each
* range
* @param[in,out] null_counts The number of non-zero bits in each range to be
* updated

/**
* @brief Functor that returns the number of set bits for a specified word
* of a bitmask array.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*

* or exclusive.
* @param[in] d_bit_indices Pointer to an array of bit indices
*/
__host__ to_word_index(bool inclusive, size_type const* d_bit_indices)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we ever need the __host__ qualifier here.

Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

CMake changes LGTM

@github-actions github-actions bot removed the CMake CMake build issue label Jul 19, 2021
@nvdbaranec nvdbaranec added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Jul 19, 2021
@nvdbaranec
Copy link
Contributor Author

Temporarily adding do-not-merge tag so @charlesbluca can run some additional tests.

@nvdbaranec nvdbaranec requested review from ttnghia and vuule July 19, 2021 18:48
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

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

still looks good :)

@nvdbaranec nvdbaranec removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Jul 19, 2021
@nvdbaranec
Copy link
Contributor Author

@gpucibot merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants