-
Notifications
You must be signed in to change notification settings - Fork 915
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 AllocateLikeTest gtests reading uninitialized null-mask #12643
Fix AllocateLikeTest gtests reading uninitialized null-mask #12643
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## branch-23.04 #12643 +/- ##
===============================================
Coverage ? 85.82%
===============================================
Files ? 158
Lines ? 25184
Branches ? 0
===============================================
Hits ? 21614
Misses ? 3570
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find! I see two other uses of allocate_like
in column_device_view_test.cu
, but in that case it looks like the columns are filled before any comparison so I think this isn't an issue, right?
Yes, it is not an issue there because the uninitialized data is not read and there is no null-mask. |
/merge |
Description
Fixes a bug found in the gtests
AllocateLikeTest.ColumnNumericTestSameSize
andAllocateLikeTest.ColumnNumericTestSpecifiedSize
where uninitialized column null-masks are used to verify null-counts.This error was found by running the following in the libcudf build directory:
The
AllocateLikeTest
testscudf::allocate_like()
which produces an uninitialized fixed-width column which is usually filled in by internal functions. The uninitialized column data includes its null-mask. The gtest usesCUDF_TEST_EXPECT_COLUMN_PROPERTIES_EQUAL
to verify the column returned byallocate_like()
which also includes thenull_count()
. The null-count property returned byallocate_like()
sets the null-count toUNKNOWN_NULL_COUNT
which causes a future call tonull_count()
to calculate the count by reading the bits in the null-mask. Since the null-mask is uninitialized (garbage), the null-count would be invalid and likely not match any predictable value.The gtests are updated to check for appropriate column properties excluding the null-count.
While debugging this issue, the
allocate_like()
logic was found to also attempt to build child columns incorrectly assuming the children will contain the same size and null-mask. Since theallocate_like()
only supports fixed-width types which do not have children, this logic was removed.Checklist