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.

Sign up for GitHub

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

Add in negative size checks for columns #12118

Merged
merged 3 commits into from
Nov 15, 2022
Merged

Add in negative size checks for columns #12118

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1b4badc
Add in negative size checks for columns
revans2 Nov 10, 2022
3ccd034
Addressed review comments
revans2 Nov 14, 2022
21d0ab2
Fixed doxegen tags
revans2 Nov 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cpp/include/cudf/column/column.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ class column {
_null_count{null_count},
_children{std::move(children)}
{
CUDF_EXPECTS(size >= 0, "Column size cannot be negative.");
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be the only one you need. The other ones are redundant since the all call through to here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

/**
Expand Down
5 changes: 5 additions & 0 deletions cpp/src/column/column_factories.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ std::unique_ptr<column> make_numeric_column(data_type type,
{
CUDF_FUNC_RANGE();
CUDF_EXPECTS(is_numeric(type), "Invalid, non-numeric type.");
CUDF_EXPECTS(size >= 0, "Column size cannot be negative.");

return std::make_unique<column>(type,
size,
Expand All @@ -97,6 +98,7 @@ std::unique_ptr<column> make_fixed_point_column(data_type type,
{
CUDF_FUNC_RANGE();
CUDF_EXPECTS(is_fixed_point(type), "Invalid, non-fixed_point type.");
CUDF_EXPECTS(size >= 0, "Column size cannot be negative.");

return std::make_unique<column>(type,
size,
Expand All @@ -115,6 +117,7 @@ std::unique_ptr<column> make_timestamp_column(data_type type,
{
CUDF_FUNC_RANGE();
CUDF_EXPECTS(is_timestamp(type), "Invalid, non-timestamp type.");
CUDF_EXPECTS(size >= 0, "Column size cannot be negative.");

return std::make_unique<column>(type,
size,
Expand All @@ -133,6 +136,7 @@ std::unique_ptr<column> make_duration_column(data_type type,
{
CUDF_FUNC_RANGE();
CUDF_EXPECTS(is_duration(type), "Invalid, non-duration type.");
CUDF_EXPECTS(size >= 0, "Column size cannot be negative.");

return std::make_unique<column>(type,
size,
Expand Down Expand Up @@ -166,6 +170,7 @@ std::unique_ptr<column> make_dictionary_from_scalar(scalar const& s,
rmm::mr::device_memory_resource* mr)
{
if (size == 0) return make_empty_column(type_id::DICTIONARY32);
CUDF_EXPECTS(size >= 0, "Column size cannot be negative.");
CUDF_EXPECTS(s.is_valid(stream), "cannot create a dictionary with a null key");
return make_dictionary_column(
make_column_from_scalar(s, 1, stream, mr),
Expand Down