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

[REVIEW] Implement type_dispatcher #379

Merged
merged 48 commits into from
Nov 27, 2018

Conversation

jrhemstad
Copy link
Contributor

@jrhemstad jrhemstad commented Nov 13, 2018

The purpose of this new function is to alleviate some of the pain of having to reconstruct the type of the type-erased data buffer from a gdf_column.

In more detail, the problem is that a gdf_column is type-erased, i.e., in the struct:

struct gdf_column{
void* data;
gdf_dtype dtype;
}
Where data points to a contiguous data buffer on the device, and the gdf_dtype enum tells us what type is actually contained in the buffer.

This requires a great deal of code of the form:

switch(gdf_column.dtype)
{
case GDF_INT: // cast gdf_column.data to int
case GDF_DOUBLE: // cast gdf_column.data to double
case GDF_FLOAT: // cast gdf_column.data to float
... 15 and counting total cases
}
Currently this particular switch logic is repeated all over the source of libgdf. Apart from being an eye-sore, it is also a nightmare for maintainability. Any time a new type is added (or removed), then a great deal of source code will need to be changed, which is prone to introduce bugs.

The new gdf_type_dispatcher function will centralize this switch logic to exactly one place. This will be a boon to both maintainability as well as code readability.

This PR introduces the gdf_type_dispatcher as well as updates some of the existing code to make use of this function, including:

  • get_column_byte_width
  • gdf_table::copy_row
  • gdf_table::hash_row
  • gdf_table::rows_equal.

It is not the goal of this PR to update all of the code-base to use the gdf_type_dispatcher, but rather to introduce it and show how it may be used.

This PR also updates the C++ standard used to C++14. This is required for the automatic return type deduction of the gdf_type_dispatcher.

@jrhemstad jrhemstad changed the title [WIP] Implement gdf_type_dispatcher [REVIEW] Implement gdf_type_dispatcher Nov 13, 2018
@jrhemstad jrhemstad added 3 - Ready for Review Ready for review by team cuda labels Nov 13, 2018
@jrhemstad jrhemstad requested a review from harrism November 14, 2018 00:05
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Overall looks good. A bunch of minor comments. One concern about testing.

libgdf/src/column.cpp Outdated Show resolved Hide resolved
libgdf/src/gdf_table.cuh Outdated Show resolved Hide resolved
libgdf/src/gdf_table.cuh Outdated Show resolved Hide resolved
libgdf/src/gdf_table.cuh Outdated Show resolved Hide resolved
libgdf/src/gdf_table.cuh Outdated Show resolved Hide resolved
libgdf/src/gdf_table.cuh Outdated Show resolved Hide resolved
libgdf/src/gdf_table.cuh Outdated Show resolved Hide resolved
libgdf/src/gdf_type_dispatcher.cuh Outdated Show resolved Hide resolved
libgdf/src/gdf_type_dispatcher.cuh Outdated Show resolved Hide resolved
libgdf/src/tests/column/column-test.cu Outdated Show resolved Hide resolved
libgdf/src/gdf_table.cuh Outdated Show resolved Hide resolved
libgdf/src/types.cuh Outdated Show resolved Hide resolved
libgdf/src/types.cuh Outdated Show resolved Hide resolved
@harrism harrism mentioned this pull request Nov 25, 2018
@harrism harrism added the libcudf Affects libcudf (C++/CUDA) code. label Nov 25, 2018
@harrism
Copy link
Member

harrism commented Nov 25, 2018

This is the next PR I would like to merge, because other PRs (I count at least 3 currently) can then be updated to use its functionality. @jrhemstad, is it ready once the conflicts are resolved?

@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

@harrism harrism added 0 - Waiting on Author Waiting for author to respond to review and removed 3 - Ready for Review Ready for review by team labels Nov 25, 2018
@harrism
Copy link
Member

harrism commented Nov 26, 2018

add to whitelist

Copy link
Collaborator

@eyalroz eyalroz left a comment

Choose a reason for hiding this comment

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

These changes are independent of the type_dispatcher, and I suggest they be merged separately to prevent confusion / for orderliness' sake.

@jrhemstad
Copy link
Contributor Author

@eyalroz not sure what changes you're referring to, but there's still some issues I'm ironing out with the submodules after merging in master after the refactor.

@eyalroz
Copy link
Collaborator

eyalroz commented Nov 27, 2018

@eyalroz not sure what changes you're referring to, but there's still some issues I'm ironing out with the submodules after merging in master after the refactor.

I was sure I saw changes in Rename libgdf namespace to cudf which were in CMakeLists.txt or other such files... maybe they disappeared somehow. Never mind, ignore my last comment.

@jrhemstad
Copy link
Contributor Author

This is the next PR I would like to merge, because other PRs (I count at least 3 currently) can then be updated to use its functionality. @jrhemstad, is it ready once the conflicts are resolved?

@harrism conflicts are resolved and this is ready for final review/merge.

@jrhemstad jrhemstad added 3 - Ready for Review Ready for review by team and removed 0 - Waiting on Author Waiting for author to respond to review labels Nov 27, 2018
jrhemstad and others added 2 commits November 27, 2018 13:24
@harrism harrism merged commit 44952a3 into rapidsai:master Nov 27, 2018
@eyalroz
Copy link
Collaborator

eyalroz commented Nov 28, 2018

Congratulations @jrhemstad for a well-deserved merge :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants