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

List element Equality comparator #10289

Merged
merged 129 commits into from
Apr 13, 2022
Merged

Conversation

devavret
Copy link
Contributor

@devavret devavret commented Feb 14, 2022

This PR implements equality comparator for LIST columns. This only supports "self" comparison for now, meaning the two rows to be compared should belong to the same table. A comparator that works on rows of two different tables will be implemented in another PR.

This works only on "sanitized" list columns. See #10291 for details.

This will partially support #10186.

@devavret devavret added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Feb 14, 2022
@github-actions github-actions bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Feb 14, 2022
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

I can't seem to comment on the start indexes discussion anymore, but the commit you linked shows a good example of what you're aiming for so I'm going to go ahead and resolve that discussion. There are a couple of small outstanding tasks (documenting the safe template parameter and making a decision on the curr_col/temp_col/prev_col naming) but otherwise LGTM! Really awesome work here.

cpp/include/cudf/detail/iterator.cuh Show resolved Hide resolved
cpp/include/cudf/detail/iterator.cuh Show resolved Hide resolved
void const* data,
bitmask_type const* null_mask,
size_type offset)
CUDF_HOST_DEVICE column_device_view_base(data_type type,
Copy link
Contributor

Choose a reason for hiding this comment

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

strictly speaking, this macro is really only necessary in a .hpp file that is expected to work in both host and device code TUs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change made as part of this feedback #10289 (comment). I can revert the macro changes made to all .cuh headers.

Copy link
Contributor

Choose a reason for hiding this comment

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

strictly speaking, this macro is really only necessary in a .hpp file that is expected to work in both host and device code TUs.

I agree that it's not necessary in cuh. I made this suggestion because not everyone will think about that and people will invariably copy-paste, so uniformly using CUDF_HOST_DEVICE helps reduce the chance of future errors and leaves one less thing for developers to think about. It's a minor suggestion though, if you prefer to use it more surgically that's OK with me.

@devavret
Copy link
Contributor Author

rerun tests

@vyasr
Copy link
Contributor

vyasr commented Apr 12, 2022

@gpucibot merge

@vyasr vyasr requested a review from a team April 12, 2022 23:26
@vyasr
Copy link
Contributor

vyasr commented Apr 12, 2022

The merge is blocked by needing an ops review because of the one-line change to the conda recipe. I've made that request.

@rapids-bot rapids-bot bot merged commit 0ea6f8e into rapidsai:branch-22.06 Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Needs Review Waiting for reviewer to review or respond CMake CMake build issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants