Add in negative size checks for columns #12118
Conversation
revans2
added
bug
| @@ -133,6 +133,7 @@ class column { | |||
| _null_count{null_count}, | |||
| _children{std::move(children)} | |||
| { | |||
| CUDF_EXPECTS(size >= 0, "Column size cannot be negative."); | |||
There was a problem hiding this comment.
This should be the only one you need. The other ones are redundant since the all call through to here.
There was a problem hiding this comment.
That is correct, but I added the other checks as a way to skip allocating negative data and null mask buffers with a negative size if we know early on that it is not needed. If you want me to remove the other checks I will.
There was a problem hiding this comment.
Nope, that makes sense. We should leave those in.
The other thing to do here is to add @throw tag to the doxygen comments for each of these
* @throws cudf::logic_error if `size < 0`
The column_factories ones go into cpp/include/cudf/column/column_factories.hpp
Codecov ReportBase: 87.47% // Head: 88.12% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## branch-22.12 #12118 +/- ##
================================================
+ Coverage 87.47% 88.12% +0.64%
================================================
Files 133 135 +2
Lines 21826 22133 +307
================================================
+ Hits 19093 19504 +411
+ Misses 2733 2629 -104
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. |
|
@davidwendt thanks for the review. I added in the doxegen tags. |
Can you move the |
|
@davidwendt sorry about getting the placement wrong. Hopefully it is fixed now. |
|
@gpucibot merge |
Description
This fixes #12116
This just adds in a few checks for negative sizes to avoid any issues with rounding errors and also helps us detect errors sooner. It will not fix small negative allocations for device buffers directly.
Checklist