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

Add a new factory function : make_fixed_width_column() #3461

Merged
merged 5 commits into from
Nov 26, 2019

Conversation

nvdbaranec
Copy link
Contributor

Adds an overload to allocate_like() that takes raw inputs instead of deriving from an input column. The primary use case for this is callers of the form:

out column = caller(scalar, scalar, column);

where the type of the output column is derived from the scalar inputs, but the size comes from the column input. Concrete example:

copy_if_else(scalar lhs, scalar rhs, column boolean_mask)
{
   out = allocate_like(lhs.type(), mask.size());
}

…state as individual params instead of deriving them from

an existing column.  The use case for here is something like:

out = copy_if_else(scalar, scalar, column boolean_mask)

where the type is dictated by the scalar inputs, but the size is dictated by the boolean_mask column.
@nvdbaranec nvdbaranec requested a review from a team as a code owner November 26, 2019 16:50
@jrhemstad
Copy link
Contributor

Instead of an allocate_like, this really should be a new factory:

unique_ptr<column> make_fixed_width_column(data_type type, size_type size, mask_state state, ...);

I originally split make_numeric_column and make_timestamp_column because I wasn't sure if the timestamp column would require additional arguments. But now that I've had some time to think about it, I'm pretty sure any additional arguments would be encapsulated into the data_type argument.

As such, I'd just make this code into the new factory.

…tion for fixed width types, make_fixed_width_column.
@nvdbaranec nvdbaranec requested a review from jrhemstad November 26, 2019 18:17
cpp/src/column/column_factories.cpp Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 26, 2019

Codecov Report

Merging #3461 into branch-0.12 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           branch-0.12    #3461   +/-   ##
============================================
  Coverage        87.35%   87.35%           
============================================
  Files               49       49           
  Lines             9329     9329           
============================================
  Hits              8149     8149           
  Misses            1180     1180

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5189050...2d261de. Read the comment docs.

@nvdbaranec nvdbaranec changed the title Add a new overload to allocate_like() Add a new factory function : make_fixed_width_column() Nov 26, 2019
@nvdbaranec nvdbaranec requested a review from jrhemstad November 26, 2019 19:13
cpp/include/cudf/column/column_factories.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/column/column_factories.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/detail/copy.hpp Outdated Show resolved Hide resolved
@nvdbaranec nvdbaranec requested a review from jrhemstad November 26, 2019 20:41
@harrism harrism merged commit 5ea5777 into rapidsai:branch-0.12 Nov 26, 2019
@harrism
Copy link
Member

harrism commented Nov 26, 2019

Just realized this is in branch-0.12. Is it needed in branch-0.11?

trxcllnt pushed a commit to trxcllnt/cudf that referenced this pull request Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants