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 all commits
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
3 changes: 3 additions & 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 @@ -109,6 +109,8 @@ class column {
* @note This constructor is primarily intended for use in column factory
* functions.
*
* @throws cudf::logic_error if `size < 0`
*
* @param[in] dtype The element type
* @param[in] size The number of elements in the column
* @param[in] data The column's data
Expand All @@ -133,6 +135,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
7 changes: 6 additions & 1 deletion cpp/include/cudf/column/column_factories.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ std::unique_ptr<column> make_empty_column(type_id id);
*
* @throws std::bad_alloc if device memory allocation fails
* @throws cudf::logic_error if `type` is not a numeric type
* @throws cudf::logic_error if `size < 0`
*
* @param[in] type The desired numeric element type
* @param[in] size The number of elements in the column
Expand Down Expand Up @@ -119,6 +120,7 @@ std::unique_ptr<column> make_numeric_column(
* @note The column's null count is determined by the requested null mask `state`.
*
* @throws cudf::logic_error if `type` is not a `fixed_point` type.
* @throws cudf::logic_error if `size < 0`
*
* @param[in] type The desired `fixed_point` element type.
* @param[in] size The number of elements in the column.
Expand Down Expand Up @@ -176,6 +178,7 @@ std::unique_ptr<column> make_fixed_point_column(
*
* @throws std::bad_alloc if device memory allocation fails
* @throws cudf::logic_error if `type` is not a timestamp type
* @throws cudf::logic_error if `size < 0`
*
* @param[in] type The desired timestamp element type
* @param[in] size The number of elements in the column
Expand Down Expand Up @@ -234,6 +237,7 @@ std::unique_ptr<column> make_timestamp_column(
*
* @throws std::bad_alloc if device memory allocation fails
* @throws cudf::logic_error if `type` is not a duration type
* @throws cudf::logic_error if `size < 0`
*
* @param[in] type The desired duration element type
* @param[in] size The number of elements in the column
Expand Down Expand Up @@ -292,6 +296,7 @@ std::unique_ptr<column> make_duration_column(
*
* @throws std::bad_alloc if device memory allocation fails
* @throws cudf::logic_error if `type` is not a fixed width type
* @throws cudf::logic_error if `size < 0`
*
* @param[in] type The desired fixed width type
* @param[in] size The number of elements in the column
Expand Down Expand Up @@ -366,7 +371,7 @@ std::unique_ptr<column> make_fixed_width_column(
* @param[in] stream CUDA stream used for device memory operations and kernel launches.
* @param[in] mr Device memory resource used for allocation of the column's `null_mask` and children
* columns' device memory.
* @return Constructed strings column
* @return Constructed strings column
*/
std::unique_ptr<column> make_strings_column(
cudf::device_span<thrust::pair<const char*, size_type> const> strings,
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