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

Fix construction of nested structs with EMPTY child #10761

Merged

Conversation

shwina
Copy link
Contributor

@shwina shwina commented Apr 29, 2022

This PR adds the ability to construct a structs columns with a parent null mask, but an EMPTY child. Prior to this PR, this would fail with a runtime error. This is because the parent null mask is superimposed on the EMPTY child, but EMPTY columns cannot contain null masks.

This PR also includes a small change in the Cython, where we map libcudf's EMPTY type to a int8 type on the Python side.

@shwina shwina requested review from a team as code owners April 29, 2022 16:49
@github-actions github-actions bot added Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Apr 29, 2022
@shwina shwina added non-breaking Non-breaking change bug Something isn't working and removed libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API. labels Apr 29, 2022
@codecov
Copy link

codecov bot commented Apr 29, 2022

Codecov Report

Merging #10761 (c75dacf) into branch-22.06 (84f88ce) will decrease coverage by 0.08%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           branch-22.06   #10761      +/-   ##
================================================
- Coverage         86.40%   86.32%   -0.09%     
================================================
  Files               143      144       +1     
  Lines             22444    22656     +212     
================================================
+ Hits              19393    19558     +165     
- Misses             3051     3098      +47     
Impacted Files Coverage Δ
python/cudf/cudf/comm/gpuarrow.py 79.76% <ø> (ø)
python/cudf/cudf/core/column/string.py 88.78% <ø> (-0.31%) ⬇️
python/cudf/cudf/core/frame.py 93.41% <ø> (ø)
python/cudf/cudf/core/series.py 95.17% <ø> (+<0.01%) ⬆️
...thon/dask_cudf/dask_cudf/tests/test_distributed.py 18.86% <0.00%> (-67.93%) ⬇️
python/cudf/cudf/io/parquet.py 90.83% <0.00%> (-1.87%) ⬇️
python/dask_cudf/dask_cudf/backends.py 85.51% <0.00%> (-0.94%) ⬇️
python/cudf/cudf/core/column/categorical.py 89.37% <0.00%> (-0.61%) ⬇️
python/cudf/cudf/core/column/decimal.py 90.60% <0.00%> (-0.50%) ⬇️
python/cudf/cudf/core/column/lists.py 91.70% <0.00%> (-0.38%) ⬇️
... and 28 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 3c208a6...c75dacf. Read the comment docs.

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.

Huh, I always assumed that EMPTY just means an invalid type.

Copy link
Contributor

@davidwendt davidwendt left a comment

Choose a reason for hiding this comment

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

Is there a better way to test this? Perhaps the pytest is enough for this change.

@github-actions github-actions bot added Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Apr 29, 2022
@shwina
Copy link
Contributor Author

shwina commented Apr 29, 2022

Is there a better way to test this? Perhaps the pytest is enough for this change.

This is an issue that exists independent of Python -- simply constructing a structs column or reading an Arrow array with the specified structure would fail prior to this PR.

@shwina
Copy link
Contributor Author

shwina commented May 16, 2022

rerun tests

@shwina shwina added the 5 - Ready to Merge Testing and reviews complete, ready to merge label May 16, 2022
@shwina
Copy link
Contributor Author

shwina commented May 16, 2022

@gpucibot merge

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 libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants