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

[BUG] gathering structs can produce valid child values underneath null struct parent #9188

Closed
jlowe opened this issue Sep 7, 2021 · 5 comments · Fixed by #9194
Closed

[BUG] gathering structs can produce valid child values underneath null struct parent #9188

jlowe opened this issue Sep 7, 2021 · 5 comments · Fixed by #9194
Assignees
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. Spark Functionality that helps Spark RAPIDS

Comments

@jlowe
Copy link
Contributor

jlowe commented Sep 7, 2021

Describe the bug
Performing a gather on a non-nullable struct column with non-nullable child columns can produce a result where the parent has null rows but the corresponding child rows are not null.

Steps/Code to reproduce bug
Create a non-nullable struct column with non-nullable child columns and perform a gather with some of the gather map indices being out-of-bounds (i.e.: -1) and specify that out-of-bounds gather indices should result in null values during the gather.

Expected behavior
If the gather operation introduces a null struct row, the corresponding child rows should also be null.

@jlowe jlowe added bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. Spark Functionality that helps Spark RAPIDS labels Sep 7, 2021
@jlowe
Copy link
Contributor Author

jlowe commented Sep 7, 2021

This appears to be fixed with the following patch. I noticed the logic at https://github.com/rapidsai/cudf/blob/branch-21.10/cpp/include/cudf/detail/gather.cuh#L663-L666 was not being applied in the struct column handling of child columns.

diff --git a/cpp/include/cudf/detail/gather.cuh b/cpp/include/cudf/detail/gather.cuh
index 1dd0d472d0..6b1e9b350c 100644
--- a/cpp/include/cudf/detail/gather.cuh
+++ b/cpp/include/cudf/detail/gather.cuh
@@ -470,7 +470,7 @@ struct column_gatherer_impl<struct_view> {
                                                                          mr);
                    });
 
-    auto const nullable = std::any_of(structs_column.child_begin(),
+    auto const nullable = nullify_out_of_bounds || std::any_of(structs_column.child_begin(),
                                       structs_column.child_end(),
                                       [](auto const& col) { return col.nullable(); });
     if (nullable) {

I'm guessing this wasn't caught by the unit tests since they are using nullable child columns.

@ttnghia
Copy link
Contributor

ttnghia commented Sep 7, 2021

I noticed the logic at https://github.com/rapidsai/cudf/blob/branch-21.10/cpp/include/cudf/detail/gather.cuh#L663-L666 was not being applied in the struct column handling of child columns.

Yeah that is because when implemented it I used the assumption that the children columns have always been superimposed their parent nulls onto.

@jlowe
Copy link
Contributor Author

jlowe commented Sep 7, 2021

that is because when implemented it I used the assumption that the children columns have always been superimposed their parent nulls onto.

In this case the input columns are correctly reflecting the parent nullability. Nothing is nullable in the input, parent struct or otherwise, yet it produces the wrong output. When nullify_out_of_bounds is enabled, nulls can appear in the output when none appear in the input.

@ttnghia
Copy link
Contributor

ttnghia commented Sep 8, 2021

Thanks Jason. That's definitely my bug. Thanks @mythrocks for taking over this. I'm sorry that it costs you guys time to deal with it 😞

mythrocks added a commit to mythrocks/cudf that referenced this issue Sep 8, 2021
Fixes rapidsai#9188.

Prior to this fix, when `cudf::gather()` is called on a STRUCT input
column, the null masks of the children of the result column would not
be set correctly if the child columns do not contain nulls.

This fix enforces null mask calculation if NULLIFY is set.
mythrocks added a commit to mythrocks/cudf that referenced this issue Sep 8, 2021
Fixes rapidsai#9188.

Prior to this fix, when `cudf::gather()` is called on a STRUCT input
column, the null masks of the children of the result column would not
be set correctly if the child columns do not contain nulls.

This fix enforces null mask calculation if NULLIFY is set.
@mythrocks
Copy link
Contributor

mythrocks commented Sep 8, 2021

That's definitely my bug.

It was a subtle bug. :/ I've been operating under the assumption that it was my doing. :] If my STRUCT tests were better, we would have caught this sooner. A fix is inbound.

rapids-bot bot pushed a commit that referenced this issue Sep 13, 2021
Fixes #9188.

Prior to this fix, when `cudf::gather()` is called on a STRUCT input
column, the null masks of the children of the result column would not
be set correctly if the child columns do not contain nulls.

This fix enforces null mask calculation if `NULLIFY` is set.

In addition, this commit also cleans up the `TypedStructGatherTest` test suite:
1. IIFEs for `STRUCT` column construction.
2. Corrections for `assert()` conditions.
3. Switched `column` construction to `column_wrapper` and `column_view`.
4. Corrected the output null mask for `TestGatherStructOfListOfStructs`.
5. Added repro test for #9188.

Authors:
  - MithunR (https://github.com/mythrocks)

Approvers:
  - Nghia Truong (https://github.com/ttnghia)
  - Jason Lowe (https://github.com/jlowe)
  - Karthikeyan (https://github.com/karthikeyann)

URL: #9194
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. Spark Functionality that helps Spark RAPIDS
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants